Project: Drupal Version: cvs Component: node.module Category: tasks Priority: normal Assigned to: Anonymous Reported by: pyromanfo Updated by: JonBob -Status: active +Status: patch Attachment: http://drupal.org/files/issues/node_access.patch (2.91 KB) Here'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. JonBob Previous comments: ------------------------------------------------------------------------ February 1, 2005 - 10:54 : pyromanfo 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. ------------------------------------------------------------------------ February 3, 2005 - 16:02 : pyromanfo Reducing 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. ------------------------------------------------------------------------ February 3, 2005 - 18:12 : killes@www.drop.org Could you give us an estimate with numbers? Or even better, make available the database for testing? ------------------------------------------------------------------------ February 4, 2005 - 14:59 : pyromanfo I 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? ------------------------------------------------------------------------ February 4, 2005 - 15:21 : killes@www.drop.org Averages 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". ------------------------------------------------------------------------ February 5, 2005 - 12:31 : pyromanfo I'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. 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 (2); +------------------------+ | COUNT(DISTINCT(n.nid)) | +------------------------+ | 2993 | +------------------------+ 1 row in set (7 min 4.00 sec) 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 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 (2); +------------------------+ | COUNT(DISTINCT(n.nid)) | +------------------------+ | 2993 | +------------------------+ 1 row in set (6 min 18.62 sec) Not as big a speed boost as I thought I was seeing unfortunately. Here's 'show variables' mysql> show variables; +---------------------------------+---------------------------------------------------+ | Variable_name | Value | +---------------------------------+---------------------------------------------------+ | back_log | 50 | | basedir | /usr/local/mysql/ | | binlog_cache_size | 32768 | | bulk_insert_buffer_size | 8388608 | | character_set_client | latin1 | | character_set_connection | latin1 | | character_set_database | latin1 | | character_set_results | latin1 | | character_set_server | latin1 | | character_set_system | utf8 | | character_sets_dir | /usr/local/mysql/share/mysql/charsets/ | | collation_connection | latin1_swedish_ci | | collation_database | latin1_swedish_ci | | collation_server | latin1_swedish_ci | | concurrent_insert | ON | | connect_timeout | 5 | | datadir | /usr/local/mysql/var/ | | date_format | %Y-%m-%d | | datetime_format | %Y-%m-%d %H:%i:%s | | default_week_format | 0 | | delay_key_write | ON | | delayed_insert_limit | 100 | | delayed_insert_timeout | 300 | | delayed_queue_size | 1000 | | expire_logs_days | 0 | | flush | OFF | | flush_time | 0 | | ft_boolean_syntax | + -><()~*:""&| | | ft_max_word_len | 84 | | ft_min_word_len | 4 | | ft_query_expansion_limit | 20 | | ft_stopword_file | (built-in) | | group_concat_max_len | 1024 | | have_archive | NO | | have_bdb | NO | | have_compress | YES | | have_crypt | YES | | have_csv | NO | | have_example_engine | NO | | have_geometry | YES | | have_innodb | YES | | have_isam | NO | | have_ndbcluster | NO | | have_openssl | NO | | have_query_cache | YES | | have_raid | NO | | have_rtree_keys | YES | | have_symlink | YES | | init_connect | | | init_file | | | init_slave | | | innodb_additional_mem_pool_size | 1048576 | | innodb_autoextend_increment | 8 | | innodb_buffer_pool_awe_mem_mb | 0 | | innodb_buffer_pool_size | 8388608 | | innodb_data_file_path | ibdata1:10M:autoextend | | innodb_data_home_dir | | | innodb_fast_shutdown | ON | | innodb_file_io_threads | 4 | | innodb_file_per_table | OFF | | innodb_locks_unsafe_for_binlog | OFF | | innodb_flush_log_at_trx_commit | 1 | | innodb_flush_method | | | innodb_force_recovery | 0 | | innodb_lock_wait_timeout | 50 | | innodb_log_arch_dir | | | innodb_log_archive | OFF | | innodb_log_buffer_size | 1048576 | | innodb_log_file_size | 5242880 | | innodb_log_files_in_group | 2 | | innodb_log_group_home_dir | ./ | | innodb_max_dirty_pages_pct | 90 | | innodb_table_locks | ON | | innodb_max_purge_lag | 0 | | innodb_mirrored_log_groups | 1 | | innodb_open_files | 300 | | innodb_thread_concurrency | 8 | | interactive_timeout | 28800 | | join_buffer_size | 131072 | | key_buffer_size | 8388600 | | key_cache_age_threshold | 300 | | key_cache_block_size | 1024 | | key_cache_division_limit | 100 | | language | /usr/local/mysql/share/mysql/english/ | | large_files_support | ON | | license | GPL | | local_infile | ON | | locked_in_memory | OFF | | log | OFF | | log_bin | OFF | | log_error | | | log_slave_updates | OFF | | log_slow_queries | OFF | | log_update | OFF | | log_warnings | 1 | | long_query_time | 10 | | low_priority_updates | OFF | | lower_case_file_system | OFF | | lower_case_table_names | 0 | | max_allowed_packet | 1048576 | | max_binlog_cache_size | 4294967295 | | max_binlog_size | 1073741824 | | max_connect_errors | 10 | | max_connections | 100 | | max_delayed_threads | 20 | | max_error_count | 64 | | max_heap_table_size | 16777216 | | max_insert_delayed_threads | 20 | | max_join_size | 4294967295 | | max_length_for_sort_data | 1024 | | max_relay_log_size | 0 | | max_seeks_for_key | 4294967295 | | max_sort_length | 1024 | | max_tmp_tables | 32 | | max_user_connections | 0 | | max_write_lock_count | 4294967295 | | myisam_data_pointer_size | 4 | | myisam_max_extra_sort_file_size | 2147483648 | | myisam_max_sort_file_size | 2147483647 | | myisam_recover_options | OFF | | myisam_repair_threads | 1 | | myisam_sort_buffer_size | 8388608 | | net_buffer_length | 16384 | | net_read_timeout | 30 | | net_retry_count | 10 | | net_write_timeout | 60 | | new | OFF | | old_passwords | ON | | open_files_limit | 1024 | | pid_file | ********** | | port | 3306 | | preload_buffer_size | 32768 | | protocol_version | 10 | | query_alloc_block_size | 8192 | | query_cache_limit | 1048576 | | query_cache_min_res_unit | 4096 | | query_cache_size | 0 | | query_cache_type | ON | | query_cache_wlock_invalidate | OFF | | query_prealloc_size | 8192 | | range_alloc_block_size | 2048 | | read_buffer_size | 131072 | | read_only | OFF | | read_rnd_buffer_size | 262144 | | relay_log_purge | ON | | rpl_recovery_rank | 0 | | secure_auth | OFF | | server_id | 0 | | skip_external_locking | ON | | skip_networking | OFF | | skip_show_database | OFF | | slave_net_timeout | 3600 | | slow_launch_time | 2 | | socket | /tmp/mysql.sock | | sort_buffer_size | 2097144 | | sql_mode | | | storage_engine | MyISAM | | sync_binlog | 0 | | sync_frm | ON | | system_time_zone | EST | | table_cache | 64 | | table_type | MyISAM | | thread_cache_size | 0 | | thread_stack | 196608 | | time_format | %H:%i:%s | | time_zone | SYSTEM | | tmp_table_size | 33554432 | | tmpdir | | | transaction_alloc_block_size | 8192 | | transaction_prealloc_size | 4096 | | tx_isolation | REPEATABLE-READ | | version | 4.1.7 | | version_comment | Source distribution | | version_compile_machine | i686 | | version_compile_os | pc-linux | | wait_timeout | 28800 | +---------------------------------+---------------------------------------------------+ 176 rows in set (0.03 sec) 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). ------------------------------------------------------------------------ February 5, 2005 - 12:33 : pyromanfo Oops, preview removed the test db, here it is. ------------------------------------------------------------------------ February 5, 2005 - 12:38 : pyromanfo It's too big, it's 4.47 MB, I'm gonna put it up on GWJ and link to it. Get it here [1] [1] http://www.gamerswithjobs.com/testdb/drupal_test_db.tar.bz2 ------------------------------------------------------------------------ February 5, 2005 - 12:46 : pyromanfo Attachment: http://drupal.org/files/issues/drupal_test_db.tar.bz2 (734.78 KB) Nevermind, I left in some sensitive info and forgot to wipe the search index, now it's much smaller. ------------------------------------------------------------------------ February 7, 2005 - 17:58 : killes@www.drop.org Thanks, 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... ------------------------------------------------------------------------ February 7, 2005 - 18:12 : killes@www.drop.org When 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... ------------------------------------------------------------------------ February 7, 2005 - 18:55 : killes@www.drop.org Ok, 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. ------------------------------------------------------------------------ February 8, 2005 - 08:06 : pyromanfo The 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. ------------------------------------------------------------------------ February 8, 2005 - 08:08 : pyromanfo Oh, 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. ------------------------------------------------------------------------ February 8, 2005 - 11:17 : Anonymous 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. 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. ------------------------------------------------------------------------ February 8, 2005 - 11:17 : pyromanfo I hate the site randomly logging you out, that was me above. ------------------------------------------------------------------------ February 8, 2005 - 14:23 : JonBob "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 [2]. 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. [2] http://drupaldocs.org/api/head/file/contributions/docs/developer/examples/no... ------------------------------------------------------------------------ February 8, 2005 - 20:08 : pyromanfo "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. " I 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 mysql> SELECT COUNT(DISTINCT(n.nid)) FROM node n INNER JOIN term_node tn0 ON n.nid = tn0.nid INNER JOIN node_access na ON (na.nid = n.nid) WHERE n.status = 1 AND na.grant_view = 1 AND CONCAT(na.realm, na.gid) IN ('all0','term_access1') AND tn0.tid IN (2); +------------------------+ | COUNT(DISTINCT(n.nid)) | +------------------------+ | 2993 | +------------------------+ 1 row in set (0.23 sec) mysql> update node_access set realm='blam' where realm='term_access'; Query OK, 41508 rows affected (5.42 sec) Rows matched: 41508 Changed: 41508 Warnings: 0 mysql> insert into node_access (nid,gid,realm,grant_view) values (0,1,'term_access',1); Query OK, 1 row affected (0.00 sec) mysql> SELECT COUNT(DISTINCT(n.nid)) FROM node n INNER JOIN term_node tn0 ON n.nid = tn0.nid INNER JOIN node_access na ON (na.nid = 0 OR na.nid=n.nid) WHERE n.status = 1 AND na.grant_view = 1 AND CONCAT(na.realm, na.gid) IN ('all0','term_access1') AND tn0.tid IN (2); +------------------------+ | COUNT(DISTINCT(n.nid)) | +------------------------+ | 2993 | +------------------------+ 1 row in set (0.16 sec) So I don't think it improves performance for the nid=0 case, though it is suprisingly slow. ------------------------------------------------------------------------ February 8, 2005 - 20:57 : moshe weitzman perhaps 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. ------------------------------------------------------------------------ February 9, 2005 - 23:36 : pyromanfo Splitting 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. mysql> select count(nid) from node_access where nid=0 and gid=1 and realm in ('all','term_access','blah'); +------------+ | count(nid) | +------------+ | 0 | +------------+ 1 row in set (0.37 sec) SELECT COUNT(DISTINCT(n.nid)) FROM node n INNER JOIN term_node tn0 ON n.nid = tn0.nid WHERE n.status = 1 AND tn0.tid IN (9); +------------------------+ | COUNT(DISTINCT(n.nid)) | +------------------------+ | 4202 | +------------------------+ 1 row in set (0.15 sec) 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','blah1') AND tn0.tid IN (9); +------------------------+ | COUNT(DISTINCT(n.nid)) | +------------------------+ | 4202 | +------------------------+ 1 row in set (1.97 sec) 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. ------------------------------------------------------------------------ February 10, 2005 - 08:38 : Anonymous Splitting 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 ------------------------------------------------------------------------ February 10, 2005 - 10:14 : moshe weitzman that 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 ------------------------------------------------------------------------ February 10, 2005 - 13:31 : killes@www.drop.org The 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. -- View: http://drupal.org/node/16558 Edit: http://drupal.org/project/comments/add/16558