Problem/Motivation

EntityQueryTest fails currently with PostgreSQL as database backend.

Failing test is EntityQueryTest::testSort(). The problem is the way that fields with the value NULL are sorted. MySQL default behavior, the behavior expected by Drupal, is to sort NULL first when in ascending order and last in descending order per its documentation.

Proposed resolution

Update the postgreSQL Select::orderBy() method, so that ascending orderby's order NULL values first and descending orderby's order NULL values last.

Remaining tasks

Write patch.
(Maybe) Create a follow-up feature request to support arbitrary NULL sorting for 8.1.x or 9.0.x???

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#14 interdiff.txt1.45 KBbzrudi71
#14 2443657-14.patch3.86 KBbzrudi71
#8 interdiff-2443657-4-7.txt1.16 KBdaffie
#8 2443657-7.patch3.6 KBdaffie
#4 2443657-4.patch2.44 KBdaffie

Comments

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
Issue summary: View changes

Filled in some details.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented

I am guessing that the second fail in the table in the issue summary is also due to the pager query. There also could be some sorting issues with EntityQuery. Didn't we have to look at Group By/Order By for EntityQuery?

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
Issue summary: View changes

Yep, I forgot about that change we made last year.

daffie’s picture

daffie commented
Status: Active » Needs review
StatusFileSize
new 2443657-4.patch2.44 KB

With this patch the EntityQueryTest passes for postgreSQL.

bzrudi71’s picture

bzrudi71 commented

Wow, great research daffie! Let's do a full testrun to make sure this has no side effects on other tests. Will do a full test run now and report the results later.

bzrudi71’s picture

bzrudi71 commented

Test run completed. Can confirm we have pass in EntityQueryTest with patch applied. I found exactly one new fail in:

Drupal\system\Tests\Database\SelectComplexTest 93 passes 1 fails

which seems to be caused by this patch. I really like the simplicity of this patch so we should see if we can get pass in SelectComplexTest again.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

- Updating status.
- Issue summary probably needs to be fixed so that it explains the change (http://www.postgresql.org/docs/8.3/static/queries-order.html).

daffie’s picture

daffie commented
Status: Needs work » Needs review
StatusFileSize
new 2443657-7.patch3.6 KB
new interdiff-2443657-4-7.txt1.16 KB

Updated the SelectComplexTest.

daffie’s picture

daffie commented
Issue summary: View changes
Issue tags: -Needs issue summary update

Updated the IS.

bzrudi71’s picture

bzrudi71 commented
Issue summary: View changes 
  1. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Select.php
@@ -52,11 +52,12 @@ public function orderRandom() {
+    $direction = strtoupper($direction) == 'DESC' ? 'DESC NULLS LAST' : 'ASC NULLS FIRST';

    Pretty sure alexpott likes a longer comment here why null first or last :-)
    Maybe we could add a @see http://www.postgresql.org/docs/9.3/static/queries-order.html here. 

  2. +++ b/core/modules/system/src/Tests/Database/SelectComplexTest.php
@@ -220,7 +221,9 @@ function testCountQueryRemovals() {
+    // The orderby string is different for postgreSQL.

    nitpick: postgreSQL -> PostgreSQL.
    Also I think we should add @see Drupal\Core\Database\Driver\pgsql\Select::orderBy()

Updated IS, as I think it should read ascending orderby's order NULL values first instead of ascending orderby's order NULL values last

@mradcliffe: what do you think?

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented

I was going to run the patch through all of the Entity tests, but I was having trouble rebuilding web-php55 container tonight.

Thank you for update the issue summary.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
Status: Needs review » Needs work

Ran Entity and Database test groups locally on my vagrant vm. Only exceptions related to #2443659: PostgreSQL: Fix system\Tests\Entity\FieldSqlStorageTest and #2443663: PostgreSQL: Fix system\Tests\Entity\EntityDefinitionUpdateTest.

Setting to needs work per review in #10 by @bzrudi.

Perhaps this as the comment?

"Emulate MySQL default behavior to sort NULL values first for ascending, and last for descending."

This is defined in MySQL documentation section 3.3.4.6 - Working with Null

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
Issue summary: View changes

- Updated issue summary with link to MySQL documentation.

Also, maybe an @todo could be added in the default Select class to note that we might want to support NULL sorting in the future? The documentation standards mention that @todo should have a follow-up issue link, but I think that's premature in this case because the future is unknown for 9.0.x.

bzrudi71’s picture

bzrudi71 commented
Status: Needs work » Needs review
StatusFileSize
new 2443657-14.patch3.86 KB
new interdiff.txt1.45 KB

Let's move forward here. Regarding the @todo follow-up in the default select class let's wait for comment from the commiters?

daffie’s picture

daffie commented

All the comments are for me now RTBC. The rest of the patch is for somebody else to review.

@bzrudy71: Do you have a IRC account or is there any other way I can contact you?

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me, and passes Entity and Database tests.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 9b5042c and pushed to 8.0.x. Thanks!

  • alexpott committed 9b5042c on 8.0.x 
    Issue #2443657 by daffie, bzrudi71: PostgreSQL: Fix system\Tests\Entity\...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

adam-vessey’s picture

adam-vessey commented

Not looking to re-open this, but: Got poking around a bit recently with index usage in PostgreSQL, and it seems like the changes made here on their own would interfere with index usage in PostgreSQL, at least with ordering operations since indexes in PostgreSQL: https://www.postgresql.org/docs/current/indexes-ordering.html

By default, B-tree indexes store their entries in ascending order with nulls last

So, since the "order by" statements no longer matches the indexes (as they're created by default), things are likely to fall back to sequential scans in situations where it otherwise would be fine.

Could/should we start adding "NULLS FIRST" to general index definitions that we generate for PostgreSQL, such that indexes will match the expectations of "order by" statements we pass it, and so facilitate their use?

chi’s picture

chi commented
Related issues: +#3359406: Postgres: Sorting NULL values causes performance degradation

Added a follow-up for #20.