SQL error on PostgreSQL in comment.module

Here's the patch. It makes the SQL error go away in PostgreSQL, and also still passes my contrib module's tests in both MySQL and SQLite (which weren't broken in the original code).

I'm using PostgreSQL 8.4, and here is the error message:

SQLSTATE[42P10]: Invalid column reference: 7 ERROR: for SELECT DISTINCT, ORDER BY expressions must appear in select list LINE 5: ORDER BY SUBSTRING(c.thread, 1, (LENGTH(c.thread) - 1)) ASC ^:

And here is the query:

SELECT DISTINCT c.cid AS cid FROM {comment} c WHERE (c.nid = :db_condition_placeholder_0) AND (c.status = :db_condition_placeholder_1) ORDER BY SUBSTRING(c.thread, 1, (LENGTH(c.thread) - 1)) ASC LIMIT 50 OFFSET 0; Array ( [:db_condition_placeholder_0] => 1 [:db_condition_placeholder_1] => 1 )

Dang, status change again. :)

Um, so I'm not sure that can be fixed in the DB engine. How would it know whether a group by was a field/expression in the select list or a new item?

From IRC - Damien asked where the DISTINCT was happening, because it's not in http://api.drupal.org/api/function/comment_get_thread/7
It's from the node access: http://api.drupal.org/api/function/node_query_node_access_alter/7

Why this query is doing node access is not clear. Seems like a stupid idea to add node access to this query (lots of overhead for a single node? and why is it checking access on this node anyway?).

But that does explain why I didn't see this in my non-simpletest try to reproduce - I didn't have a node access module enabled.

Probably we could make a test that would fail on Postgres based on this:
- Use a node access module (there are tests modules that do this).
- Create a content type with threaded comments (which I think is the default).
- Add a node
- Add a comment and a reply
- View the node

Damien and I agreed on IRC that we shouldn't be tagging this issue for node access, and we think that will get rid of the problem. I will try that out later today.

I don't know if that will fix it, and I think andypost cross-posted...

andypost: I tried your suggestion and got the same SQL error:

SQLSTATE[42P10]: Invalid column reference: 7 ERROR: for SELECT DISTINCT, ORDER BY expressions must appear in select list LINE 5: ORDER BY SUBSTRING(c.thread, 1, (LENGTH(c.thread) - 1)) ASC ^

Here's the query. As you can see, c.thread is now in the SELECT:

SELECT DISTINCT c.cid AS cid, c.thread AS thread FROM {comment} c WHERE (c.nid = :db_condition_placeholder_0) AND (c.status = :db_condition_placeholder_1) ORDER BY SUBSTRING(c.thread, 1, (LENGTH(c.thread) - 1)) ASC LIMIT 50 OFFSET 0; Array ( [:db_condition_placeholder_0] => 1 [:db_condition_placeholder_1] => 1 )

So what I would like to do is suggest #1 be reviewed/committed, since that does fix the problem.

Regarding #7, I am not sure right now about taking the node access out of this query. Why was it put in? I am not sure...

#1: comment-postgre.patch queued for re-testing.

I'm still seeing this error in my tests today, and wondering why the patch in #1 can't be committed?

I guess that regarding #7, I need to make a simple test that will fail on postgres to illustrate this problem. I'll work on that shortly.

Here's a new patch. It includes a test. The test fails on PostgreSQL without the comment.module portion of the patch, and passes with the comment.module portion of the patch. Damien suggested fixing this in the PostgreSQL driver, but didn't propose anything definite as to how to go about doing that.

So in the meantime, this fixes the actual manifestation of the problem that I found. It would be great if we could get this reviewed and in. It's a simple patch, and a simple straightforward test that fails in PostgreSQL... for a bit of an obscure combination of circumstances, but so it goes, at least it's well-specified. :)

Actually, anyone using threaded comments and a node access module of any kind, on PostgreSQL, will not be able to display nodes due to this error. I think that classifies this issue as at least "major", if not "critical".

According to webchick's current definition, this should be critical.

Anyone running a contrib node access module (ANY node access module) and having a node with threaded comments, on PostgreSQL, will not be able to view the node. I think that qualifies.

OK. I've been trying in IRC for *quite* a long time, and have given up. It's such a small patch, shouldnt' be controversial...

Whoops, yes, that line should not be there. Thanks for the review. Here's a new patch without that line, setting to RTBC as per #20.

Right. Apparently, PostgreSQL doesn't let you use a complex expression in the ORDER BY (EDIT: added) with DISTINCT(end of EDIT). It needs to be a field in the field list.