PostgreSQL orderBy method adds fields it doesn't need to, leading to fatal errors when the result is used as an insert subquery

Sorry! No idea how the component got changed.

I had this problem working on FACETS.

Here is the query:

    // Adds subquery to group the results in the r table.
    $subquery = db_select($this->query, 'r')
      ->fields('r', array('value'))
      ->groupBy('r.value')
      ->orderBy('count', 'DESC');

    // Adds COUNT() expression to get facet counts.
    $subquery->addExpression('COUNT(r.value)', 'count');

And here is the error:

Uncaught PHP Exception Drupal\\Core\\Database\\DatabaseExceptionWrapper: "SQLSTATE[42703]: Undefined column: 7 ERROR:  column "count" does not exist\nLINE 1: SELECT r.value AS value, count AS count, COUNT(r.value) AS c...\n                                 ^: SELECT r.value AS value, count AS count, COUNT(r.value) AS count\nFROM \n(SELECT n.type AS value\nFROM \n{search_index} i\nINNER JOIN {node_field_data} n ON n.nid = i.sid AND n.langcode = i.langcode\nINNER JOIN {search_total} t ON i.word = t.word\nINNER JOIN {search_dataset} d ON i.sid = d.sid AND i.type = d.type AND i.langcode = d.langcode\nWHERE ...

I've fixed the problem adding the expression before the orderBy() and it works because we verify the expression from the Driver:

    // Also check expression aliases.
    foreach ($this->expressions as $expression) {
      if ($expression['alias'] == $this->connection->escapeAlias($field)) {
        return $this;
      }
    }

So the solution is something like this:

    // Adds subquery to group the results in the r table.
    $subquery = db_select($this->query, 'r')
      ->fields('r', array('value'))
      ->groupBy('r.value');

    // Adds COUNT() expression to get facet counts.
    $subquery->addExpression('COUNT(r.value)', 'count');

    $subquery->orderBy('count', 'DESC');

@marthinal: Good work with solving the problem. Can you make a patch.

I am afraid that a test must be added o make sure that the problem is fixed and that is does not return.

The method Drupal\Core\Database\Driver\pgsql\Select::orderBy adds fields that are needed for the order by clause. Lets remove that part and see what the testbot does.

This problem is not solvable. What they do in #1759144: taxonomy_update_7005 on pgsql returns error "column "td" of relation "taxonomy_update_7005" does not exist" is the following:

    $query = db_select('test_people', 'tp')
      ->fields('tp', array('name', 'job', 'age'))
      ->condition('tp.name', 'Meredith')
      ->orderBy('tp.id');

    db_insert('test')
      ->from($query)
      ->execute();

This result in the following query:

INSERT INTO test (age, name, job)
 SELECT tp.age AS age, tp.name AS name, tp.job AS job
 FROM test_people tp
 WHERE tp.name = 'Meredith'
 ORDER BY tp.id

And this is a query that PostgreSQL does not like. The column "td.id" is not added to the select part of the query and results in: "SQLSTATE[42703]: Undefined column: 7 ERROR: column tp.id does not exist".
The documentation from Drupal\Core\Database\Driver\pgsql\Select::orderBy states:

PostgreSQL adheres strictly to the SQL-92 standard and requires that when using DISTINCT or GROUP BY conditions, fields and expressions that are ordered on also need to be selected.

Test added to prove my point.

In the method Drupal\Core\Database\Driver\pgsql\Select::orderBy the order by field is added to the select fields. In the patch is that part removed. To show that that will not fix the problem.

I did it wrong. Should have selcted from the test table and inserted in the test_people table. Not the other way around. The test table has an extra column ("id").

Next try.

Added a test to make sure that we do not need the addField functionality in Drupal\Core\Database\Driver\pgsql\Select::orderBy.

I have tried to find out when the addField functionality was added to Drupal\Core\Database\Driver\pgsql\Select::orderBy. Why it was added and for which issue/use case. The most I could find out was that @webchick added it on 2010-11-29 for Drupal 7.x and it was the initial commit. So not very helpful.

Please ignore my comments #9 - #12, because they are wrong. Wrong tests with wrong conclusions.

I have added two test:

1. Test for checking if a select query with a order by clause can be used by an insert query. This test only passes with the fix.
2. Test for checking if the code that is removed by the fix is necessary what that code suppoost to be doing. That test passes with the code removed. So that code to not necessary and can be removed.

This issue is ready for a good review.

Interesting to see no fails with all that code removed. Maybe this was added back in D7 for some special upgrade path requirement or such, no idea. I really like the idea to get rid of all that code and test coverage shows it is indeed not needed for D8 to pass tests. I will try to test the patch over the weekend on a real D8 site with PG as database backend (and lot's of own modules using orderby's) before doing review.
Thanks @daffie!

It took some time, but here we go :)
I testet the patch locally and can't find anything broken, nice! Additionally we have passing tests and new test too. During review I just found some nits...

  /**
   * Overrides SelectQuery::orderBy().
   *
   * PostgreSQL adheres strictly to the SQL-92 standard and requires that when
   * using DISTINCT or GROUP BY conditions, fields and expressions that are
   * ordered on also need to be selected. This is a best effort implementation
   * to handle the cases that can be automated by adding the field if it is not
   * yet selected.
   *
   * @code
   *   $query = db_select('example', 'e');
   *   $query->join('example_revision', 'er', 'e.vid = er.vid');
   *   $query
   *     ->distinct()
   *     ->fields('e')
   *     ->orderBy('timestamp');
   * @endcode
   *
   * In this query, it is not possible (without relying on the schema) to know
   * whether timestamp belongs to example_revision and needs to be added or
   * belongs to node and is already selected. Queries like this will need to be
   * corrected in the original query by adding an explicit call to
   * SelectQuery::addField() or SelectQuery::fields().
   *
   * Since this has a small performance impact, both by the additional
   * processing in this function and in the database that needs to return the
   * additional fields, this is done as an override instead of implementing it
   * directly in SelectQuery::orderBy().
   */

needs some update/removal

and
* Tests that the INSERT INTO ... SELECT (fields) ... with ORDER BY.
the "that the" part is obsolete.

Other than that RTBC from my side

@bzrudi71: Thanks for the review. For both points: great find!.

Changing the parent issue to #2564307: [meta] Remaining Drupal 10/11 PostgreSQL issues.

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Select.php
@@ -21,33 +21,7 @@ public function orderRandom() {
-   *   $query = db_select('example', 'e');
-   *   $query->join('example_revision', 'er', 'e.vid = er.vid');
-   *   $query
-   *     ->distinct()
-   *     ->fields('e')
-   *     ->orderBy('timestamp');

Is there a test that tries to do a similar query? I tried searching for one, but didn't have much luck searching all the orderBy and join and distinct queries in core. Is it something we should test?

Sorry to not get to this within a reasonable time. :(

Edit: there's probably an appropriate table in database_test module that could be added to SelectComplexTest.

This type of query seems not to be used anywhere in core (I guess). And why should the pg driver be responsible of "automagically" adding fields to make it work? ;)

* In this query, it is not possible (without relying on the schema) to know
* whether timestamp belongs to example_revision and needs to be added or
* belongs to node and is already selected. Queries like this will need to be
* corrected in the original query by adding an explicit call to
* SelectQuery::addField() or SelectQuery::fields().

This may be true but I still think it makes more sense to change the query itself to do what one want's todo, no? :)

Updated issue summary.

I didn't mean to RTBC it myself.

Thanks a lot for the great IS update @mradcliffe! I'm happy with this massive code cleanup and the possible performance impact, so let's see what the committers think. :)
RTBC

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Select.php
@@ -56,58 +30,6 @@ public function orderBy($field, $direction = 'ASC') {
-
-    if ($this->hasTag('entity_query')) {
-      return $this;
-    }
-
-    // If there is a table alias specified, split it up.
-    if (strpos($field, '.') !== FALSE) {
-      list($table, $table_field) = explode('.', $field);
-    }
-    // Figure out if the field has already been added.
-    foreach ($this->fields as $existing_field) {
-      if (!empty($table)) {
-        // If table alias is given, check if field and table exists.
-        if ($existing_field['table'] == $table && $existing_field['field'] == $table_field) {
-          return $this;
-        }
-      }
-      else {
-        // If there is no table, simply check if the field exists as a field or
-        // an aliased field.
-        if ($existing_field['alias'] == $field) {
-          return $this;
-        }
-      }
-    }
-

So removing all this is great, but #890994: Entity query tests fail on PostgreSQL added this code, and I don't see the original issue mentioned here - so it'd be good to double check it won't regress that bug. Especially for any likely 7.x backport if not 8.x

Re-rolled

Thank you for the re-roll, @Saviktor. Nice, only deprecation items to fix up. :-)

  $query = db_select('test', 't')

I guess should be Connection->select('test', 't) instead along with the appropriate db_insert.

Thanks for review, @mradcliffe! Here is updated patch

I ran into this issue on D7, so I created a simple patch. I'm attaching it here in case anyone else would find it useful. It can be the start of a D7 backport, if there is demand for that. It does not include any updates to automated tests yet.

The D8 patch is still "Needs Review", per @catch's comment #29 above (and I assume the same will apply to the D7 patch).

@m.stenta sure you uploaded the right patch?

The patch for D9 needs to be rerolled.

Rerolled patch #39 for 9.1.x. Please review!

Fixing failed test

@m.stenta sure you uploaded the right patch?

@steinmb - I think so? Note that it was for D7, not D9, if that was confusing. We are including my D7 patch in the farmOS distribution to fix the issue with D7+PostgreSQL: https://github.com/farmOS/farmOS/blob/7.x-1.x/drupal-org-core.make

@m.stena I know. What confused me was that the patch removed the entire orderBy() and added nothing.

@steinmb Gotcha. :-)

Yes, to summarize (it's been a while so hopefully I am remembering correctly): the issue was that Drupal was overriding the default SelectQuery class when a PostgreSQL database was being used, which added additional logic to the SelectQuery::orderBy(). This logic caused the issue described here. So removing that method from the class causes Drupal to use the default orderBy() logic, which fixes the issue. My patch did not include any tests, so it was only removing code (the overridden method), not adding any.

Notably, we can't forget to review @catch's comment in #29 above:

So removing all this is great, but #890994: Entity query tests fail on PostgreSQL added this code, and I don't see the original issue mentioned here - so it'd be good to double check it won't regress that bug. Especially for any likely 7.x backport if not 8.x

Hope that makes sense!

Aha! :-D Thank you for taking the time getting me up to speed. I really appreciate it.

Update remaining tasks.

The test coverage added here addresses the problem found in this issue, the insert query. However, the issue that caused the problematic code #890994: Entity query tests fail on PostgreSQL does not appear to have added test coverage for the problem it was addressing.

Therefore I think the only way we can know if it is safe to remove this code is to add test coverage for the original problem:

* PostgreSQL adheres strictly to the SQL-92 standard and requires that when
* using DISTINCT or GROUP BY conditions, fields and expressions that are
* ordered on also need to be selected.

If the code is needed, the simplest fix here might be to wrap it in a conditional so it is only used when needed or not used in an insert.

I asked @berdir (who wrote the original code in #890994: Entity query tests fail on PostgreSQL). He said:

to be honest, no idea. I'm not an expert on postgresql at all, i just helped to fix tests. that was for d7 entity query, which is an entirely different beast than d8. also, today, even mysql requires that you add things you group on to be selected, so it is indeed quite likely that postgresql no longer requires special logic and I have no objections to removing whatever code I added there.
(in strict mode at least)

> However, the issue that caused the problematic code #890994: Entity query tests fail on PostgreSQL does not appear to have added test coverage for the problem it was addressing.

We did not have PostgreSQL testing the way we do today. we were working our way through the failing tests on PostgreSQL. I didn't add new tests because it didn't have to, it fixed existing fails in HEAD.

So no, IMHO this does not require new tests.

Back in the day we had a number of strange fixes for PostgreSQL when you ran queries that were made for MySQL. Most if not all are fixed. As the subsystem maintainer I have confidence that we can remove the code. In the worsed case some code will fail. I do not think so, but I cannot oversee everything. Nothing in Drupal core breaks. We have new tests that show that it is not happening any more. For me it is a go.

1. +++ b/core/tests/Drupal/KernelTests/Core/Database/InsertTest.php
   @@ -173,6 +173,29 @@ public function testInsertSelectFields() {
   +    $this->connection->insert('test_people')
   +      ->from($query)
   +      ->execute();
   

   We should add a comment that the table "test_people" does not have a column called "id". Therefor if the PostgreSQL database adds the column to the result of the select query with order by, the insert query should fail.

2. +++ b/core/tests/Drupal/KernelTests/Core/Database/SelectTest.php
   @@ -425,6 +425,19 @@ public function testUnionOrderLimit() {
   +      ->fetchCol();
   

   Could we change this to fetchAll() and test that we have only one column "name".

The patch needs a reroll.

Reroll the patch #50 with Drupal 10.1.x

Hi Ankit,

Thank you for the reroll. I have 2 remarks:

1. The change to web/core/modules/pgsql/src/Driver/Database/pgsql/Select.php is missing

2. +++ b/core/tests/Drupal/KernelTests/Core/Database/InsertTest.php
   @@ -175,6 +175,29 @@ public function testInsertSelectFields() {
   +  function testInsertSelectFieldsOrderby() {
   
   +++ b/core/tests/Drupal/KernelTests/Core/Database/SelectTest.php
   @@ -426,6 +426,19 @@ public function testUnionOrderLimit() {
   +  function testOrderbyColumnNotInSelect() {
   

   These 2 methods need to be public methods.

Updated patch address #65 and fix CCF #64
assertEqual( ) is removed from drupal:10.x so we need to Use assertEquals( ) instead.

Fixing CCF #66
assertIdentical( ) is deprecated in 10.x so use assertSame( ) instead

I changed the status Needs work per @daffie's comment in #50 since the recent patches are incomplete. Hopefully including the changes will keep it green.

Rerolled the patch, with the code from #67 and #50.

The asterisks in the comment missed a space.

Adjusted the PHPStan baseline, because there was a error for a variable that we have deleted with this patch.

Accidentally added the updated filemode 755, but should have been 644.

All changes look good to me.
The removed special code for PostgreSQL is for a version from a long time ago.
The added testing shows that the special code is no longer necessary.
For me it is RTBC.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Problem with the bot disregard #75

Back to RTBC.

Back to RTBC

I think #61 addresses #29, removing that from the issue summary

Issue credits

I did some debugging of this, and was able to produce an entity query that errors on postgres with the following interdiff.

Is this something we should be supporting or is postgres stricter on that front? It works with MySQL.

I note this fails in HEAD too, so I assume 'not supported' on postgres (or rather - not valid SQL full stop, MySQL is just more tolerant).

Either way, this no longer applies - NW for that