do_search() fails hard on Postgres

As score is not aggregated this is right to fail. We need like MAX MIN or SUM in there.

There are two issues on PostgreSQL: first, PostgreSQL mandates that aggregate functions be used in all non-grouping columns (fair enough), second POWER() on two numeric columns is treated in arbitrary codecision and thus fails with an overflow error.

Before the patch:

MySQL 5.0:
Search engine queries 105 passes, 0 fails, 0 exceptions
Bike shed 16 passes, 0 fails, 0 exceptions
Search engine ranking 26 passes, 0 fails, 0 exceptions

PostgreSQL 8.3:
Search engine queries 105 passes, 0 fails, 0 exceptions
Bike shed 16 passes, 0 fails, 0 exceptions
Search engine ranking 21 passes, 5 fails, 20 exceptions

After the patch:

MySQL 5.0:
Search engine queries 105 passes, 0 fails, 0 exceptions
Bike shed 16 passes, 0 fails, 0 exceptions
Search engine ranking 26 passes, 0 fails, 0 exceptions

PostgreSQL 8.3:
Search engine queries 105 passes, 0 fails, 0 exceptions
Bike shed 16 passes, 0 fails, 0 exceptions
Search engine ranking 26 passes, 0 fails, 0 exceptions

I still have a failure on Search engine ranking on MySQL 5.1 (see #276039: Test "Search engine ranking" fails on MySQL 5.1), both before and after the patch.

Because this breaks the functionality on PostgreSQL, this is a critical issue.

Promoted to the PostgreSQL surge.

I looked at Search module and found this issue:

line 952:
$select = "SELECT SUM(i.score * t.count) AS calculated_score FROM {search_index} i $join WHERE $conditions GROUP BY i.type, i.sid HAVING COUNT(*) >= %d ORDER BY calculated_score DESC";

This query will always fail under PostgreSQL.

The correct query would be:
$select = "SELECT SUM(i.score * t.count) AS calculated_score, i.type, i.sid FROM {search_index} i $join WHERE $conditions GROUP BY i.type, i.sid HAVING COUNT(*) >= %d ORDER BY calculated_score DESC";

as GROUP BY columns must appear in the SELECT clause.

My knowledge of D7 internals is not sufficient to provide a patch, because I don't understand (yet) how is used $select.
Can someone fix and submit a patch?

@jmpoure: there is already a patch in #2 (that may need a reroll).

No, we need to backport this, so let's fix it in the old way, and only then convert those parts to the new database API.

Here we are, reroll of #2.

Ok there was three issues here:
(1) As explained above, the queries were wrong, because all some non grouping columns were not aggregated
(2) PostgreSQL needs explicit casts in expressions, without them it defaults to INTEGER
(3) the test was not refreshing the variables AND the "recent" ranking score computing was wrong (it was (0.5^t)/t0 instead of 0.5^(t/t0))... and this only appears in our test when we fixed (1)...

Fixed. Ran flawlessly on MySQL, PostgreSQL and SQLite.

Patch fixes the search tests!

Wow. Awesome work Damien!

Arg...did not mean to change status.

have tested patch: 188 passes, 0 fails, and 0 exceptions for simple tests search module

works great :)

I've committed this patch to CVS HEAD. Admittedly, despite the fact that I spent about 20 minutes looking at the patch, I'm not sure I 100% grok the SUM()-related changes to do_search(). Frustrating patch, but I'll trust Damien's research and judgment. The fact I don't have PostgreSQL installed doesn't really help. :)

This will require a partial backport to Drupal 6.

Some background on the SUM() part: the standard search query is:

  SELECT i.type, i.sid, i.relevance AS score
    FROM {search_index} i
      INNER JOIN search_total t ON i.word = t.word
    WHERE i.word = ? AND i.type = ?
    GROUP BY i.type, i.sid HAVING COUNT(*) >= ?

i.type and i.sid are part of the GROUP BY clause, but not i.relevance. This is invalid SQL, even if we know that because (1) (type, sid) is a unique key for {search_index} and (2) t.work is a primary key, we can only have one i.relevance per (i.type, i.sid) couple (we are smarter than the planner...). We simply transform all i.relevance into SUM(i.relevance) to make the query planner happy.

Note that both MySQL and SQLite happily deals with that situation, even if the result can be quite undetermined, when the result set is not unique.

Given the off chance that some future change could result in > 1 relevance row per grouped set, it might be best to go with max(relevance) so that we don't start summing multiple rows together and getting odd results unexpectedly.

Unless summing them IS what we'd want should that somehow ever change (i.e. someone adds a table to the query that makes relevance non-unique).

Note that both MySQL / SQLLite and PostgreSQL technically misbehave as regards group by. MySQL's acceptance of items in the select list that aren't grouped by, aggregated, and can't be guaranteed unique is against spec (and some would say common sense.)

PostgreSQL OTOH, according to the spec, should note that the relevance field is unique per tuple output by group by and allow the query to run. But it's a check that isn't made and while it is a TODO item for pgsql, I don't think it's a priority for anyone.

Has this been fixed in the latest D6?

No, it's not fixed in Drupal 6. If it had been ported, there would be a patch above and the issue would be marked closed/fixed. If you would like to help, make a patch for Drupal 6.

Tagging

Talked with Gabor (the Drupal 6 branch maintainer) and D6 issues are really not being committed unless they're really essential -- we really don't have a test system for Drupal 6 and it's too dangerous. So... putting this back to D7 / fixed.