I use npbr on a large site with quite a few roles. One of our performance killers are related to the queries that are generated by npbr. I'm not close to a patch yet so I thought I would open the discussion so that possibly the maintainer could give insight into this improvement. The first obvious thing to fix is to use a join instead of many and's that are appended to check each role for the current user. http://pastie.org/994423 This is a query I caught which gives a count alias? but I've noticed before this happens on quite a few different types of queries.

In concern of just simplifying the query and using keys instead of conditions for each role see below:

select * from node_privacy_byrole as npb,users_roles as ur where nid='1195613' and ur.uid='1014161' and ur.rid=npb.gid

Obviously the nid value and uid value must exist and will be a variable once in the module. So I guess my first question is what function in this module generates the conditional part of a query when a node is loaded by a user. Then we can further refine this query to account for other things I may not be aware of.

I can see the funky conditions for each role when I use _node_access_join_sql('node'); in a custom module of mine which I assume drupal core is using to simply display the node.

Thanks!

Comments

jlporter’s picture

jlporter commented
Project: node privacy byrole » Node
Version: 6.x-1.5 »
Component: MySQL » Code

I thought this was node_privacy_byrole code creating the queries but I now see that it is created by node_access function.

    foreach (node_access_grants($op, $account) as $realm => $gids) {
      foreach ($gids as $gid) {
        $grants[] = "(gid = $gid AND realm = '$realm')";
      }
    }

    $grants_sql = '';
    if (count($grants)) {
      $grants_sql = 'AND ('. implode(' OR ', $grants) .')';
    }

for example in node.module in the node_access function.

Rather than creating these recursive and/ors couldn't we use the key columns? It looks like hook_node_access for D7 is a great improvement(let the module do it's own thing rather than mixing role ids and access module lists).

Our site only uses drupal core node access and node_privacy_byrole which uses the role id in the list that it returns, i wonder how many modules use their own list? This would tell us how much churn it would cause D6 to change this query generation, if it's to much I guess there will be a patch only for users that know they aren't using an access module with it's own lists.

jlporter’s picture

jlporter commented

The query show that only roughtly .4 million of 11 millions rows are storing anything useful. Couldn't this be modified so that if there is nothing returned from the query then assume 0 0 0?

mysql> select count(*) from node_privacy_byrole where grant_view=0 and grant_update=0 and grant_delete=0;
+----------+
| count(*) |
+----------+
| 11343447 |
+----------+
1 row in set (4.63 sec)

mysql> select count(*) from node_privacy_byrole;
+----------+
| count(*) |
+----------+
| 11772627 |
+----------+

jlporter’s picture

jlporter commented
Project: Node » node privacy byrole
Version: » 6.x-1.6

swinging back around to this issue, makes more sense for it to be in npbr queue

deekayen’s picture

deekayen commented

Yes, it makes more sense here in npbr and we should do it. Are you any closer to writing a patch?

bdwelle’s picture

bdwelle commented

We are having performance issues with this module as well. Any recent progress? Anyone out there that we could have take a look at our site/db and make some optimizations?

bdwelle’s picture

bdwelle commented

@jlporter - We have the same situation with our site; our node_privacy_byrole table has 12,177,945 rows and 11,898,741 of those rows (98%) have the default values in the grant_view, grant_update and grant_delete columns. The code actually does seem to assume default values if it doesn't find a matching row in the table, so the change that I would suggest would be to skip doing the INSERT if we're just inserting default values.

Due to performance issues we have been forced to experiment with removing all the rows in that table with default values... That reduced he size of the table from 12M rows to 200K rows.

Has anyone else experimented with removing all the rows in that table with default values?

deekayen’s picture

deekayen commented

I'm working on a patch today that would omit database entries where the permissions match the default unless a checkbox specifically specifies to lock in the current default permissions, regardless of future changes to the defaults.

ankur’s picture

ankur commented

I've encountered performance issues before when the node_access table gets to large. The way I usually deal with this is to log the slow queries and then do a MySQL explain on the slow queries that involve querying the node_access table.

The idea is to identify the columns/values being used to do the joins and to add indexes on either individual columns or combinations of columns.

Here's a useful article on analyzing slow queries w/ EXPLAIN and adding indexes: http://hackmysql.com/case4

deekayen’s picture

deekayen commented
Status: Active » Needs work

Here's a commit of what I did so far.

http://drupalcode.org/project/node_privacy_byrole.git/commit/9f87c95

The changes should now prevent the node_privacy_byrole table from getting entries with no grants. As far as I can tell, that is already how the node_access table works - no rows with no grants.

It also adds a cleaner to remove all rows from the node_privacy_byrole table that already have no grants. Then it examines each role on selected content types and deletes all node_privacy_byrole nid records which match the default role grants for that content type. This is intended to cause the node_privacy_byrole module to apply more defaults to nodes in the absence of specific, database-stored permissions.

To assist with ensuring nodes apply default permissions when checked by core node access functions, there is a new checkbox intended to apply default content type permissions per role to new and edited nodes. The node form defaults to using defaults. Unchecking the checkbox for defaults will let you lock-in specific node permissions by role. As long as they differ from the defaults, the cleaner shouldn't ever erase them, however if you intend to uncheck the default checkbox to set the defaults on a node regardless of how the content type level defaults might change in the future, the cleaner could potentially erase those static entries.

Improving the storage of permissions is really a matter of decision making at this point and having someone more than just me reviewing the code to make sure it does what is intended.

I'm marking it needs work because I want to implement hook_help for the cleanup page instead of the field description. I also think node_access might not hurt from getting a cleaning, too, which is currently not considered in the cleanup function in the aforementioned commit.

deekayen’s picture

deekayen commented

ymmv, but testing on one of my sites with 3,188,000 rows in node_privacy_byrole table reduced the count down to 159,485 with the cleaner code in 9f87c95. That makes fewer rows in the node_privacy_byrole table than in node_access. I'm not even sure if such a drastic decrease passes a basic sanity test or not.

deekayen’s picture

deekayen commented

Another thing I'm sensitive to in this issue is the ability in Drupal 7 to have explicit deny. The absence of a grant or the entry of a zero in the node_privacy_byrole table could be interpreted as a deny, whereas in D6 and older it means the absence of a grant, which is different.

I'm not even sure what to support, if at all, the ability to do an explicit deny going forward in D7 and how the data in a node_privacy_byrole table which doesn't get cleaned by this would interpret that. I don't have a good grasp right now on whether it would be fair to force a clean using this methodology for people upgrading from D6 to D7 so that zero entries can positively be interpreted as an explicit deny.

cyu’s picture

cyu commented

This is probably a non-issue, but previously if the page content type was available to authenticated and then you created a page that you wanted to be viewed only by authenticated you'd save with the defaults and get what you desired. If later the page content type had default permissions changed to include anonymous, the page that you saved with default permissions before would not change to include anonymous. After this patch, that same page would have no npbr entry and would inherit whatever content type permissions were configured.

Having the module work either way could be considered a feature, here you can see an example of people calling the current behavior a bug #364513: Views disappear. In any case, if that behavior changes it should at least be documented.