nailing down db_rewrite_sql (mysql 5)

Committed to HEAD. Marking this active because the forum.module problem is still present.

These forum.module queries are unlike any other in drupal. I traced their introduction to this diff [1] and this issue [2]. the user ccourtne engineered these queries for us. he is not accepting contacts via drupal.org. anyone know how to contact him? . I see this comment above one of the queries:

// This query does not use full ANSI syntax since MySQL 3.x does not support
// table1 INNER JOIN table2 INNER JOIN table3 ON table2_criteria ON table3_criteria

In that issue, the patch before the one that was commitrted used more conventional SQL but apparently did not work on Mysql3. So one option here would be to change HEAD accordingly and drop mysql 3 support. I wouldn't mind seeing that happen anyway. But hopefully we don't need to do that. Anyone care to jump in here?

This patch radically changes db_rewrite_sql -- for good. It moves the JOIN just before the first thing after the JOINs. If you come to think of SQL syntax, that is just what we use for WHEREs. Hence the patch.

my comments here may be moot due to chx's patch, but I thought I should share them none the less

First, IMHO I don't believe this is a node access problem. I think this is a db_rewrite_sql() and hook_db_rewrite_sql() problem.

These sql errors may show up on any site that has a module that implements hook_db_rewrite_sql(). When the hook is implemented the developer of the module defines what logic is needed, and if those defined logical parameters are meet, the hook will change queries passed thru the db_rewrite_sql() function.

example #1: see http://drupal.org/node/44291, the logic here is related to the value returned by a call to user_access('xxx'), if FALSE is returned the hook will change the query.
example #2: see http://drupaldocs.org/api/4.6/function/hook_db_rewrite_sql, the logic here is to change all the queries

There are a few reasons this is largely thought of as a node access problem:
1. the node module implements hook_db_rewrite_sql (you can also think of this as example #3) The logic here is related to the return value of the node_access_view_all_nodes() function. If TRUE, it doesn't do anything, if FALSE, then the hook changes the queries to include this join clause: "INNER JOIN node_access na ON na.nid = n.nid"

2. hook_db_rewrite_sql() is not a commonly implemented hook (it's difficult to name another module that implements it).

3. It hasn't been a problem until MySQL 5.0, so it's really just starting to show up.

Second, I want to help with this but am not sure what to do exactly.

This issue has been posted in several places (with forum module, with event module, with image module, on the development mailing list, and now here). This is a problem in both 4.6 and 4.7. I think some bigger discussion needs to happen about db_rewrite_sql() and hook_db_rewrite_sql() and their limitations. Once descisions are made about those functions, then we will better know how to procede in terms of what other modules need to do in terms of using db_rewrite_sql(). Then we will what patches are needed. As someone fairly new to drupal, I don't really understand how the whole core development part works and the chain of command and the descision making process, and this is why I am not sure what to do to help. Your advice would be invaluable.

In the immediate term though, here is a rewrite of the query for webchick's problem above that I think should do the trick

SELECT n.nid, f.tid, n.title, n.sticky, u.name, u.uid, n.created AS timestamp, n.comment AS comment_mode, l.last_comment_timestamp, IF(l.last_comment_uid != 0, cu.name, l.last_comment_name) AS last_comment_name, l.last_comment_uid, l.comment_count AS num_comments 
FROM {node} n INNER JOIN {users} u ON n.uid=u.uid INNER JOIN {term_node} r ON n.nid = r.nid INNER JOIN {forum} f ON n.vid = f.vid INNER JOIN {node_comment_statistics} l ON n.nid = l.nid INNER JOIN {users} cu ON l.last_comment_uid = cu.uid
WHERE n.status = 1 AND r.tid = %d

sorry, didn't know my previos post changed the whole issue title (I get confused and think I am commenting) this post just changes the title back to chx's suggestion

rjl , if you can, please test my patch.

Further simplification.

Further simplification.

@chx: part of the patch seems wrong

+    if (!empty($where)) {
+      $new = " WHERE $where ";
+    }
+    $new = " $join $new";

Sorry, my mistake.

I would argue against changing the function at all

Noting the parameters of the function:
There are 4: $query, $primary_table, $primary_field, $args

Noting the functionality of hook_db_rewrite_sql...
In 4.6 it only allows for nodes (see http://drupaldocs.org/api/4.6/function/hook_db_rewrite_sql)
In 4.7 (head) it adds vocabularies, terms and comments (see http://drupaldocs.org/api/head/function/hook_db_rewrite_sql)

The description for the hook says this about what values may be passed as $primary_table and $primary_field

For node objects, primary field is always called nid. For taxonomy terms, it is tid and for vocabularies it is vid. For comments, it is cid. Primary table is the table where the primary object (node, file, term_node etc.) is.

Regarding the db_rewrite_sql function see
in 4.6 (http://drupaldocs.org/api/4.6/function/db_rewrite_sql)
in 4.7 (head) (http://drupaldocs.org/api/head/function/db_rewrite_sql)

the $primary_field parameter is defined as follows

$primary_field Name of the primary field.

This should be as specific as the hook says above and should specifically state the same 4 fields that can be used, so that developers know what is allowed without having to hunt down the hook.

the $primary_table parameter is defined as follows

$primary_table Name or alias of the table which has the primary key field for this query. Possible values are: comments, forum, node, term_data, vocabulary.

I IMHO believe this is way to open. I think $primary_table should be limited to specific tables the same way $primary_field is. My suggestion would be "n" for node, "td" for term_data "v" for vocabulary, "c" for comment. And not allow any other tables, period.

Also I would add the following limitations for the value of $query.
The $primary_table must be the first table listed in the FROM clause
Implied cross joins are disallowed (ie queries like this: FROM {node} n, {users} u must be this: FROM {node} n INNER|LEFT|RIGHT JOIN {users} u ON ...)

The only change needed to the code would be slight changes to the function descriptions and paramenter definitions, and this is why I don't think the function itself really needs a patch at all

IMHO, this function works really great and is one of the most amazing aspects of drupal.

While my patch is needed so that we do not suffer from the order of JOINs the above me is totally right -- it seems that FROM x , y INNER JOIN boo ON x.f1 = boo.f1 does not work under MYSQL5 because this to JOIN on table y and x is nowhere to be seen.

Yes, implicit JOINs are not to be used. Upgraded doxygen, will upgrade the handbook later, merged webchick's patch into mine.

someone please test that if you revert http://drupal.org/files/issues/statistics-node_access.patch stats still work, I have no time for that. If it does, then my patch is good and solves all such issues.

i applied your patch against includes/database.inc and reverted modules/project/issue.inc to revision 1.145 (revision 1.146 was a patch i recently made to fix this problem via reordering the SQL handed to db_rewrite_sql() so the {node} table came first). i tried viewing some project issues as an anonymous user with node_privacy_byrole enabled. my test environment with issue.inc revision 1.145 failed without your patch and worked once i applied it. so your changes to db_rewrite_sql() appear to be good! i haven't tested the changes to forum that your patch makes, so i'm not going to set the status of this to "ready to commit". but, it appears the main bug is finally solved. thanks much!! at last, there appears to be an end in sight to my long nightmare of porting drupal modules to MySQL 5.0.12+. :)

-derek

sorry i left this out...just to be clear: i tested db_rewrite_nail_2.patch (2.78 KB)

I just wan to draw your attention to the following error report for using OG module:
http://drupal.org/node/52012

thoughts on the og module problems

on line 626

$result = pager_query(db_rewrite_sql("SELECT og.nid, n.title, r.body, n.uid, u.name, og.description FROM {og} og INNER JOIN {node} n ON og.nid = n.nid INNER JOIN {node_revisions} r ON r.vid = n.vid INNER JOIN {users} u ON n.uid = u.uid WHERE og.directory=1 AND n.status=1 ORDER BY n.nid DESC", 'og', 'nid'), 50);

This could potentially be a problem:
note the 2nd argument ($primary_table) 'og' for the db_rewrite_sql() function. Why not use 'n' as the node table is present in the query?

on lines 1409 and 1410

$sql = "SELECT COUNT(*) FROM {node} n INNER JOIN {og} og ON n.nid = og.nid WHERE og.directory=1 AND n.type = 'og' AND n.status = 1";
$cnt = db_result(db_query(db_rewrite_sql($sql)));

This could also potentially be a problem if joining the node_access table returns more records than the query without the join does. The distinct logic won't kick in for this query.

the implementation of hook_db_rewrite_sql()

function og_db_rewrite_sql($sql, $primary_table, $primary_field,  $args) {
  if (is_numeric($args['og_nid'])) {
    // we add this JOIN for node admins and when node access is disabled because node_rewrite_sql() adds it for everyone else
    if (user_access('administer nodes') || node_access_view_all_nodes()) {
      $query['join'] = 'INNER JOIN {node_access} na ON n.nid = na.nid'; 
    }
    $where[] = '(na.gid = '. $args['og_nid']. " AND na.realm = 'og_subscriber')"; // private nodes
    $where[] = '(na.grant_view = '. $args['og_nid']. " AND na.realm = 'og_public')"; // public nodes
    $query['where'] =  implode(' OR ', $where);
    return $query;
  }
}

Given the logic, it is possible to output the WHERE conditions without the JOIN (this seems to be the problem jyang is having). I think that both the join and where functionality be wrapped in your 2nd if statement

I think chx's patch is great and nice to see that the order of the joins shouldn't then matter. It makes it a better function.

One more question?

What about restricting the values allowed for the $primary_table value?

The reason I ask is that in my post above regarding the og module, the first section notes use of 'og' as the value of the $primary_table. If I have two modules that that implement hook_db_rewrite_sql and perform the check exampled in drupaldocs for the hook (on one of my sites I currently have this situation).

if ($primary_table != 'n') { 
  $return['join'] = "LEFT JOIN {node} n ON $primary_table.nid = n.nid"; 
} 

The resulting join (using the og module join as example) would be
FROM {og} og INNER JOIN {node} n ON og.nid = n.nid INNER JOIN {node} n ON og.nid=n.nid INNER JOIN {node} n ON og.nid = n.nid INNER JOIN {node_revisions} r ON r.vid = n.vid INNER JOIN {users} u ON n.uid = u.uid
This will provide a non-unique alias SQL error for using the alias "n" multiple times

Thanks
Randall

Sorry, the resulting join (using the og module join as example) would acutally be
FROM {og} og INNER JOIN {node} n ON og.nid = n.nid INNER JOIN {node_revisions} r ON r.vid = n.vid INNER JOIN {users} u ON n.uid = u.uid LEFTJOIN {node} n ON og.nid=n.nid LEFT JOIN {node} n ON og.nid = n.nid

None the less, it will still provide a non-unique alias SQL error for using the alias "n" multiple times

My patch _is_ good to go. I am asking the committer to reset the issue to active once it's in and we will cook a patch for the remaining taxo queries.

I had also had the error Unknown column 'n.nid' when enabling Organic Group. Db_rewrite_nail_2.patch fixed the problem.

Let's first figure out the problem with the remaining taxonomy queries ...

Why did we still had to rewrite the forum.module query? Isn't the parser smart enough?

I explained it above: FROM x , y INNER JOIN boo ON x.f1 = boo.f1 does not work under MYSQL5 because this to JOIN on table y and x is nowhere to be seen.

It's not my parser. The above query would fail written by humans. Simply, you can not use the FROM x, y syntax.

I redid the taxonomy queries as requested, and rolled it into chx's patch. I have not had the time to test the queries, but they are all quite straightforward. Please review.

I applied the patch, so far it seems all the errors are gone, thank you all for the hard work.

I revised the system and much to my surprise I found lines like:

$join[] .= $result['join'];

What I've been smoking?

Just wondering if this is also good?
- $query .= ' WHERE '. $where;
+ $query .= $new;

Thanks

yes, that's good.

Sorry, I mean the dot "." in front of equal sigh?

Code looks good. Needs significant testing.

i re-tested the problem with MySQL 5, and updated HEAD drupal install, revision 1.145 of modules/project/issue.inc (which has the known-broken SQL query) and db_rewrite_sql_4.patch (7.51 KB). once again, before the patch, the query died and nothing worked when trying to view project issues as an anonymous user. after the patch, everything's fine.

further evidence that this patch is RTBC.

thanks,
-derek

Committed to HEAD. Thanks. Great work!

Is someone going to test and commit this to the 4.6 tree? Currently anyone on 4.6 and MySQL 5.0.12+ is dead.

i noticed this issue got automatically closed by cron (since the cvs version of it has been fixed). however, this is still broken in 4.6.6. i don't know if this should be marked as "won't fix" (which would be a shame), but at this point, i think we should re-roll this patch for 4.6 and commit it. i won't spend any time on this until someone in a position of authority marks it to "code needs work". if it's set as "won't fix", i won't press the issue, but i think drupal core should *definitely* support MySQL 5.0.12+, and we really need this patch for that to be true. so please, set this to "code needs work" and i'll see what i can do.

thanks,
-derek

and many contrib modules have been patched, 1-by-1, to be MySQL compliant in the face of the old db_rewrite_sql() behavior, too. i suppose we don't care about changing core in a stable release to make it easier for contrib modules to get this right. so, yeah, minimum new patch would be to manually re-order the remaining core queries (i'd have to look for them to be sure, but webchick, you're probably right that it's only forum + statistics at this point). given the effort that went into the fundamental fix to db_rewrite_sql(), and given that i can't imagine how that's really 4.7-specific, i'd be in favor of including that change in 4.6, too. but, as i said, i can see why people would be hesitant to do that, and i'd be willing to live with 4.6 == manually reordered queries and >=4.7 == works like a charm. ;)

thanks,
-derek

After closing another issue about this problem, we really need to resolve this problem!

@dww: take a look at the (now marked duplicated) issue http://drupal.org/node/49430
@chx: what can be done?

Should we mark this as "won't fix" and state that Drupal 4.6.x *DOES NOT* work with mysql 5.x ?

I'm sorry I am a usless morronic idiot

I have no idea what to do with that "patch file"

And before I get some php developer clique answer that I will be unable to understand I'm an utterly useless fat, pathetic, n00b, dickhead moron fishfaced spam brain who still can only use that broken and worthless OS Windows

I appologise if my stench offends you "gods".

and anyway you are wrong about 4.1.18-standard

We are slipping off topic so please do NOT answer but there is a handbook page aboutappling patches on Windows.

Giving this a final shot! Is there anyone using Drupal 4.6 with mysql5 or need to?
According to dww at #47, is there anyone with some authority to take a decision about this?

@magico: I believe this is now "won't fix" with the 5.0 release. Closing, but feel free to reopen -)

had a bad luck to install private nodes module on a production site with Drupal5.x and MySQL5 and can't get rid of the exactly same message. so the problem still persists.

yngens - there were numerous queries which were revised and might be producing various subtly different error messages. Please paste the specific problem and any steps needed to reproduce.

Any chance this is a duplicate of http://drupal.org/node/183012?

drumm, i have solved my problem here: http://drupal.org/node/250615
thank you

Same problem here with Drupal 5.10 and Refine by taxonomy 5.x 0.1 (http://drupal.org/node/230460). The curious thing is it works perfectly with Firefox 3.0 but I get twice the same error with Internet Explorer 6:

user warning: Unknown column 'n.nid' in 'on clause' query: SELECT DISTINCT tn.tid FROM term_node tn INNER JOIN term_data td ON td.tid = tn.tid INNER JOIN node_access na ON na.nid = n.nid WHERE (na.grant_view >= 1 AND ((na.gid = 0 AND na.realm = 'all') OR (na.gid = 1 AND na.realm = 'workflow_access') OR (na.gid = 0 AND na.realm = 'workflow_access_owner') OR (na.gid = 1 AND na.realm = 'forum_access'))) AND ( (tn.nid IN (97,96,95,91,89,87,84,83,82,81)) AND (td.vid = 6) AND NOT (tn.tid IN (73) )) in /home/macarro/www/includes/database.mysql.inc on line 172. 

user warning: Unknown column 'n.nid' in 'on clause' query: SELECT DISTINCT tn.tid FROM term_node tn INNER JOIN term_data td ON td.tid = tn.tid INNER JOIN node_access na ON na.nid = n.nid WHERE (na.grant_view >= 1 AND ((na.gid = 0 AND na.realm = 'all') OR (na.gid = 1 AND na.realm = 'workflow_access') OR (na.gid = 0 AND na.realm = 'workflow_access_owner') OR (na.gid = 1 AND na.realm = 'forum_access'))) AND ( (tn.nid IN (97,96,95,91,89,87,84,83,82,81)) AND (td.vid = 5) AND NOT (tn.tid IN (73) )) in /home/macarro/www/includes/database.mysql.inc on line 172. 

Hi,

Will this be committed in Drupal 5.11 please (after current version 5.10)?
On different places I have this error. And it seems that the error is there because of MYSQL 5 differences with MYSQL 4.

Off course every module builder can alter their module, but is it not better to commit the patch here?

Greetings,
Martijn

#331651: db_rewrite_sql improperly rewriting a subquery