Fix syntax of comment ordering subquery

SEcond query should be:

$result = db_query('(SELECT thread FROM {comments} WHERE cid IN (SELECT cid FROM {comments} WHERE nid = %d  AND status = 0 ORDER BY timestamp DESC LIMIT %d) ORDER BY SUBSTRING(thread, 1, (LENGTH(thread) - 1)) LIMIT 1', $node->nid, $new_replies);

Update to critical as all forum running under PostgreSQL should have the same problem.

Could you point us to where this is actually documented?

I don't know where this is documented. Actually the message displays "clauses ORDER BY multiples non autorisées", which means "multiple ORDER BY not authorized".

The query I provided is a traditional nested query. I can be executed on any database. MySQL query analyser should rewrite the query as a traditional nested query with some kind of IN. Therefore there is no degradation using the SQL query I provided.

If MySQL has some kind of SQL parser, try looking how MySQL rewrites the query.

The idea behind this fix is that when you write this MySQLism, MySQL is going to rewrite the query inside the engine. It will surely not execute like that. So it is better to write the correct SQL directly and there is no time execution loss.

@jmpoure: you can really not call that query an "MySQLism", especially if you can't point us to where it is documented that you can't have an ORDER in a FROM-based subquery.

A database should always know how it should run joins.

(SELECT thread FROM comments WHERE nid = 63743 AND status = 0 ORDER BY timestamp DESC LIMIT 136) ORDER BY thread DESC LIMIT 1
is not a standard query. I can be rewritten internaly in MySQL as a subquery or a join.

To know whether this is a standard query, you can access the analyser in the database and read how the query is rewritten.
MySQL will not execute(SELECT thread FROM comments WHERE nid = 63743 AND status = 0 ORDER BY timestamp DESC LIMIT 136) ORDER BY thread DESC LIMIT 1 directlty. The parser will rewrite it and you wron't see it.

So it is better to write a standard query where you see the JOIN or the SUBQUERY. Anyway PostgreSQL is picky and will not accept this SQL clause, probably because it is not standard. PostgreSQL hackers usualy don't accept non-standard queries. It could take them 5 minutes to modify PostgreSQL core, but if it is not standard, they don't accept it. If you know FFmpeg, the same applies for Video and Audio codecs. If it is not standard, even a simple hack, core hackers do not accept something non-compliant with standards. This is not my fault.

Using MySQL is like using Windows 95. You always run into problems that you don't know how to analyse because you don't see what is "inside".

If this is standard SQL, I apologize in advance, but I doubt.

All comments regarding standard or non-standard aside, the fact that the query fails should be of primary concern. I can verify that the query fails in PostgreSQL. I tried the suggested alternative query in the parent issue and while that -does- work in PostgreSQL, it does not work in MySQL 5:

How about this?

That at least didn't throw an error both in MySQL and PostgreSQL.

According to PostgreSQL's documentation,

ORDER BY Clause

The optional ORDER BY clause has this general form:

ORDER BY expression [ ASC | DESC | USING operator ] [ NULLS { FIRST | LAST } ] [, ...]

The ORDER BY clause causes the result rows to be sorted according to the specified expression(s). If two rows are equal according to the leftmost expression, they are compared according to the next expression and so on. If they are equal according to all specified expressions, they are returned in an implementation-dependent order.

... snip ...

Note that ordering options apply only to the expression they follow; for example ORDER BY x, y DESC does not mean the same thing as ORDER BY x DESC, y DESC.

So it appears that multiple ORDER BY statements are legitimate.

Jay,

You should work off the CVS code. The current CVS for this statement is:

Actually, can you test if this CVS code gives the same issues under PostgreSQL?

Sorry, I feel stupid. It added this code to my SVN drupal installation but don't know in which situation I should test it. What kind of comments does it display? Sorry, I feel lost and stupid.

In fact, it seems like this is just a syntax issue:

... is an implicit FROM subquery. The syntax is apparently valid on MySQL, but not on PostgreSQL, so we should write:

No, I don't agree to multiple ORDER BY statements in postgres.
http://www.postgresql.org/docs/8.3/interactive/sql-select.html
talks about The ORDER BY clause (not clauses) and the syntax shows only
one (optional) ORDER BY clause per SELECT statement.:

The optional ORDER BY clause has this general form:
ORDER BY expression [ ASC | DESC | USING operator ] [ NULLS { FIRST | LAST } ] [, ...]

That means: you can have one or more specified expression(s)
(indicated by [, ...]), but only one ORDER BY clause

Instead of:

(SELECT thread FROM comments WHERE nid = 63743 AND status = 0 ORDER BY timestamp DESC LIMIT 136) ORDER BY thread DESC LIMIT 1 ;

it should simply say:
SELECT thread FROM comments WHERE nid = 63743 AND status = 0
ORDER BY timestamp DESC , thread DESC LIMIT 1 ;

@Norbert: see my comment #13, it's nothing more then a syntax issue: MySQL read the query as an implicit sub-query on FROM, which is what we want. Making the subquery explicit should solve the issue.

Instead of:
(SELECT thread FROM comments WHERE nid = 63743 AND status = 0 ORDER BY timestamp DESC LIMIT 136) ORDER BY thread DESC LIMIT 1 ;

it should simply say:
SELECT thread FROM comments WHERE nid = 63743 AND status = 0
ORDER BY timestamp DESC , thread DESC LIMIT 1 ;

No, those are not the same queries (see the difference in the LIMIT clauses).

Bumping to D7 to quick fix that there (and add a test for that...), before backporting.

#237509: PostgreSQL 8.3 error: multiple ORDER BY clauses in comment.module was a duplicate.

Folks!
(i'm author of bugreport and patch #237509 which is duplicate to this bug)

As I can see

1. PosgreSQL does not supports multiple ORDER BY
2. MySQL does not supports LIMIT within subquery
3. comment.module really needs two ORDER BYs (first select N latest comments, than select highest thread id in them)

The only way is to use
if ( $GLOBALS['db_type']=='pgsql')
{
pgsql query
}
else
{ mysql query.

So, patch is trivial, just combine my patch from http://drupal.org/node/462270 to these ugly if-s

@Alex Tutubalin:

MySQL does support LIMIT inside sub-queries (of course... the current code, that is an implicit subquery works). The only thing left to do is to roll a patch according to my suggestion in #13.

@Damien

This is different queries.
First one selects Top N threads by timestamp, than maximum thread ID in these N top-timestamp;
Second one will effectively selects latest (by timestamp).

Again, my solution is
- Left MySQL query as is (assuming that two order-by-s is doing right thing)
- Make special PgSQL query (subselect with Order-by/Limit)
Select between two queries at PHP level by GLOBALS[pg_type]

This should work, althought not beautiful solution.

@Alex_Tutubalin: Please read carefully. This query is what we want and it should work the same on both PostgreSQL and MySQL:

Have you check it on MySQL ?

Also, on PgSQL 8.3 you should use slightly different syntax
SELECT column FROM (SELECT ....) AS column ORDER BY... LIMIT...

SELECT thread FROM (SELECT thread FROM {comments} WHERE nid = %d  AND status = 0 ORDER BY timestamp DESC LIMIT %d) AS last_threads ORDER BY thread DESC LIMIT 1

works perfectly on MySQL.

So, my patch from http://drupal.org/node/462270 should work.

tag for http://geek.joshwaihi.com/content/drupal-code-sprints-postgresql

tagging properly

attaching patch by Alex_Tutubalin, which i am testing at the Postres Code Sprint

The last submitted patch failed testing.

So far i can't replicate this error - i've tried in D6 and D7
Can someone please add the steps to make this error occur? Then i can verify it is fixed by the patch.
(otherwise the patch looks very sane and safe for D6 - needs re-rolling for D7).

#27 was for drupal6

Now patch for D7

But suppose reply for comments are broken in d7

The last submitted patch failed testing.

misbehaving bot

Bot passed all tests

Makes total sense, and the bot is happy.

There have been a lot of back and forth in this issue, so here is some clarification for the core committers.

The issue here is that we have a query that has an invalid syntax:

It is an implicit FROM-based subquery (we first get the threads of the latest comments, then we order them by thread).

MySQL happily deals with this query, even if it is invalid, but PostgreSQL ignore the parenthesis and return a cryptic "Multiple ORDER BY clauses are not allowed).

The patch simply transforms this query into:

I post new issue related to comment ordering #529374: Reply on comment is broken
Maybe this one should be fixed first

The last submitted patch failed testing.

Let's re-test

Looks like bot was broken so rtbc

Committed to HEAD. Thanks!

We need to backport this.

Reroll #27

Thanks. Will review shorly as I am back from holidays. Sorry for the delay.

The patch works as expected. I tested on my D6 forums.
Hope this can be applied ASAP.
It seems straightforward and will not break anything.

Thanks!

Please inform us when it is committed to CVS.

Extra space in the SQL.

@jmpoure: Please read http://drupal.org/node/317.

So here reroll without extra space

And status change... sorry

Thank you Andy and Damien.

In Guidelines for writing MySQL and PostgreSQL compliant SQL
read MySQLism: avoid nested ORDER BY, use nested queries

This describes the SQL-99 standard and how to rewrite nested ORDER BY.

So you can apply the patch safely.
The query will execute very well under PostgreSQL and MySQL.

We hope that the quide will improve Drupal conformance to SQL-99.

Need another review to rtbc this!

looks good

Who tested this on an actual Drupal 6 setup on MySQL? I'm seeing people tested the standalone query which was formed into the patch and people also tested the patch on PostgreSQL, but nobody tested the patch on MySQL, right?

I don't get the same result between the new and old queries. New query also didn't pass the SQL-99 validator (http://developer.mimer.com/validator/parser99/index.tml)