Search results page broken on PostgreSQL

Thanks! Simple fix and looks correct to me, from what I know about PostgreSQL vs. MySQL.

I am much less sure about it. Not saying it's wrong because I do not have a good grasp on what the table contains multilanguage wise but a) shouldn't it be the first in grouping b) instead of grouping; if it's always the same in a group; why not MAX(i.langcode) AS langcode in select and be done? Of course FIRST or LAST would be the best but guess what? MySQL doesn't support that.

I'm not sure about the order of groupings. Does it matter?

But yes I think it needs to be a group by and not a max(), first() etc. For a given value of sid, there can be multiple languages. For instance, when the Node module uses the search index to index a node, it adds an entry for each language that exists on the node, and "sid" here is the node ID.

Sorry, I was wrong, the order of grouping (here) doesn't matter. In ANSI SQL it doesn't matter and we are not using the MySQLisms where it does.

I was too quick to set this to RTBC, as pointed out by alexpott in IRC today.

So. The problem is that i.langcode is not always added to the search query, and in particular, at the time of SearchQuery::executeFirstPass(), in particular, it has not yet been added to the query, and it will not be there for all search queries for general search plugins.

So, let's step back here and look at NodeSearch::execute().

First off, this method will add i.langcode to the query if the user is running an advanced search that is limited by language. This code will most likely not work in PostgreSQL because it is adding where() clauses without necessarily adding the fields to the query. So we should test advanced searches in PostgreSQL and see if they are working.

Second, this method will add i.langcode **after excecuteFirstPass()**, in this code:

    $find = $query
      // Add the language code of the indexed item to the result of the query,
      // since the node will be rendered using the respective language.
      ->fields('i', array('langcode'))
      ->limit(10)
      ->execute();

So here, where it does fields() it also needs to add the groupBy there.

So we need a new patch and we also need to test advanced searches on PostgreSQL to see if they work.

Did you test with your earlier (incorrect) patch in there? That could be a problem.

I think we have tests (I hope we have tests!) that verify the language advanced search works, and if so they have been run on MySQL... I'm testing now and will report back shortly.

I just tested and I agree that in MySQL the advanced search is totally broken by language. I've just filed a separate issue on that:
#2161067: Advanced search with Language filter does not work

So let's in this issue try to just fix the PostgreSQL bug on plain non-advanced search.

I installed D8 with PostgreSQL today and tested this. It was still broken, and running the existing Search module tests was sufficient to trigger this error.

I analyzed what was going on. The error you get is:

PDOException: SQLSTATE[42803]: Grouping error: 7 ERROR: column "i.langcode" must appear in the GROUP BY clause or be used in an aggregate function LINE 1: SELECT i.langcode AS langcode, i.type AS type, i.sid AS sid,... ^ in PDOStatement->execute() (line 61 of /home/jhodgdon/gitrepos/drupal_pg8/core/lib/Drupal/Core/Database/Statement.php).

This is happening at the main execute() step in SearchQuery (after the first pass query is done). The reason is that NodeSearch has a line saying:

   // Load results.
    $find = $query
      // Add the language code of the indexed item to the result of the query,
      // since the node will be rendered using the respective language.
      ->fields('i', array('langcode'))

So it is adding the i.langcode field to the query. This doesn't work in PostgreSQL because the SearchQuery::execute() method has made it ba an aggregate query:

   $this
      ->condition('i.type', $this->type)
      ->groupBy('i.type')
      ->groupBy('i.sid')
      ->having('COUNT(*) >= :matches', array(':matches' => $this->matches));

So now that NodeSearch is adding a field, it also needs to either add a groupBy for the field, or aggregate it.

I believe in this case that the correct answer is to add a groupBy -- we would want to have each translation of each node as a separate search result.

So, the attached patch fixes the problem.

Do we have a way to trigger a PostgreSQL test? On my local machine, it is fixing the tests so that instead of mostly failing, they mostly work... So far at least; I'm still running them and they take a while, but tests that were failing before this patch are now passing, so I've verified by manually running the existing Search module tests on PostgreSQL that it works. I also verified manually that without this patch, I get the above error when I try to run any search on nodes, and with this patch, node searching works.

Also, functionality that is broken completely for PostgreSQL is a "major" bug.

Rather than +1 for RTBC, how about just marking it RTBC? Or I will, based on comment #16 -- it's a one-line fix anyway and fairly obvious I think... Thanks for the review!

Committed e17552a and pushed to 8.x. Thanks!