Fix changes to db_distinct_field() in D6.16 / PostgreSQL

Can we first discuss the significant regressions in PostgreSQL, which have appeared in Drupal 6.16, please? Seems best to do that before the D5 patch. (And I'm hijacking this bug report since the original thread is locked.)

Summary for this purpose:
* A bug in db_distinct_field() was found, which generates SQL with invalid syntax.
* In the process of fixing this, there was confusion about what db_distinct_field() really needed to do... which apparently got a perfectly good PostgreSQL patch from benoitg dropped, because it was a solution for a different problem. (At least, that's what I can make of it.)
* there were a remarks "let's just get this fix out and worry about Postgres later" (See comment #362)
* The fix (last tested by benoitg in #397 as "fixes the use case described in #278 on PostgreSQL") caused different behavior on (at least) PostgreSQL systems; DISTINCT is now added to several queries where it wasn't before. Sometimes this yields illegal SQL syntax.
* The fix got included in Drupal 6.16 and the following issues started appearing:
#742006: ERROR: for SELECT DISTINCT, ORDER BY expressions must appear in select list
#742060: ERROR: for SELECT DISTINCT, ORDER BY expressions must appear in select list
#742424: Error when upgrade to 6.16 from 6.15

The issue:

'SELECT fields FROM table ORDER BY orderfield' is only legal (on systems which follow the SQL92 standard) if 'orderfield' appears somewhere in 'fields'.

There are several places in Drupal code which have perfectly legal queries, but where some hook_db_rewrite_sql() sets $return['distinct'] = 1 -- which can yield illegal SQL on non-mysql systems.

database.pgsql.inc:db_distinct_field() apparently ignored this 'distinct' flag in cases where it was illegal (as indicated by the absence of database errors before D6.16 :-) ) ... but that particular db_distinct_field() is no more. It has been replaced by a general db_distinct_field() in database.inc, which adds the DISTINCT keyword always.

The scope of the error is at least comments.module. (See #742006/#2 for code.) The 'recent comments' block has disappeared on PostgreSQL installations, because node_db_rewrite_sql() sets the 'distinct' flag and creates an illegal query.
Outside of core, there are other modules which are harmed by this. #742006 & #742424 show that things break when the forum_access module is installed (which is because forum_access_db_rewrite_sql() sets the 'distinct' flag on an otherwise legal query which is in taxonomy.module)

The fix:

In above bug reports, people are trying to add fieldnames to the 'SELECT' list of queries, to fix things.
This is indeed a way to fix things... (since adding an extra field will not create problems in most cases...) but it seems dissatisfying because the queries in themselves (without DISTINCT) are perfectly legal.

A second way is to put the responsibility on hook_db_rewrite_sql() implementations, to make sure that they don't set the 'distinct' flag in cases where that's illegal. (Which is the strictest way from an SQL standpoint. And which is also the road to hell, IMHO.)

Another way is to ignore the 'distinct' flag in cases where it's illegal.

My patch does the last thing (but can do the second thing)... by introducing a function named db_distinct_allowed(). This is supposed to return FALSE if it is illegal to add DISTINCT to a query.
I'm calling the function from db_distinct_field() and leaving it inside database.inc... because you guys must have had a reason to strip the function from the database-specific include files. And also because you can play around with it according to your tastes.
You could elect to:
- scrap the test for $db_type mysql, in order to unify behavior on MySQL & Postgres. (Which may introduce changes to MySQL behavior, so you probably don't want that :) )
- call this from hook_db_rewrite_sql() implementations instead, like I said above in 'a second way'. Instead of $return['distinct'] = 1, everyone must then call $return['distinct'] = db_distinct_allowed($query)

Note that this function is far from perfect. It isn't meant to be perfect, and it's meant to be killed just as soon as db_distinct_field() is killed, and never thought of again. It's just a stop gap for the breakage introduced in D6.16.
...and since my stop gap only removes the DISTINCT keyword from the query in cases where PostgreSQL would choke on it... I am going to assume that all use cases that tested correctly before, still test correctly.

(Actually I would think that some code that benoitg wrote in the process of the preceding issue, would be more suitable than mine. But he's much more capable of commenting on that himself.)

As as general rule, I can see this.

But are you really going to port and include a patch:
- in code which isn't going to be working 100% cleanly, no matter what we do
- which fixes errors that were reported as related to Views 2 (which isn't available in D5)
- knowing that applying that patch will break basic core functionality on PostgreSQL systems

and then start thinking about fixing the breakage in D5... which currently does not exist yet?

I'll keep this in mind for next time (and will be watching the issue to ease problems that my "scope shift" has now created).

But I just hope this gets attention before the D5 patch port gets implemented, because D6.16/PostgreSQL has killed at least a functional
- comment_get_recent()
- taxonomy_node_get_terms() on any site which runs forum_access (and probably other taxonomy access control modules)

My patch is a start (or 'the final solution' if people want it to be), because it fixes these issues.

I'll be running it on my Postgres sites, and will modify it locally so that I'll spot possible side effects on my Mysql dev/non-critical sites too.

Well, here is my comment on this (as requested by roderik).

I must admit than silently removing a distinct explicitely requested by a hook in cases when it may not work in postgres is much worse, and much more likely to return different results than under MySql, than rewriting the query to make it sql92 compliant, especially since we should be able to make it SQL92 compliant for mysql as well with no ill effect.

If memory serves all the code required to add order by fieldnames in select list if not already present (including some applicable unit tests) should be available with little modifications in one of my original patch for http://drupal.org/node/284392.

I'm willing to resurrect this code and integrate it in this new context, but I need volunteers willing to commit to testing it with the modules exhibiting problems.

For the record: here's one. I'll test. I haven't used forum_access yet, but will install/configure a psql (+ mysql if needed) site and test any results...

And thank you for your offer to dig up your code.

(I would've used your code as a starting point for a patch, instead of doing the above, except
- my eyes glazed over when I saw the original database.pgsql.inc:db_distinct_field() - and made me assume that different SQL was already being returned on Postgres
- they did the same when trying to determine from the original discussion, whether or not there had been a scope change after your patch :-)
)

@roderik Just wondering if you have seen this other related thread, http://drupal.org/node/681760

Ok, I'll get on it at the begining of next week. If I don't, feel free to poke me by email.

@izmeez: I've seen garywiz' proposal discussed. I'm not qualified to comment on whether his approach is "better".

But if I'd realized that this solves my personal problem... I would not have made my own patch :-)

So what we have now:

1) #681760: Try to improve performance and eliminate duplicates caused by node_access table joins - eliminates the problem by simply not setting "$return['distinct'] = 1" anywhere in D6 core, so db_distinct_field() does not get called.
It's a practical solution, but does not solve the error when forum_access.module is enabled, which is reported in #742006 / #742424. forum_access.module -and any other applicable code- would need to be adjusted to make sure that db_distinct_field() is evaded whenever it might create trouble.

2) my patch in comment#1 above.
It's a practical solution, but runs the risk of returning duplicate rows on PostgreSQL.

3) a change to db_disctinct_field() that makes queries SQL92 compliant always - as mentioned in #4 above. I'm sorry but I'm unable to isolate that code from the discussion in #284392: db_rewrite_sql causing issues with DISTINCT, myself.

I don't know about Postgres, but we are seeing the issue http://drupal.org/node/742006 which is supposedly a duplicate of this issue, on MySQL which runs on a new Ubuntu Lucid Beta install. So presumably the new MySQL version that is used in drupal brings the SQL in line with Postgres?

Is there a timeframe on fixing this? It seems to me to be quite a critical bug, if people see an error on almost every page...

I commit to testing a fix to this problem. I currently have a production system still on 6.13. Prior to upgrading it, I installed 6.16 on my development box and began getting this error. I won't be upgrading PRD until it's resolved, so I'd be happy to help.

My postgresql is 8.3.7 on both boxes.

The module creating the issue for me is Taxonomy Access Control 6.x-1.2.

Thanks...

I think that this patch is still needed, even after #681760: Try to improve performance and eliminate duplicates caused by node_access table joins lands. Avoiding 'distinct' in core is good for performance, but it must still work if used in some other module.

Currently, the core blogs module is broken under postgres due to this issue. I'll test under d16 if a patch is submitted. My workaround was to roll back to an earlier version of database.inc and database.pgsql.inc, which is working well enough for now, but is not a good long-term solution.

Regarding the patch in #1, I would recommend that the implementation of db_distinct_allowed should be placed in database.pgsql.inc and database.mysql.inc so that it does not need to do special checking on the global $db_type.

Marked #846080: PostgreSQL query error as dupe

subscribe

Subscribe

Just to confirm, this patch is still needed for taxonomy with Drupal 6.20
Had to re-apply after upgrading Drupal core.