Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I've observed that node_access for node/n gets invoked three times for "view" and once for "update" for a normal user on a bare-.bones Drupal 6 install. If you have some contrib module which make similar checks, this adds up. The result of the SQL and other lookups should be statically cached. The attached patch does this.
The patch is for D6 but also applies with some offset to HEAD.
I am a bit confuesed as to why this hasn't been noticed before.
Comment | File | Size | Author |
---|---|---|---|
#37 | drupal_353595_node_access-37.patch | 2.41 KB | hefox |
#35 | node_access_353595-35.patch | 2.24 KB | msonnabaum |
#34 | node_access_353595.patch | 2.25 KB | catch |
#33 | node_access_353595_D6.patch | 2.25 KB | catch |
#31 | 353595-node_access_static-d6.patch | 1.89 KB | catch |
Comments
Comment #1
yched CreditAttribution: yched commentedI don't think you intended to leave that in :
Comment #2
dawehnerif the rights are cached there should be the possibility to reset the cache
Comment #3
dawehnershouldn't be there be a patch to test this functionality
Comment #4
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedMaybe, I haven't a clue about writing tests, though.
Also, maybe the whole query shouldn't be run at all if you have a site which doesn't use node_access (such as drupal.org).
I imagine that we could do a count on the node_access table similar to what path.module does.
Comment #5
dawehneri 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?
Comment #6
dawehnerduring testing node_access i have an issue with user_access see http://drupal.org/node/353679
Comment #7
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedUpdated patch to check the number of entries in node_access, also putting this back to "needs review". There currently is no test for node_access and is isn't the topic of this patch to create one. I've created a separate issue here: http://drupal.org/node/353847
Comment #8
dawehnerif seems fine,
but if you reset rights you should also reset count
posted a updated patch
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedsee the description here of bugs that result from using
unset
on statics: http://drupal.org/node/275796should be:
Comment #10
dawehnerinteresting thx, thx
here is the updated patch
Comment #11
dawehnerComment #12
moshe weitzman CreditAttribution: moshe weitzman commentedOne 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.
Comment #13
catchShouldn'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.
Comment #14
catchAlso it should use ->fetchField() instead of db_result(), so marking CNW for that.
Comment #15
dawehnerhere is a rerole the dbtng is solved through another issue.
if #353847: Tests for node_access is ready i will add a test.
Comment #17
Gerhard Killesreiter CreditAttribution: Gerhard Killesreiter commentedtagging
Comment #18
catchOn 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.
Comment #19
moshe weitzman CreditAttribution: moshe weitzman commentedInstead 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.
Comment #20
catchThis 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:
Patch:
More generally, my test D6 install does the same rough page at 23.04 #/sec - so still a long way to go..
Comment #21
catchCaching when given a node type too now, which has the benefit of being a bit more concise as well.
Comment #22
catchMissed one, also updated benchmarks (apc on, xdebug off).
HEAD:
Patch:
When profiling, node_access() comes up anywhere between 5-10% of the request, so that seems about right.
Comment #23
catchgrr
Comment #24
HedgeMage CreditAttribution: HedgeMage commentedI 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
Comment #25
moshe weitzman CreditAttribution: moshe weitzman commentedLooks good to me. RTBC.
My current distaste for new caches is mostly limited to persistent caches. This is just a static var.
Comment #26
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks!
Comment #28
catchThis was never backported, node_access() still issues four duplicate queries in D6.
Comment #29
catchHere'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.
Comment #30
catchOne 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.
Comment #31
catchAnd this brings it closer in line to the D7 code.
Comment #32
catchNeeds the $cid = is_object($node) ? $node->nid : $node; from the D7 version as well.
Comment #33
catchGets uglier but here's a re-roll.
Comment #34
catchWithout the D6 suffix so it triggers test bot.
Comment #35
msonnabaum CreditAttribution: msonnabaum commentedSame patch with extra whitespace removed.
Patch looks good. I'm no longer seeing any duplicate queries in node_access.
Comment #36
armin1980 CreditAttribution: armin1980 commentedI applied this patch to Node on Pressflow. Patched successfully but significantly reduced node loading speed.
Comment #37
hefox CreditAttribution: hefox commentedIsn't this a uneeded white space change?
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.