Nodes (e.g. blogs) are sorted incorrectly.
Either 'sticky' and creation date are ignored. Nodes are sorted in nid order.

I think the problem exists in db_distinct_field in database.pgsql.inc:
$query = preg_replace('/(ORDER BY )(?!'.$table.'\.'.$field.')/', '\1'."$table.$field, ", $query);

Given query (e.g. "SELECT .... ORDER BY n.sticky DESC, n.created DESC") is rewrited as:
SELECT DISTINCT ON (n.nid) .... ORDER BY n.nid, n.sticky DESC, n.created DESC

Postgres requires DESTINCT ON expressions must match initial ORDER BY expressions,
but we don't expect "ORDER BY n.nid, ....".

OS: Debian GNU/Linux (stable/sarge)
Drupal: 5.1
Postgres: 7.4.7-6sarge4
PHP: 4.3.10-4
Apache: 2.0.54-5sarge1

CommentFileSizeAuthor
#70 database.pgsql_.inc_.patch1.29 KBShiny
#69 database.pgsql_.inc_.patch1.27 KBsirprize
#60 database.pgsql_.inc_.128846.59.patch1.52 KBsalvis
#47 database.pgsql_.inc_.128846.47.patch1.42 KBsalvis
#27 128846.patch1.82 KBwedge
#23 db_distinct_3.patch7.12 KBbdragon
#21 db_distinct_2.patch7.14 KBbdragon
#15 db_distinct.patch6.3 KBchx
#7 20070330-drupal-database.patch5.11 KBtakashi
#4 20070323-drupal-bug-report.patch5.1 KBtakashi
#2 Screenshot_21.png174.6 KBtakashi

Comments

Shiny’s picture

Shiny commented

i've tried to replicate this on ubuntu + postgresql 7.4 - but can't

on which page are they not sorting correctly?

takashi’s picture

takashi commented
StatusFileSize
new Screenshot_21.png174.6 KB

I've tested '/blog/' page: modules/blog/blog.module:blog_page_last().

I've add DEBUG code into blog_page_last() as follows:

| function blog_page_last() {
| global $user;
| $output = '';
| $result = pager_query(db_rewrite_sql("SELECT n.nid, n.created FROM {node} n WHERE n.type = 'blog' AND n.status = 1 ORDER BY
| n.sticky DESC, n.created DESC"), variable_get('default_nodes_main', 10));
| while ($node = db_fetch_object($result)) {
+| /*DEBUG*/$output .= 'nid[' . $node->nid . ']';
| $output .= node_view(node_load($node->nid), 1);
| }

As you see in screenshot (attached file), blog entries are in nid order, not in creation date order.

takashi’s picture

takashi commented

I've found how to reproduce the bug:

* Install and enable 'tac_lite' module
* Push 'rebuild permissions' button on '/admin/content/node-settings' screen.

tac_lite module provides 'node_grants' .
If 'rebuild permissions' button has been pushed, node_access_rebuild () is called from node_configure_rebuild_confirm_submit().

node_access_rebuild () deletes all records from node_access,
and do not insert default record (nid=0, gid=0, realm='all', view=0, update=0, delete=0) if any module
provide 'node_grants'.

If there are no record with nid=0 in the node_access table, node_access_view_all_nodes() returns 0,
then node_db_rewrite_sql() returns 'distinct' => 1. _db_rewrite_sql() also returns 'distinct' => 1.

If so, db_rewrite_sql () calls db_distinct_field (), but as I reported, db_distinct_field() in database.psql.inc has
undesirable side effect on blog listing.

takashi’s picture

takashi commented
StatusFileSize
new 20070323-drupal-bug-report.patch5.1 KB

IMHO, there is a bug in db_distinct_field() (both database.mysql.inc
and database.pgsql.inc).

MySQL (database.mysql.inc)

(1) db_distinct_field ("n", "nid",
"SELECT n.nid, n.sticky, n.created
FROM {node} n
ORDER BY n.sticky DESC, n.created DESC")
=> SELECT DISTINCT(n.nid), n.sticky, n.created
FROM {node} n
ORDER BY n.sticky DESC, n.created DESC

Rewritten SQL is equivalent to

SELECT DISTINCT n.nid, n.sticky, n.cread FROM ....

and n.nid could be duplicated (see [TEST-M-3]), may be this
isn't what we want.

(2) db_distinct_field ("n", "nid",
"SELECT n.sticky, n.nid, n.created
FROM {node} n
ORDER BY n.sticky DESC, n.created DESC")
=> SELECT n.sticky, DISTINCT(n.nid), n.created
FROM {node} n ORDER BY n.sticky DESC, n.created DESC

Rewritten SQL is syntax error (see [TEST-M-4]).

PostgreSQL (database.pgsql.inc)

(1) db_distinct_field ("n", "nid",
"SELECT n.nid, n.sticky, n.created
FROM {node} n
ORDER BY n.sticky DESC, n.created DESC")
=> SELECT DISTINCT ON (n.nid) n.nid, n.sticky, n.created
FROM {node} n ORDER BY n.nid, sticky DESC, created DESC

n.nid is added to ORDER BY in rewritten SQL; but with SELECT
DISTINCT ON (n.nid), n.nid becomes unique, so that ORDER BY
n.nid is dominant, succeeding sticky DESC, created DESC had no
meaning on ORDER BY.

(2) db_distinct_field ("n", "nid",
"SELECT n.sticky, n.nid, n.created
FROM {node} n
ORDER BY n.sticky DESC, n.created DESC")
=> SELECT n.sticky, DISTINCT ON (n.nid) n.nid, n.created
FROM {node} n ORDER BY n.nid, sticky DESC, created DESC

Rewritten SQL is syntax error.

A simple test:

Create a database for testing, create table named 'node' with only 3
columns (nid: integer, sticky: integer, created: integer), and insert
the following data.

nid | sticky | created
-----+--------+---------
1 | 0 | 1005
2 | 1 | 1004
3 | 0 | 1001
4 | 0 | 1010
4 | 0 | 1010
6 | 0 | 1020
6 | 1 | 1030

MySQL 5.0.32
============

$ mysql -p testing
Enter password:
....
Server version: 5.0.32-Debian_7etch1-log Debian etch distribution
....

[TEST-M-1] Select all records, we got 7 records.

mysql> SELECT nid, sticky, created FROM node;
+------+--------+---------+
| nid | sticky | created |
+------+--------+---------+
| 1 | 0 | 1005 |
| 2 | 1 | 1004 |
| 3 | 0 | 1001 |
| 4 | 0 | 1010 |
| 4 | 0 | 1010 |
| 6 | 0 | 1020 |
| 6 | 1 | 1030 |
+------+--------+---------+
7 rows in set (0.00 sec)

[TEST-M-2] With "SELECT DISTINCT nid, sticky, ...", duplicated record
(nid = 4, sticky = 0, created = 1010) is removed, and we got 6
records.

mysql> SELECT DISTINCT nid, sticky, created FROM node;
+------+--------+---------+
| nid | sticky | created |
+------+--------+---------+
| 1 | 0 | 1005 |
| 2 | 1 | 1004 |
| 3 | 0 | 1001 |
| 4 | 0 | 1010 |
| 6 | 0 | 1020 |
| 6 | 1 | 1030 |
+------+--------+---------+
6 rows in set (0.00 sec)

[TEST-M-3] With "SELECT DISTINCT (nid), sticky, ...", also we got 6
records, duplicated nid (nid = 6) is not removed. This means that
"SELECT DISTINCT (nid), sticky, ..." is equivalent to "SELECT DISTINCT
nid, sticky, ....":

"SELECT DISTINCT (nid), sticky, ..."
== "SELECT" + "DISTINCT" + "(nid), sticky, ...."
== "SELECT" + "DISTINCT" + "nid, sticky, ...."

mysql> select distinct (nid), sticky, created from node;
+------+--------+---------+
| nid | sticky | created |
+------+--------+---------+
| 1 | 0 | 1005 |
| 2 | 1 | 1004 |
| 3 | 0 | 1001 |
| 4 | 0 | 1010 |
| 6 | 0 | 1020 |
| 6 | 1 | 1030 |
+------+--------+---------+
6 rows in set (0.00 sec)

[TEST-M-4] "SELECT nid, DISTINCT (sticky), ...." is a wrong syntax in
MySQL.

mysql> SELECT nid, DISTINCT (sticky), created FROM node;
ERROR 1064 (42000): You have an error in your SQL syntax; ....
near 'distinct (sticky), created from node' at line 1

[TEST-M-5] "SELECT DISTINCT (nid, sticky), ..." is a wrong syntax in
MySQL.

mysql> SELECT DISTINCT (nid, sticky), created FROM node;
ERROR 1241 (21000): Operand should contain 1 column(s)

[Solution] use sub query with GROUP BY, duplicated nid record is
removed, and we got 5 records.

mysql> SELECT n.nid, n.sticky, n.created
FROM (SELECT * FROM node GROUP BY nid) n;
+------+--------+---------+
| nid | sticky | created |
+------+--------+---------+
| 1 | 0 | 1005 |
| 2 | 1 | 1004 |
| 3 | 0 | 1001 |
| 4 | 0 | 1010 |
| 6 | 0 | 1020 |
+------+--------+---------+
5 rows in set (0.01 sec)

Every record has unique value in nid, so that DISTINCT has no effect.

mysql> SELECT DISTINCT n.nid, n.sticky, n.created
FROM (SELECT * FROM node GROUP BY nid) n;
+------+--------+---------+
| nid | sticky | created |
+------+--------+---------+
| 1 | 0 | 1005 |
| 2 | 1 | 1004 |
| 3 | 0 | 1001 |
| 4 | 0 | 1010 |
| 6 | 0 | 1020 |
+------+--------+---------+
5 rows in set (0.00 sec)

PostgreSQL 8.1.8
================

$ psql
Welcome to psql 8.1.8, the PostgreSQL interactive terminal.
....

[TEST-P-1] Select all records, we got 7 records.

=> SELECT nid, sticky, created FROM node;
nid | sticky | created
-----+--------+---------
1 | 0 | 1005
2 | 1 | 1004
3 | 0 | 1001
4 | 0 | 1010
4 | 0 | 1010
6 | 0 | 1020
6 | 1 | 1030
(7 rows)

[TEST-P-2] With "SELECT DISTINCT nid, sticky, ...", duplicated record
(nid = 4, sticky = 0, created = 1010) is removed, we got 6 records.

=> SELECT DISTINCT nid, sticky, created FROM node;
nid | sticky | created
-----+--------+---------
1 | 0 | 1005
2 | 1 | 1004
3 | 0 | 1001
4 | 0 | 1010
6 | 0 | 1020
6 | 1 | 1030
(6 rows)

[TEST-P-3] With "SELECT DISTINCT (nid), sticky, ...", we got 6
records, and also this is not a syntax error in PostgreSQL.

=> SELECT DISTINCT (nid), sticky, created FROM node;
nid | sticky | created
-----+--------+---------
1 | 0 | 1005
2 | 1 | 1004
3 | 0 | 1001
4 | 0 | 1010
6 | 0 | 1020
6 | 1 | 1030
(6 rows)

[TEST-P-4] "SELECT nid, DISTINCT (sticky), ..." is a wrong syntax in
PostgreSQL.

=> SELECT nid, DISTINCT (sticky), created FROM node;
ERROR: syntax error at or near "distinct" at character 13
LINE 1: select nid, distinct (sticky), created from node;
^

[TEST-P-4] "SELECT DISTINCT (nid, sticky), ..." is a wrong syntax in
PostgreSQL.

=> select distinct (nid, sticky), created from node;
ERROR: could not identify an ordering operator for type record
HINT: Use an explicit ordering operator or modify the query.

[Solution] use sub query with DISTINCT ON, duplicated nid record is
removed,and we got 5 records.

=> SELECT n.nid, n.sticky, n.created
FROM (SELECT DISTINCT ON (nid) * FROM node) n;
nid | sticky | created
-----+--------+---------
1 | 0 | 1005
2 | 1 | 1004
3 | 0 | 1001
4 | 0 | 1010
6 | 0 | 1020
(5 rows)

Patch for database.mysql.inc:
/**
* Wraps the given table.field entry with a DISTINCT(). The wrapper is added to
* the SELECT list entry of the given query and the resulting query is returned.
* This function only applies the wrapper if a DISTINCT doesn't already exist in
* the query.
*
* @param $table Table containing the field to set as DISTINCT
* @param $field Field to set as DISTINCT
* @param $query Query to apply the wrapper to
* @return SQL query with the DISTINCT wrapper surrounding the given table.field.
*/
function db_distinct_field($table, $field, $query) {
/*
* Rewrite rule: (t: $table, f: $field)
*
* (1) SELECT .... FROM .... {some}_table t ....
* => SELECT .... FROM .... (SELECT * FROM {some}_table GROUP BY f) t ....
*
* (2) Remove DISTINCT or DISTINCTROW
* SELECT DISTINCT .... FROM ....
* => SELECT .... FROM ....
*
* (3) Remove pgsql specific syntax SELECT DISTINCT ON (...)
* SELECT .... DISTINCT ON (....) ....
* => SELECT .... ....
*/

$query = preg_replace('/\s+DISTINCT(ROW)?(\s+ON\s*\([^\)]*\))?/si', "", $query);
if (preg_match('/(.*FROM\s+)(.*?)(\s+(WHERE|GROUP|HAVING|ORDER|LIMIT|FOR).*)/Asi', $query, $m)) {
$query = $m[1] . preg_replace('/([\{\w+\}]+)\s+(' . $table . ')\b/si',
'(SELECT * FROM \1 GROUP BY ' . $field . ') \2', $m[2]) . $m[3];
}
return $query;
}

Patch for database.pgsql.inc:
/**
* Wraps the given table.field entry with a DISTINCT(). The wrapper is added to
* the SELECT list entry of the given query and the resulting query is returned.
* This function only applies the wrapper if a DISTINCT doesn't already exist in
* the query.
*
* @param $table Table containing the field to set as DISTINCT
* @param $field Field to set as DISTINCT
* @param $query Query to apply the wrapper to
* @return SQL query with the DISTINCT wrapper surrounding the given table.field.
*/
function db_distinct_field($table, $field, $query) {
/*
* Rewrite rule: (t: $table, f: $field)
*
* (1) SELECT .... FROM .... {some}_table t ....
* => SELECT .... FROM .... (SELECT DISTINCT ON (f) * FROM {some}_table f) t ....
*
* (2) Don't rewrite SELECT DISTINCT ON (t.n)
* SELECT DISTINCT ON (t.n) .... FROM .... {some}_table t ....
* => SELECT DISTINCT ON (t.n) .... FROM .... {some}_table t ....
*
* SELECT DISTINCT ON (n) .... FROM .... {some}_table t ....
* => SELECT DISTINCT ON (t.n) .... FROM .... {some}_table t ....
*
* (3) SELECT DISTINCT ON (...., t.n, ....) .... FROM .... {some}_table t ....
* => SELECT DISTINCT ON (...., t.n, ....) .... FROM .... (SELECT DISTINCT ON (f) * FROM {some}_table) t ....
*/

if (!preg_match('/DISTINCT\s+ON\s*\(\s*('. $table .'\s*\.\s*)?'. $field .'\s*\)/si', $query)
&& preg_match('/(.*FROM\s+)(.*?)(\s+(WHERE|GROUP|HAVING|ORDER|LIMIT|FOR).*)/Asi', $query, $m)) {
$query = $m[1] . preg_replace('/([\{\w+\}]+)\s+(' . $table . ')\b/Usi',
'(SELECT DISTINCT ON (' . $field . ') * FROM \1) \2', $m[2]) . $m[3];
}
return $query;
}

Result:

With the above path,

(1) db_distinct_field ("n", "nid",
"SELECT n.nid, n.sticky, n.created
FROM {node} n
ORDER BY n.sticky DESC, n.created DESC")

MYSQL => SELECT n.nid, n.sticky, n.created
FROM (SELECT * FROM {node} GROUP BY nid) n
ORDER BY sticky DESC, created DESC]
PGSQL => SELECT n.nid, n.sticky, n.created
FROM (SELECT DISTINCT ON (nid) * FROM {node}) n
ORDER BY sticky DESC, created DESC]

(2) db_distinct_field ("n", "nid",
"SELECT n.sticky, n.nid, n.created
FROM {node} n
ORDER BY sticky DESC, created DESC")

MYSQL => SELECT n.sticky, n.nid, n.created
FROM (SELECT * FROM {node} GROUP BY nid) n
ORDER BY sticky DESC, created DESC]
PGSQL => SELECT n.sticky, n.nid, n.created
FROM (SELECT DISTINCT ON (nid) * FROM {node}) n
ORDER BY sticky DESC, created DESC]

wedge’s picture

wedge commented

takashi's patch worked for me. PGSQL 8.2.3, Drupal 5.1 and using the taxonomy_access module. The nodes are sorted as expected and no more SQL errors.

wedge’s picture

wedge commented

The frontpage worked ok with this patch but other pages do not. For instance my forum and image_gallery doesn't work.
Here this query
SELECT r.tid, COUNT(n.nid) AS topic_count, SUM(l.comment_count) AS comment_count FROM {node} n INNER JOIN {node_comment_statistics} l ON n.nid = l.nid INNER JOIN {term_node} r ON n.nid = r.nid WHERE n.status = 1 AND n.type = 'forum' GROUP BY r.tid
is rewritten to
SELECT r.tid, COUNT(n.nid) AS topic_count, SUM(l.comment_count) AS comment_count FROM (SELECT DISTINCT ON (nid) * FROM {node}) n INNER JOIN {node_comment_statistics} l (SELECT DISTINCT ON (nid) * FROM ON) n.nid = l.nid INNER JOIN {term_node} r (SELECT DISTINCT ON (nid) * FROM ON) n.nid = r.nid WHERE n.status = 1 AND n.type = 'forum' GROUP BY r.tid
Which fails: ERROR: syntax error at or near "SELECT" Character: 166.

takashi’s picture

takashi commented
StatusFileSize
new 20070330-drupal-database.patch5.11 KB

As wedge pointed out, the last patch I've posted wasn't complete.
Substring 'ON n' of '.... INNER JOIN .... ON n.nid' is recognized in error as 'ON AS n',
i.e. ON: table-reference; n: alias.

I've modified the patch (attached).

wedge’s picture

wedge commented

The new patch fixed the problem I had (only tested on pgsql). Thanks!

takashi’s picture

takashi commented
Status: Active » Needs review

Update status.

Shiny’s picture

Shiny commented

i've managed to replicate this - completely accidently, after installing node_access modules.

This patch fixed this.
http://drupal.org/node/128846

i haven't seen any side effects - looks good to me.

PMunn’s picture

PMunn commented

My PostgreSQL error problem here is fixed by this patch, so I'm willing to throw my support behind this patch.

Things that still aren't sorting properly for me:
- block listing the forum posts.
- the forum

These are still sorting on newest NID to oldest.

Anonymous’s picture

Anonymous (not verified) commented

I apparently solved this same issue via another bug report

see http://drupal.org/node/115117

for an alternative patch

Anonymous’s picture

Anonymous (not verified) commented

After further review, I recommend takashi's patch over my own

plj’s picture

plj commented

This patch has also fixed those problems with views.module I've described in bug #146908, so I propose accepting this patch.

chx’s picture

chx commented
Version: 5.1 » 6.x-dev
StatusFileSize
new db_distinct.patch6.3 KB

Very crafty solution. However unnamed views are MySQL 4.1 features so I am unsure about 5.1 but six is fine.

PMunn’s picture

PMunn commented

But but but! But some of us want PostgreSQL fixes so we're not living with an unofficially patched core system until the distant future when 6.x appears.

pearcec’s picture

pearcec commented

Postgresql patch worked for me.

stevenpatz’s picture

stevenpatz
he/him
commented
Version: 6.x-dev » 7.x-dev

Too late for 6.x?

pearcec’s picture

pearcec commented

7? When is 6 coming out. I am developing on 5 and having even released yet. Either way this is a very critical patch IMO. Should get checked into what ever release is coming out next.

Crell’s picture

Crell commented
Version: 7.x-dev » 6.x-dev

Bug fixes are unaffected by the freeze, and are welcome at pretty much any time.

Note that Drupal 6 will require MySQL 4.1 anyway, so a fix that depends on MySQL 4.1 features is perfectly acceptable.

bdragon’s picture

bdragon commented
StatusFileSize
new db_distinct_2.patch7.14 KB

Unbreak patch by rerolling without a coding style fix that already made it in.

bdragon’s picture

bdragon commented
Status: Needs review » Needs work

patching file includes/database.mysql-common.inc
Hunk #2 FAILED at 527.
1 out of 2 hunks FAILED -- saving rejects to file includes/database.mysql-common.inc.rej
patching file includes/database.mysql.inc
patching file includes/database.mysqli.inc
patching file includes/database.pgsql.inc

bdragon’s picture

bdragon commented
Status: Needs work » Needs review
StatusFileSize
new db_distinct_3.patch7.12 KB

End of file whitespace breakage. No problemo.

davidwhthomas’s picture

davidwhthomas commented
Version: 6.x-dev » 5.1

Thanks, I can confirm the latest patch above works for me on
Drupal 5.1
Postgres 8.2
Debian 4.0 (Etch)

I had this error when creating a table style View with pagination. Node sorting was odd too.

It's all working now, thanks again.

DT

drumm’s picture

drumm
he/him
Location NY, US
commented
Version: 5.1 » 6.x-dev

This patch does not even apply cleanly on Drupal 5.x. Lets get this fixed for 6.x first.

pedro7’s picture

pedro7 commented
Version: 6.x-dev » 5.2
Status: Needs review » Needs work

bdragon, your patch works fine, thank you! I'm running Drupal 5.2, PostgreSQL 8.2.3 and PHP 4.4.7. It works fine until i don't enable any access control module (nodeaccess or content_access with ACL module). In this case, nodes are displayed (or counted) multiple times. For example, i have 4 roles:

Anonymous user
Authenticated user
Student
Teacher

when view access to node 41 is allowed for authenticated users, for students and for teachers and there is logged on user with role Authenticated user, everything looks OK. But when the user has (of course) role Authenticated user AND role Student, node is displayed twice. When i assign to the test user also role Teacher, node is displayed three times on the front page. I have block taxonomy_dhtml and here nodes are also counted multiple times. The front page looks like:

Node41

Node41

Node41

Node40

Node40

Node39

etc.

It depent on assigned roles and access rights.

I think, some SQL queries are not rewritten correctly, missing DISTINCT (nid) or something similar. I dont know how to find the SQL query in Drupal where nodes are selected for the front page or where are counted for the taxonomy_dhtml module. It's in node.module ? Or where ?

Thank you for help!

wedge’s picture

wedge commented
Version: 5.2 » 6.0-beta4
StatusFileSize
new 128846.patch1.82 KB

This patch is still needed to make the node sort order correct on my postgresql system. Rerolled patch for 6b4 attached.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Hm, looking at this patch, I am not sure core on contrib uses DISTINCT ON, but simply DISTINCT instead.

SELECT DISTINCT translation, language FROM {locales_target} ...
SELECT DISTINCT(menu_name) FROM {menu_links} ...
SELECT DISTINCT(bid) FROM {book}...
SELECT DISTINCT(type) FROM {watchdog}...
SELECT COUNT(DISTINCT nid) FROM {node}...
SELECT COUNT(DISTINCT(path)) FROM {accesslog}...
SELECT DISTINCT f.* FROM {upload}...

These are some of the different uses I identified from my grep.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Version: 6.0-beta4 » 6.x-dev
gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Without interest in this bug being fixed, there is no point in holding up the 6.x release with this. As it stands currently, this is one of the few issues holding up Drupal 6.x. Any interest?

zig007’s picture

zig007 commented
Version: 6.x-dev » 5.5

Hi,
None of these patches works for me. I am running 5.5..
Aren't these patches merged into the main branch? I mean, this affect almost everything in my setup..

Gábor Hojtsy- I'd say that this bug is a bit too critical to just ignore, I get variants of this error from all over the site...Maybe this only happens to postgresql users like myself, but it is still critical and a showstopper for me.

Some examples:

Listing organic groups:

* warning: pg_query() [function.pg-query]: Query failed: ERROR: syntax error at or near "ON" at character 24 in /usr/share/drupal5/includes/database.pgsql.inc on line 125.
* user warning: query: SELECT count( DISTINCT ON (node.nid) node.nid) FROM node node LEFT JOIN og og ON node.nid = og.nid INNER JOIN users users ON node.uid = users.uid WHERE (node.status = '1') AND (og.directory = '1') AND (node.type IN ('groups')) in /usr/share/drupal5/includes/database.pgsql.inc on line 144.

Recent posts(tracker):

* warning: pg_query() [function.pg-query]: Query failed: ERROR: syntax error at or near "ON" at character 24 in /usr/share/drupal5/includes/database.pgsql.inc on line 125.
* user warning: query: SELECT count( DISTINCT ON (node.nid) node.nid) FROM node node LEFT JOIN node_comment_statistics node_comment_statistics ON node.nid = node_comment_statistics.nid INNER JOIN users users ON node.uid = users.uid WHERE (node.status = '1') AND (node.uid = '3') in /usr/share/drupal5/includes/database.pgsql.inc on line 144.

zig007’s picture

zig007 commented

Hm, i am a bit confused, i just fetched the latest and greatest from CVS, and there, this problem was nowhere to be seen. Have someone fixed it?

Gábor Hojtsy- I don't understand....if you meant that these bugs were only present in the 5.x versions, why would that hold up the 6.x versions release?

bfo’s picture

bfo commented

I'm in the same situation.

I'm using Drupal 5.5 with Postgres 8.1.4 and PHP 5.1.6

I really would like some access control in the forums, however each of the modules which do this breaks the views and the patches I've tried so far break other bits.

If it gets fixed for 6 I could just about live with that but please don't put it off any longer.

bfo

zig007’s picture

zig007 commented

bfo-

I manually edited the db_distinct_field-function in /include/database.pgsql.inc to look like it does in wedge's patch above(#27):

function db_distinct_field($table, $field, $query) {
  if (!preg_match('/DISTINCT\s+ON\s*\(\s*('. $table .'\s*\.\s*)?'. $field .'\s*\)/si', $query)
      && preg_match('/(.*FROM\s+)(.*?\s)(\s*(WHERE|GROUP|HAVING|ORDER|LIMIT|FOR).*)/Asi', $query, $m)) {
    $query = $m[1];
    $query .= preg_replace('/([\{\w+\}]+)\s+(' . $table . ')\s/Usi', '(SELECT DISTINCT ON (' . $field . ') * FROM \1) \2 ', $m[2]);
    $query .= $m[3];
  }
  return $query;
}

It actually seem to work(the patch above was for a different version).
On the other hand, I am no longer that sure about the 6.x code, since:
1. I didn't have any modules active there when I tested.
2. When I look a bit closer at the 6.x code it looks awfully unpatched.

I haven't tested it much yet, but so far, at least there are no errors.
Obvously a patch would be better, but I am not that into that stuff yet.

zig007’s picture

zig007 commented

Ok, I just found one weird thing that might have something to do with the fix, in some lists like organic groups and My network, there are two of each object.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

zig007: last time wedge moved this to the 6.x queue. I can only trust you and understand if this does not work in Drupal 6. If it works there however, then I am all happy.

zig007’s picture

zig007 commented

Gábor Hojtsy -
Actually, the problem is in 5.5.

nospam2’s picture

nospam2 commented

The Takashi patch broke Case Tracker's dash board on version 5.5 with Postgresql. Here is the fixed version I made:

function db_distinct_field($table, $field, $query) {
    if (!preg_match('/FROM\s+\S+\s+AS/si', $query)
      && !preg_match('/DISTINCT\s+ON\s*\(\s*('. $table .'\s*\.\s*)?'. $field .'\s*\)/si', $query)
      && preg_match('/(.*FROM\s+)(.*?\s)(\s*(WHERE|GROUP|HAVING|ORDER|LIMIT|FOR).*)/Asi', $query, $m)) {
        $query = $m[1];
        $query .= preg_replace('/([\{\w+\}]+)\s+(' . $table . ')\s/Usi', '(SELECT DISTINCT ON (' . $field . ') * FROM \1) \2 ', $m[2]);
        $query .= $m[3];
    }

    return $query;
}

I added a condition to ignore any lines with "FROM [table] AS". Without it, this function produces some messed queries like this:

SELECT n.nid, c.csid, Count(1) AS number_of_cases FROM node (SELECT DISTINCT ON (nid) * FROM AS) n

EDIT: Fixed the code format. <PRE> tag didn't work.

zig007’s picture

zig007 commented

nospam2 - That seems to work for me. Thanks!

bfo’s picture

bfo commented

is a fix for this still planned for the drupal 6 core code?

This is quite an important factor for me as I'm about to create a site for a youth group (Guides) and currently although i can hide the comments the first post in a forum shows and is readable to anonymous users, this would be no use for a site where kids with be posting.

Thanks

robin t’s picture

robin t commented

Patch also fixed forum displays for me. Without it, no numbers was shown in /forum and nothing was shown in /forum/xx.

Oh, and the log was filled with stuff like:

query: SELECT COUNT( DISTINCT ON (n.nid) n.nid) FROM node n INNER JOIN term_node tn ON n.nid = tn.nid AND tn.tid = 18 LEFT JOIN history h ON n.nid = h.nid AND h.uid = 1 WHERE n.status = 1 AND n.type = &#039;forum&#039; AND n.created &gt; 1197288468 AND h.nid IS NULL in includes/database.pgsql.inc on line 144

This needs to go into 5.x

klance’s picture

klance commented

I used the patch in #38 and it worked for me with 5.6. Thanks!

bfo’s picture

bfo commented

I used atch #38 also, but I'm getting duplicate copies of the articles on the front page when one of my user roles logs in (the user only has 1 role set), has anyone sloved this yet?

bfo’s picture

bfo commented

Actually I've fixed this.

I'm using the Taxonomy Access Control Module and if you set authenticated user to be allowed to view a taxonomy then you need to set the roles ignore access or it will show up twice. This may be documented somewhere but I missed it

sammys’s picture

sammys commented
Component: database system » postgresql database

Bumping onto my issue list. Just arrived back from overseas and will review over the weekend.

--
Sammy Spets
Synerger
http://synerger.com

stella’s picture

stella commented

FYI: This issue is also reported in #190503. A patch (which may or may not work) is proposed there too.

salvis’s picture

salvis
he/his
commented
Title: Nodes are sorted incorrectly with PostgreSQL » Rewritten PostgreSQL queries fail
Version: 5.5 » 5.x-dev
Status: Needs work » Needs review
StatusFileSize
new database.pgsql_.inc_.128846.47.patch1.42 KB

I'm confused by the long history of this issue, but I've just had a Forum Access user who reported success after applying the code in #38 (http://drupal.org/node/223835), so I'm posting it here in the proper patch format, hoping this will get the issue moving.

I don't have access to PostgreSQL myself and this needs serious reviewing! The comment in the old code

  // ... (no need to rewrite queries that already use DISTINCT).

makes me suspect that the patch may neglect this aspect, but the old code is definitely buggy.

Please fix this!

nospam2’s picture

nospam2 commented

Thanks for making the patch for the patch I posted. The patch does have the code to skip queries with "DISTINCT ON". This is the code:

!preg_match('/DISTINCT\s+ON\s*\(\s*('. $table .'\s*\.\s*)?'. $field .'\s*\)/si'

But it does not skip queries with just "DISTINCT", but without "ON". Fortunately, Drupal coders write in mysql dialect. They always use "ON" with "DISTINCT". So I didn't bother to add another section that will never get executed. If I ever see a module with "DISTINCT", but without "ON", I will add that section. It works now, I just don't want to mess with it. :)

salvis’s picture

salvis
he/his
commented

Seeing how long it takes to get a necessary patch committed, and how difficult it is to diagnose this problem, I'm not sure that's the best approach...

@drumm: what can we do to get this moving?

drumm’s picture

drumm
he/him
Location NY, US
commented
Version: 5.x-dev » 6.x-dev

salvis, no one has tested your patch. This can't get committed until it is well-tested.

Since Drupal 6 is out, this should be made against Drupal 6 and then backported. More people will see and test Drupal 6 patches.

salvis’s picture

salvis
he/his
commented

Ok, the patch applies equally to D6 and to D5.

bfo’s picture

bfo commented

I have been running with this patch on 3 sites since January 22nd

I have 2 live sites using it and 1 demo, the demo site has had it running the longest.

All sites are using Drupal 5.6, PHP 5.1.6 and postgres 8.1.4

I have the following modules running on my test site

admin_message/
adminblock/
authorship/
block/
captcha/
color/
comment/
contact/
contact_forms/
drupal/
event/
filter/
forum/
help/
image/
img_assist/
inline/
links/
linksdb/
mail_edit/
menu/
node/
notify/
path/
poll/
profile/
search/
smileys/
statistics/
statistics_filter/
subscriptions/
system/
taxonomy/
taxonomy_access/
upload/
user/
watchdog/
webform/

The test site was tested since january with the patch with 25 testers accessing all parts of the site before I put it live.

What extra testing is required for this to make drupal core?

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

My issue at #28 above (http://drupal.org/node/128846#comment-665726) was not solved yet, so this looks like a pretty heavy handed API change to require all DISTINCT queries to use DISTINCT ON, if I am not mistaken.

salvis’s picture

salvis
he/his
commented

@Gábor: I'm not sure I understand what you mean. Right now the query

SELECT r.tid, COUNT(n.nid) AS topic_count, SUM(l.comment_count) AS comment_count FROM {node} n 
INNER JOIN {node_comment_statistics} l ON n.nid = l.nid 
INNER JOIN {term_node} r ON n.nid = r.nid 
WHERE n.status = 1 AND n.type = 'forum' GROUP BY r.tid

in forum_get_forums() in forum.module gets rewritten to

SELECT r.tid, COUNT( DISTINCT ON (n.nid) n.nid) AS topic_count, SUM(l.comment_count) AS comment_count FROM node n
INNER JOIN node_comment_statistics l ON n.nid = l.nid 
INNER JOIN term_node r ON n.nid = r.nid 
WHERE n.status = 1 AND n.type = 'forum' GROUP BY r.tid

under PostgreSQL, and this fails. The patch in #38 and #47 fixes it.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

The current code has a negative look-behind for "DISTINCT", the patched code has a negative preg_match() on "DISTINCT ON" (note the added "ON"). Core uses "DISTINCT" in many queries as I pointed out above. How could I explain this better? Do I miss something?

JamieR’s picture

JamieR commented

I can also confirm that this patch works.

For me this issue was also a syntax error for:
SELECT COUNT (DISTINCT ON (n.nid) n.nid)
where it should have been written as:
SELECT COUNT (DISTINCT(n.nid))

The issue appeared when using views.module with tac_lite.module.
I wish I understood the issue more so that I could help.

salvis’s picture

salvis
he/his
commented

@Gábor: So nospam2 should follow through with his promise in #48 and revise his patch, right?

nospam2?

nospam2’s picture

nospam2 commented

If somebody can tell me what modules use "DISTINCT" instead of "DISTINCT ON", I will get the patch fixed. I can't test the fixes without a module to test on.

nospam2’s picture

nospam2 commented

I just saw the code in #28. This updated patch should be able to deal with those queries:

function db_distinct_field($table, $field, $query) {
    if (!preg_match('/FROM\s+\S+\s+AS/si', $query)
      && !preg_match('/DISTINCT\s+ON\s*\(\s*('. $table .'\s*\.\s*)?'. $field .'\s*\)/si', $query)
      && !preg_match('/DISTINCT[ (]'. $field .'/si', $query)
      && preg_match('/(.*FROM\s+)(.*?\s)(\s*(WHERE|GROUP|HAVING|ORDER|LIMIT|FOR).*)/Asi', $query, $m)) {
        $query = $m[1];
        $query .= preg_replace('/([\{\w+\}]+)\s+(' . $table . ')\s/Usi', '(SELECT DISTINCT ON (' . $field . ') * FROM \1) \2 ', $m[2]);
        $query .= $m[3];
    }

    return $query;
}

Gábor, can you please test it and see if it works?

salvis’s picture

salvis
he/his
commented
StatusFileSize
new database.pgsql_.inc_.128846.59.patch1.52 KB

Code in #59 in patch format — please review!

catch’s picture

catch
he/him
Primary language English
commented
Version: 6.x-dev » 7.x-dev

bumping to 7.x, it can be backported once applied there.

bfo’s picture

bfo commented

why is this just being bumped to future versions and no dealt with in the code we have, this has been known about since before 6 was released.

catch’s picture

catch
he/him
Primary language English
commented

bfo - because all bugs get fixed in HEAD then backported to previous versions for consistency. The reason it hasn't been fixed yet is because no-one has yet reviewed the patch in #60, which is because there are very few people running postgresql who test patches.

I've added this to the unofficial list of Core / Contrib Issues That Require a Postgres Review, perhaps that will help.

Crell’s picture

Crell commented
Version: 7.x-dev » 6.x-dev

Since the D7 database layer is being totally rewritten, I am fairly sure this patch and approach won't even apply to D7. Let's just fix it straight in D6.

("Fix in HEAD and backport" is the usual routine, but not if the API has or is in the process of changing to the point that isn't feasible.)

sirprize’s picture

sirprize commented

I have applied the patch in #60 and now the core forum works well on my test D6 installation. Not sure if that already counts as a review though. Will gladly make further tests if you tell me what I could specifically try out :-)

dtougas’s picture

dtougas commented

I have tested this in Drupal 6.1 and have found that the patch in #60 fixes the problem for me too.

sirprize’s picture

sirprize commented
Status: Needs review » Reviewed & tested by the community

I'm taking the liberty to change the patch status in order to gain the attention of someone able to commit this patch. Haven't encountered any problems with that patch for more than 2 weeks.

keith.smith’s picture

keith.smith commented
Status: Reviewed & tested by the community » Needs work

Note that the issue to change the coding standards notwithstanding, this patch does not currently follow conventions regarding spacing around concatenation operators and string literals.

sirprize’s picture

sirprize commented
Status: Needs work » Needs review
StatusFileSize
new database.pgsql_.inc_.patch1.27 KB

I have updated the patch with coding standards. Please review.

Shiny’s picture

Shiny commented
StatusFileSize
new database.pgsql_.inc_.patch1.29 KB

i return to this patch over and over, for many sites.
the .patch no longer applies cleanly on 5.7, so here is is re-rolled.

would appreciate someone from this thread reviewing it on the end of the DRUPAL-5 branch.

jaydub’s picture

jaydub commented

Tested with Drupal 5.7. I was running into the DISTINCT ON problem with
stock og installed and the patch so far has cleared out all the errors.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented

Just applied it to a Drupal 5.7 installation with og, works for me...

SoAnIs’s picture

SoAnIs commented
Version: 6.x-dev » 6.2

AFTER patching with the patch in post #69 of #128846: Rewritten PostgreSQL queries fail and post #93 of #153998: Access checks returning NULL is ugly. I get the following errors when Forum_access and ACL are enabled. No errors with only ACL. Postgres 8.3, Drupal 6.2

    * warning: pg_query() [function.pg-query]: Query failed: ERROR: operator does not exist: character varying = integer LINE 1: ...a ON t.tid = fa.tid LEFT JOIN acl acl ON acl.name = t.tid AN... ^ HINT: No operator matches the given name and argument type(s). You might need to add explicit type casts. in /home/soanis/public_html/includes/database.pgsql.inc on line 138.
    * user warning: query: SELECT t.* FROM term_node r INNER JOIN (SELECT DISTINCT ON (tid) * FROM term_data) t ON r.tid = t.tid INNER JOIN vocabulary v ON t.vid = v.vid LEFT JOIN forum_access fa ON t.tid = fa.tid LEFT JOIN acl acl ON acl.name = t.tid AND acl.module = 'forum_access' LEFT JOIN acl_user aclu ON aclu.acl_id = acl.acl_id AND aclu.uid = 0 WHERE ((fa.grant_view >= 1 AND fa.rid IN (1)) OR fa.tid IS NULL OR aclu.uid = 0) AND ( r.vid = 5 )ORDER BY v.weight, t.weight, t.name in /home/soanis/public_html/modules/taxonomy/taxonomy.module on line 618.
jaydub’s picture

jaydub commented

PostgreSQL 8.3 no longer allows automatic type casting as per this comment:

http://groups.drupal.org/node/9103#comment-28800

The query you have above fails most likely because of the JOIN of
LEFT JOIN acl acl ON acl.name = t.tid because acl.name is a
VARCHAR and t.tid is an INTEGER

You should post to the issue queue for forum_access and/or acl to address this. All
that should be needed here is to CAST the tid to CHAR such as:

LEFT JOIN acl acl ON acl.name = CAST(t.tid AS CHAR)

SoAnIs’s picture

SoAnIs commented

The above fix worked, posted an appropriate bug in forum_access (where the actual problem is.)
Since it's not the ORDER BY in those queries causing the problem, it seems this patch worked fine. After fixing the cast, everything works perfectly. Thus, the patch in #69 works with PostgreSQL 8.3 and Drupal 6.2.

salvis’s picture

salvis
he/his
commented
Version: 6.2 » 6.x-dev

Forum Access / PostgreSQL 8.3 issue fixed in FA 5.x-1.x-dev and 6.x-1.x-dev (#249375: Postgres 8.3 does not support automatic typecasting.) — thanks SoAnIs and jaydub.

Now let's focus back on this issue here!

nospam2’s picture

nospam2 commented

Wow, a lot has happened in the last few days after weeks of inactivity. So is there anything else we need to do to get the code committed?

I am sorry for not following the coding convention. I didn't read the code convention document and I work with java a lot more than PHP. I just passed along the fixes I made, based on Takashi's patch, to give back to this project. I thought the original contributors more involved with the project would be able to write a better fix than mine once we point out where the problems are. But that didn't happen. I don't mind picking up a little slack and do more work on it. But I really am not familiar with the in's and out's of this project, evidently, my lack of success locating the right person to pass the code to. Salvis, are you the project maintainer now?

salvis’s picture

salvis
he/his
commented

@nospam2: I'm the maintainer of Forum Access and ACL, two node access modules that suffer from this issue (like all other node access modules), but the issue is in core.

This is one ugly function, and apparently we have no one smart enough in the area of regexes and pgsql to stand up and say that the patch doesn't cause any new bugs. I don't know how to proceed here...

nospam2’s picture

nospam2 commented

I reviewed the code again and I am quite sure that it will not cause any problems. The regular expression is very specific and it does deal with both "distinct" and "distinct on" now.

salvis’s picture

salvis
he/his
commented

Ok, then change CNR to RTBC...

pedrogk’s picture

pedrogk commented
Version: 6.x-dev » 5.7
Status: Needs review » Reviewed & tested by the community

I am using Drupa 5.7 with Postgres 8.3.1. I was having problems with some queries throwing warnings about use of "select distinct on" ... (see http://drupal.org/node/115117).

Anyway, I applied the patch on comment 70 and everything looks good now.

catch’s picture

catch
he/him
Primary language English
commented
Version: 5.7 » 6.x-dev

Back to 6.x

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Needs review

Was there any MySQL testing done on this patch? I don't see it mentioned here. Do I miss something?

nospam2’s picture

nospam2 commented

I don't think any testing is needed with MySQL. This patch only applies to the core database Postgresql "driver" database.pgsql.inc. It has no effect on MySQL installations.

takashi’s picture

takashi commented
Status: Needs review » Reviewed & tested by the community

Patch (http://drupal.org/node/128846#comment-805250) works fine with my site (http://imotos.net) for more than 1 month.

selenamarie’s picture

selenamarie commented

I'm using this patch, and it works well. found the problem while using organic groups and the og_modr8 module. used the patch from comment #70. fixed it!

Shiny’s picture

Shiny commented

tested patch from #69 against drupal6.2, postgres8.3 works as described.

Shiny’s picture

Shiny commented

The patch from #70 is for drupal5.7 -- what's the right course to get that into drupal cvs branch? sepereate issue thread?

salvis’s picture

salvis
he/his
commented

It needs to go into D6 first, before it'll be considered for D5.

I don't know what it'll take to actually get it in...

This is pgsql only, absolutely no risk for mysql, and the pgsql people unanimously vote in favor of the patch — seems like it should be committable...

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Needs work

Unfortunately this does not apply:

$ patch -p0 < database.pgsql_.inc__1.patch.txt
patching file includes/database.pgsql.inc
Hunk #1 FAILED at 418.
1 out of 1 hunk FAILED -- saving rejects to file includes/database.pgsql.inc.rej

thilen’s picture

thilen commented
Priority: Critical » Normal
Status: Needs work » Reviewed & tested by the community

#69 works perfect for me

drupal 6.2
pgsql 8.1
on etch

But for a User assigned two or more roles all blogs (?q=blog) appear twice ....

thilen’s picture

thilen commented

No not the roles ... and not the patch !

The problem is the modul "Node Privacy By Role"

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Version: 6.x-dev » 5.x-dev

Great, thanks for pointing the version differences out! I've just committed #69 to 6.x This still needs to go into both 7.x and 5.x, so I am moving to 5.x first. The 7.x code base might or might not change with the PDO changes, so that might involve more work there.

Niel: #70 is supposed to be the patch for 5.x.

dries’s picture

dries commented

The patch in #69 does not apply against CVS HEAD.

GeoffGlasson’s picture

GeoffGlasson commented

I have just patched Drupal 5.8 using the patch supplied in #70 and my organic group pages are displaying in the correct order. Previously my organic group summary pages were displaying in reverse order.

System: Windows XP
PostgreSQL: 8.2

I will apply the patch to my production system in a few days and will advise.

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Reviewed & tested by the community » Fixed

Committed to 5.x.

catch’s picture

catch
he/him
Primary language English
commented
Version: 5.x-dev » 7.x-dev
Status: Fixed » Needs work

Whilst this might not need to go into D7, moving there to keep it active until at least PDO has gone in.

pearcec’s picture

pearcec commented
Status: Needs work » Reviewed & tested by the community

For what it is worth that patch

http://drupal.org/files/issues/views_query_pgsql_hack.patch

Fixed my problem. Don't hold out much hope for a views 5.x-1.7 release. Moving this to a patched version in subversion. Good luck.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

PDO is in. db_rewrite_sql is nearly out.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.