Closed (fixed)
Project:
Drupal core
Component:
node.module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
1 Feb 2005 at 15:54 UTC
Updated:
5 Mar 2005 at 11:15 UTC
Jump to comment: Most recent file
I've got a Drupal 4.5.2 site running with Taxonomy Access Control. When you go to view a taxonomy and it tries to do an access check, the SQL query takes literally minutes to complete. This is on a dual Xeon box, it's rather beefy and runs two other sites (vBulletin and a hacked up Postnuke site) just fine. The problem comes from the taxonomy_select_nodes function
$sql_count = 'SELECT COUNT(DISTINCT(n.nid)) FROM {node} n '. node_access_join_sql() .' INNER JOIN {term_node} tn ON n.nid = tn.nid WHERE tn.tid IN ('. $str_tids .') AND n.status = 1 AND '. node_access_where_sql();
Which ends up looking like
SELECT COUNT(DISTINCT(n.nid)) FROM node n INNER JOIN node_access na ON (na.nid = 0 OR na.nid = n.nid) INNER JOIN term_node tn0 ON n.nid = tn0.nid WHERE n.status = 1 AND na.grant_view = 1 AND CONCAT(na.realm, na.gid) IN ('all0','term_access1') AND tn0.tid IN (9)
This will take several minutes to complete. The query next to it that selected the nodes themselves took 3 minutes to complete.
Where is the problem exactly? I've seen elsewhere that this is a problem for node_privacy_byrole as well, which makes sense because really all the various access control modules are contributing here is an extra entry in the concat search.
Let me know if you need any details of how the MySQL server is configured. Here's some data from my drupal database
mysql> select count(nid) from node;
+------------+
| count(nid) |
+------------+
| 10376 |
+------------+
1 row in set (0.00 sec)
mysql> select count(nid) from node_access;
+------------+
| count(nid) |
+------------+
| 41508 |
+------------+
1 row in set (0.00 sec)
Drupal performs very snappily with the node_access stuff disabled, and otherwise it performs very well. A three minute increase in the execute time of an SQL query is just strange, there's something odd going on here I think.
| Comment | File | Size | Author |
|---|---|---|---|
| #23 | node_access.patch | 2.91 KB | jonbob |
| #8 | drupal_test_db.tar.bz2 | 734.78 KB | pyromanfo |
Comments
Comment #1
pyromanfo commentedReducing the size of the 'realm' field of node_access to something manageable like 12 instead of the current 255 speeds up the query tremendously, still it's rather slow.
I also added a search index on the grant_view permission, since the view permission is the only permission you'd want to check for more than one node usually. It did speed it up, however it still take a minute or so on my little test box. I'm going to tweak with it some more and see if I can get it performing better.
Comment #2
killes@www.drop.org commentedCould you give us an estimate with numbers? Or even better, make available the database for testing?
Comment #3
pyromanfo commentedI can't really make the database available without going through and setting all the text and titles to something random and the uid's to 2, I may try that.
I can give you some more numbers tonight though when I start messing with it again. You want execution times and table sizes? Any other specific numbers? Should I just dump show status?
Comment #4
killes@www.drop.org commentedAverages of execution times before and after would be fine, I think.
The db would be great of course You could simple set all text to "foo" and email addresses to "root@localhost".
Comment #5
pyromanfo commentedI'm running these tests on a similar database I haven't been developing on. Created from the same source data with the same script though, I just haven't been hacking it up and it runs on faster hardware.
Then I ran the following queries
alter table node_access modify realm varchar(12) NOT NULL default '';
alter table node_access add index view_indx (nid,gid,realm,grant_view);
Then I ran
mysqlcheck --auto_repair -os
Then the query again
Not as big a speed boost as I thought I was seeing unfortunately.
Here's 'show variables'
I'm attaching my test database which takes several minutes to execute a similar query
If you want me to run the tests on the test database I can do that, it's just on a much skimpier machine and it takes quite a bit longer (Athlon 600, 256MB Ram vs Dual Xeon).
Comment #6
pyromanfo commentedOops, preview removed the test db, here it is.
Comment #7
pyromanfo commentedIt's too big, it's 4.47 MB, I'm gonna put it up on GWJ and link to it.
Get it here
Comment #8
pyromanfo commentedNevermind, I left in some sensitive info and forgot to wipe the search index, now it's much smaller.
Comment #9
killes@www.drop.org commentedThanks, but I think you stripped the DB a bit too much. If I execute the query it is very fast because it does not return anything...
Comment #10
killes@www.drop.org commentedWhen I use the query from your original post, I get some result. I guess I will write a script and run some tests over night...
Comment #11
killes@www.drop.org commentedOk, here are some preliminary results:
- The unchanged query from the original post runs nearly 14 minutes on the hardware redLED lent to us.
- If I shorten the varchar field to 18 from 255, it gets about halfed.
- neither adding an index for the realm nor the grant_view field does make a change
- Omitting the "na.nid = 0 OR" part makes the query very fast (quarter of a second)
- SELECT COUNT(DISTINCT(n.nid)) FROM node n INNER JOIN node_access na ON ( na.nid = n.nid) left jo
in node_access nb ON nb.nid = 0 INNER JOIN term_node tn0 ON n.nid = tn0.nid WHERE n.status = 1 AND ((n
a.grant_view = 1 AND CONCAT(na.realm, na.gid) IN ('all0','term_access1')) OR (nb.grant_view = 1 AND CON
CAT(nb.realm, nb.gid) IN ('all0','term_access1'))) AND tn0.tid IN (9);
This query is still very much faster (about half a second) than the original query and gives the same result for your database. I don't know if it is fully equivalent, though. Note that it is a left join.
Comment #12
pyromanfo commentedThe na.nid = 0 stuff is basically a catchall, that's sort of a default permission that overrides anything else.
This is good when you're not using node_access, however when you are the na.nid=0 stuff probably isn't there, I know Node Privacy By Role and Taxonomy Access Control both delete the entries for nid=0.
Perhaps it's a good idea to control this another way, by setting a variable and using that to determine whether or not some sort of node_access is installed, as it seems using nid = 0 as a default simply kills performance.
Comment #13
pyromanfo commentedOh, and thanks for taking a good look at it killes, I should've explained better. The DB I provided, the test DB, has a large number of nodes in tid=9, the other DB I ran the tests on has alot in tid=2.
Comment #14
(not verified) commentedThe more I think about replacing nid=0 with a configuration setting the more it makes sense. The two major node access modules right now have to have a seperate "enable/disable" setting to deal with nid=0, since it overrides everything. In fact, it seems like it's only purpose it to override all other node access settings with a universal default.
Why not instead have a global configuration setting under permissions that enables or disable node level permissions, then the various node access modules wouldn't need a seperate disable/enable setting. Furthermore, this performance problem would disappear, because either the node_access stuff would be there and you aren't looking for nid=0, or it isn't and you don't care what's in node_access. It seems it'd save queries to node access when you're not using node level permissions, and make it cleaner to implement a node access modifying module.
Comment #15
pyromanfo commentedI hate the site randomly logging you out, that was me above.
Comment #16
jonbob commented"The more I think about replacing nid=0 with a configuration setting the more it makes sense. The two major node access modules right now have to have a seperate "enable/disable" setting to deal with nid=0, since it overrides everything. In fact, it seems like it's only purpose it to override all other node access settings with a universal default."
This is not the case. I think you're confusing the case of nid=0 with the case of realm=all.
As an example of the utility of nid=0, consider the scenario given in the example access module. This module implements a simple system whereby anyone can view "public" nodes, and only users with a specific permission can view "private" nodes. This can be rephrased as follows: anonymous users can only view "public" nodes, while privileged users can view all nodes. A grant with nid=0 takes care of the second case, so that the module only needs to deal with granting permission to view the public nodes as necessary.
Gerhard's query works in the case where every node has at least one node_access entry with its nid, but I'm afraid it fails in the above example. A node that is private would not have any corresponding entry, and so the inner join would fail for it, returning no record. The left join doesn't even have a chance to be matched.
It would be worth benchmarking the scenario in which the nid=0 possibility is removed from the query, but the node_access table has an entry added for each node instead of a single nid=0 entry. If this is faster than the OR in the join, the extra DB storage is probably worth the change.
Comment #17
pyromanfo commentedI think there's a bug in the module, in the docs in the top shouldn't the initial SQL statement be adding that permission to gid=1?
Though you're right, I'd forgotten because I hadn't seen any of the current modules use it.
As for that performance comparison
So I don't think it improves performance for the nid=0 case, though it is suprisingly slow.
Comment #18
moshe weitzman commentedperhaps we should split the query into 2 queries in order to avoid the 'na.nid = 0' part of the JOIN. Alternatively, perhaps we could inject that clause only when needed using the db_rewrite_sql hook. not sure.
Comment #19
pyromanfo commentedSplitting it into two queries would make sense I think.
Basically you could have it entirely in node_access_join_sql and node_access_where_sql. There we can check for the nid=0 case for any gids and realms. Then if it returns true, we know we don't have to check anything else, so we return null for the SQL to enter. We could even store this in a static variable to avoid calling this twice for join_sql and where_sql
So in the nid=0 case, this should be similarly fast as we're removing a table from the JOIN in exchange for checking node_access for one nid.
It looks to be faster, actually.
Then, when we don't get a result, we check node access with the 'na.nid=0 OR' removed. This is certain to be faster as the test have shown, taking the query down from 7 minutes to a matter of seconds.
In either case, this method would be faster. Plus, it wouldn't require any changes to other modules, simply change the node_access_where_sql and node_access_join_sql functions.
JonBob, would you be okay with that? I can code it if you think you'll accept the patch.
Comment #20
(not verified) commentedSplitting the queries into two is probably the only way to go. However, I'd like to avoid calling the nid = 0 query if possible. My Drupal installations that use node access do not use nid = 0 at all. I think we could store the available (nid = 0) => realm combinations somewhere and only execute the query if a matching condition is found. Gerhard
Comment #21
moshe weitzman commentedthat nid=0 is going to be very fast when done all by itself. no need to introduce any other mechanism IMO. testing will tell us for sure though
Comment #22
killes@www.drop.org commentedThe nid = 0 query would be executed for every node query on a page call even if your access module doesn't use nid = 0 at all. Most implementations do not use nid = 0. If we can avoid a query we should do so.
Comment #23
jonbob commentedHere's a patch that splits the nid=0 check off into a separate query. If the user has such a grant, things should be really fast since we don't need any join or even DISTINCT() check. If not, the join is done as before, but without the "OR nid=0" piece.
This ideally needs performance checking on systems using node access with nid=0 rows, using it without them, and not using it at all.
Comment #24
killes@www.drop.org commentedOk, here is a first test result:
After installing the node_privacy_by_role module for the drupal.org database, the Drupal install became unusable for all but the No. 1 user and the load went high due to many hanging mysql processes. After applying the patch the site does work again. The module seems to generate one entry per node and role in the node_access table. I did not make any benchmarks, the result is so obvious.
Comment #25
killes@www.drop.org commentedI've used a script provided by JonBob to create entries in the node_access table and installed the node_access_example module. The script generated about 500 node_access entries. I then benchmarked the tracker page.
Results:
11.78 +- 0.48 s with patch
17.04 +- 0.70 s without patch
The patched version needs only 70% of the time the non patched version needs.
Comment #26
(not verified) commentedI haven't run numbers, but subjectively my test site seems much faster with the patch in the case where there is a nid=0 row, a bit faster where there is no such row (but not as obvious), and about the same with node access turned off.
Comment #27
jonbob commentedI've reviewed the changes once again, and I'd like to see this committed. Seems a very necessary performance improvement, and more and more sites are using node-level access rights and would benefit from it.
Comment #28
matt westgate commented+1 as well
This patch would eliminate the performance penalty sites currently have that only use node-level permissions for group editing, and keep all content public.
Comment #29
dries commentedCommitted to HEAD.
Comment #30
(not verified) commented