node_access issues four queries on default install node/1 and has no static cache

I don't think you intended to leave that in :

+    watchdog('nodeacces_sql', $module ." ". $op ." ". serialize($node) .' '. $sql);

if the rights are cached there should be the possibility to reset the cache

shouldn't be there be a patch to test this functionality

i looked for a node_access test and i didn't find one,
some red on http://acquia.com/files/test-results/modules_node_node.module.html

so there should be first a specific node_access test before writing the test here.

how do you manage to disable node_access on drupal.org?

during testing node_access i have an issue with user_access see http://drupal.org/node/353679

if seems fine,
but if you reset rights you should also reset count

posted a updated patch

see the description here of bugs that result from using unset on statics: http://drupal.org/node/275796

    unset($count);

should be:

    $count = NULL;

interesting thx, thx

here is the updated patch

One unfortunate byproduct of our new menu system in D6 is that it performs access callback calculations for each local task. Thats why you are seeing this duplication.

This seems like the right solution to me.

I don't think you will get this committed without adding a test but feel free to make a case.

Shouldn't the count() query be done earlier on? iirc node_get_types() hits the database too, so we should save a query there as well if we can.

Also it should use ->fetchField() instead of db_result(), so marking CNW for that.

here is a rerole the dbtng is solved through another issue.

if #353847: Tests for node_access is ready i will add a test.

tagging

On a default D7 install, node_access() issues three SQL queries - whether any modules which implement hook_node_grants() are enabled or not, and they're the same queries.

Bumping to critical since this takes 5.73% of the request in total. My latest benchmarks have Drupal 7 node/1 at 15 requests per second, Drupal 6 at 20 requests per second, so we're still 25% slower on that page out of the box.

Instead of querying node_access table, could we do: count(module_implements('node_grants'). thats what we do inside of node_access_rebuild(). needs some thought.

This is a re-roll of #15 with #19 accounted for.

I'm not really keen on returning so often from node_access() instead of setting $return - but there's no clear if elseif path either, and that's not invented here.

Also it seems $node can be either an object or a node type, but the patch currently only caches on nid rather than node type. I'm tempted to change this to a ternary and cache on type as well, not done yet, but not the main gain here either. However this is enough for benchmarks, which are promising.

This removes at least four queries from node/1 for a normal auth or anonymous user with no node access modules installed (I missed one query when counting before), and should pass all node tests - we now have quite extensive tests for node_access() which failed while I was working on the patch in all kinds of different ways so that's no longer a stopper here.

Here's benchmarks with ab:

HEAD:

Concurrency Level:      1
Time taken for tests:   62.705 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      6907000 bytes
HTML transferred:       6359000 bytes
Requests per second:    15.95 [#/sec] (mean)
Time per request:       62.705 [ms] (mean)
Time per request:       62.705 [ms] (mean, across all concurrent requests)
Transfer rate:          107.57 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    53   63   9.5     59      92
Waiting:       53   62   9.5     59      92
Total:         53   63   9.5     59      92

Patch:

Concurrency Level:      1
Time taken for tests:   58.934 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      6907000 bytes
HTML transferred:       6359000 bytes
Requests per second:    16.97 [#/sec] (mean)
Time per request:       58.934 [ms] (mean)
Time per request:       58.934 [ms] (mean, across all concurrent requests)
Transfer rate:          114.45 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    50   59   9.1     55     106
Waiting:       50   59   9.0     55     105
Total:         50   59   9.1     55     106

More generally, my test D6 install does the same rough page at 23.04 #/sec - so still a long way to go..

Caching when given a node type too now, which has the benefit of being a bit more concise as well.

Missed one, also updated benchmarks (apc on, xdebug off).

HEAD:

Concurrency Level:      1
Time taken for tests:   61.394 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      6907000 bytes
HTML transferred:       6359000 bytes
Requests per second:    16.29 [#/sec] (mean)
Time per request:       61.394 [ms] (mean)
Time per request:       61.394 [ms] (mean, across all concurrent requests)
Transfer rate:          109.87 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    54   61   6.4     59      98
Waiting:       54   61   6.4     59      98
Total:         54   61   6.4     59      98

Patch:

Document Path:          /node/1
Document Length:        6359 bytes

Concurrency Level:      1
Time taken for tests:   56.805 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      6907000 bytes
HTML transferred:       6359000 bytes
Requests per second:    17.60 [#/sec] (mean)
Time per request:       56.805 [ms] (mean)
Time per request:       56.805 [ms] (mean, across all concurrent requests)
Transfer rate:          118.74 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    51   57   5.7     55     108
Waiting:       51   57   5.7     55     107
Total:         51   57   5.7     55     108

When profiling, node_access() comes up anywhere between 5-10% of the request, so that seems about right.

grr

I applied this to a copy of head and played around on a D7 test site...I see nothing weird going on. After catch entertained a couple of (probably dumb) questions on my part I understand the code involved, too (thanks catch!), and it looks good. I'm sleepy, so I'm not going to benchmark anything tonight, but consider this a very positive review. :D

Looks good to me. RTBC.

My current distaste for new caches is mostly limited to persistent caches. This is just a static var.

Committed to CVS HEAD. Thanks!

This was never backported, node_access() still issues four duplicate queries in D6.

Here's a start on a backport, for now this just adds the static cache with no other changes to keep the patch as small as possible.

One bug found in the caching so far - was causing edits tabs to display for people with no access, although you'd still get proper access denied when trying to visit the page.

And this brings it closer in line to the D7 code.

Needs the $cid = is_object($node) ? $node->nid : $node; from the D7 version as well.

Gets uglier but here's a re-roll.

Without the D6 suffix so it triggers test bot.

Same patch with extra whitespace removed.

Patch looks good. I'm no longer seeing any duplicate queries in node_access.

I applied this patch to Node on Pressflow. Patched successfully but significantly reduced node loading speed.

+++ b/modules/node/node.module
@@ -2020,20 +2021,44 @@ function node_access($op, $node, $account = NULL) {
+

Isn't this a uneeded white space change?

+++ b/modules/node/node.module
@@ -2020,20 +2021,44 @@ function node_access($op, $node, $account = NULL) {
+  if (isset($node->nid) && isset($rights[$cid])) {

Do all the work above to make a cid that works for 'create' or node nid, but don't bother using cached create permissions. Huh? Patch addresses that and makes it explicitly nid for non-create, node type for create.

(unassigning killes cause my understanding is assign is for when working on patch, not reviewing).

Using pressflow also.