Optimize node access query building

rolled into patch, upped version, setting to needs review, didn't add limit in to this patch.

interesting. your probably need a very large node access table for this to matter. groups.drupal.org has pretty fast node access queries so there isn't much room to improve. just one example though.

No longer applies.

Free reroll courtesy of patch bingo.

good idea, but a bit late now ... benchmarks might help people get more interested, though I'm not sure that is a prerequisite.

good idea, but a bit late now.

Re-roll to remove some offset. I'm not set up for benchmarks, yet, but agree that'd be nice here.

Patch still apply.

If you tell me what real-world conditions need to apply for this to matter, I can try creating them with the devel generator. I'd stress test this with a large database and see if it makes an appreciable difference.

I very much doubt this will be committed without some benchmarks to prove it is worthwhile (or even that it doesn't slow it down).

See: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476

Subscribing

just a reroll to the latest version and a fix of codestyle

does anyone know how to test this intelligent

We should probably have a test_node_grants module, set some grants and ensure that users have the appropriate viewing rights. Maybe two modules to handle the interactions. I don't think this patch should go in without a test like that. Additionally, this query needs converting to dbtng.

1. just want to point out the above patch is not sufficient. You will need to make similar changes to "_node_access_where_sql" function below as well as it overrides for modules using hook_dbrewrite_sql.

2. Another thought
instead of

$grants[] = "((realm = '$realm') AND (gid = ".implode(' OR gid = ', $gids)."))";

This may be better perhaps? (but I can not prove with data.)

$grants[] = "(realm = '$realm' AND gid in (".implode(' , ', $gids)."))";

I tried to benchmark all 3 conditions (A. WIthout the patch B. With the patch C. With "In" clause instead of OR clause as above.) Unfortunately my data is not conclusive. For all first invocations I would get 3 digits (190 millisecond) query time and with subsequent invocation it would be in in single digits(3 milliseconds). SO I am thinking my data/environment is not pristine. Also the difference between all 3 tests (ABC) was not significant.

Ok here is my second try with a clean environment with mysql query cache turned off.

I tried patch but as used "IN" clause from my comments above, For tracker there is slight improvement
before patch

Executed 232 queries in 510.51 milliseconds. Page execution time was 750.65 ms.
Executed 232 queries in 510.38 milliseconds. Page execution time was 750.31 ms.
Executed 232 queries in 509.5 milliseconds. Page execution time was 749.92 ms.

After patch

Executed 232 queries in 502.94 milliseconds. Page execution time was 743.57 ms.
Executed 232 queries in 501.72 milliseconds. Page execution time was 738.95 ms.
Executed 232 queries in 501.93 milliseconds. Page execution time was 742.45 ms.

However for a normal node page the difference is less obvious. The page has a related content view block.
before patch

Executed 265 queries in 105.15 milliseconds.  Page execution time was 597 ms.
Executed 265 queries in 104.33 milliseconds. Page execution time was 595.01 ms.
Executed 265 queries in 104.86 milliseconds. Page execution time was 595.47 ms.

After patch

Executed 265 queries in 104.54 milliseconds. Page execution time was 587.45 ms.
Executed 265 queries in 111.47 milliseconds. Page execution time was 593.68 ms.
Executed 265 queries in 111.47 milliseconds. Page execution time was 593.68 ms

where exactly (how) you would add the LIMIT clause? I could not find it in any of the code above other than your comment? can you write some code? Also if we add LIMIT here, whats happens to the limit added by query , say pager query?

But there is no Count(*) in this particular patch. So if you need to change the query, which query need to change? I just tested the patch which dynamically adds the criteria for an existing query.
Do you mean something like

$grants[] = "((realm = '$realm') AND (gid = ".implode(' OR gid = ', $gids).")) limit 1";

But then how you will determine where the preceding query uses select fieldname1, fieldname 2 or select count(*).

Your idea may be correct but I am not understanding implementation details for all use cases. Could you please submit exact patch?

It looks like this issue hasn't been touched in quite a while. My patch is essentially a re-roll with DBTNG in the mix. I ran into this issue with a client running tac_lite with a large number of terms. I want to submit this patch for 7.x for d.o testing (along with the client testing this), then push into 8.x afterwards.

Bumping the priority because this change has a massive improvement on any site using node access with a large number of grants.

This makes sense, but it should go into D8 first.

EDIT: I see you said that in #23 :)

#23: drupal-optimize_node_access_queries-106721-23.patch queued for re-testing.

Rolled for 8.x and made the same change to a fourth query.

Derp. Guess I should run more of the test suite and/or wait til I'm sober and rested to write patches.

Or maybe that's not all my problems? :P

So, I had planned to test this on other database drivers (just in case) but it looks like that's not going to happen because of issues like #1799310: WebTestBase->setUp() broken for sqlite.

#32: 106721-optimize-node-access-query-building-30.patch queued for re-testing.

#32: 106721-optimize-node-access-query-building-30.patch queued for re-testing.

#32: 106721-optimize-node-access-query-building-30.patch queued for re-testing.

rerolled the patch in #32

#41: 106721-optimize-node-access-query-building-41.patch queued for re-testing.

#41: 106721-optimize-node-access-query-building-41.patch queued for re-testing.

This one needs a re-roll.
@see http://drupal.org/patch/reroll

I think, that the patch doesn't need to be re-roll anymore.
The node access queries were replaced by checkAllGrants() method in NodeAccessController.
I think that this issue should be closed, but I suggest somebody else to check again.

So let's put it in D7 then?

I believe my patch was the last for D7. We can re-start over from here.

You should check whether the $gids array contains any items before using it in an IN query.
Otherwise, PDO will throw exceptions whenever $gids is empty.

Issue summary updated

Looks good now

52: drupal-optimize_node_access_queries-106721-52.patch queued for re-testing.

Still needs to go in Drupal 8 first (code is in core/modules/node/lib/Drupal/node/NodeGrantDatabaseStorage.php).

Here's the d8 version.

Here's a new version for D8.

I ran into this on a D7 site with a many node grants, which resulted in severael seconds worth of DatabaseCondition::compile(). This approach doesn't really fix the query much (I think mysql's parser will figure that out just fine), but it helps a ton in assembling the query.

I also extracted this logic into it's own static helper method, since we repeat it three times. I made it static because it's a pure function and it's possible that contrib will need it (views would in D7).

And here's a D7 version.

D7 looks good, dunno why 8 fails

Think I see what happened.

1. +++ b/core/modules/node/lib/Drupal/node/NodeGrantDatabaseStorage.php
   @@ -264,4 +243,19 @@ public function deleteNodeRecords(array $nids) {
    
   +  static function addGrantsToQuery($node_access_grants) {
   +    $grants = new Condition("OR");
   +    $grants = $query->orConditionGroup();
   +    foreach ($node_access_grants as $realm => $gids) {
   +      if (!empty($gids)) {
   +        $grants->condition(db_and()
   

   I know you don't like you write documentation ;), but this will need a proper docblock and $node_access_grants should be type hinted as an array.

Fair enough.

Also reuploading the D7 version, the one I posted earlier was bad.

Nitpick review.

1. +++ b/core/modules/node/lib/Drupal/node/NodeGrantDatabaseStorage.php
   @@ -264,4 +243,26 @@ public function deleteNodeRecords(array $nids) {
   +   * Helper function to add node access grants to a query.
   ...
   +  static function addGrantsToQuery(array $node_access_grants) {
   

   This doesn't add anything, it *builds* an object. So what about buildGrantsCondition()?

   Furthermore, all function/method docblocks should begin with a verb AFAIK.

2. +++ b/core/modules/node/lib/Drupal/node/NodeGrantDatabaseStorage.php
   @@ -264,4 +243,26 @@ public function deleteNodeRecords(array $nids) {
   +   * @param $node_access_grants
   

   Missing the typehint.

3. +++ b/core/modules/node/lib/Drupal/node/NodeGrantDatabaseStorage.php
   @@ -264,4 +243,26 @@ public function deleteNodeRecords(array $nids) {
   +   *   An array of grants, matching the return value of node_access_grants.
   ...
   +   *   A condition object to be passed to $query->condition.
   

   s/node_access_grants/node_access_grants()/
   s/$query->condition/$query->condition()/

   Without the parentheses, it looks like you're talking about a DB table/variable/concept and an object property, respectively, rather than functions.

   Also, maybe:

   s/matching the return value of/as returned by/

   ?

Good call on the method name. I originally had it take the query as an arg and add the condition, but didnt update the name when I changed it.

Everything else in #68 should be addressed.

+++ b/core/modules/node/lib/Drupal/node/NodeGrantDatabaseStorage.php
@@ -264,4 +243,28 @@ public function deleteNodeRecords(array $nids) {
+        $grants->condition(db_and()

Another reason I dislike static methods: You can't inject the connection here properly.

At minimum, we should use \Drupal or Database:: here rather than the function.

Although I'd prefer to just make it a normal method and inject the connection. I don't know if AND/OR handling varies by database, but in practice I think we have it setup such that it could.

db_and is an inconsistency considering that i used the class version above it, so I'm fixing that.

The rest I disagree with because no connection should be involved here.

Here's a re-roll of #59 (D7) that fixes some issues applying (contains a /docroot path and some changes to the 7.x contrib Views module), so that we can include this improvement in Commons 7.x.

Looks good to me.

(Same code for a while now, so I can keep repeating this :) )

I will compromise on new Condition().

Related #1349080: node_access filters out accessible nodes when node is left joined

sorry

killes did some benchmarks of this approach for a 6.x site but I can't find them on this issue. iirc the results were it made some queries worse, some better, and was neutral for others. That sounds somewhat similar to Mark's findings as well.

I think this is worth it for less time spent in PHP and we can call the query changes neutral, so going ahead and committing, but retitling so we don't make any exciting claims.

It wasn't 100% clear to me whether this was indeed committed & pushed, but it is: http://drupalcode.org/project/drupal.git/commit/d1d797046ffe79faf45d0aa0.... Yay :)

The patch in #72 needs to be rerolled so testbot can find it. If it passes, that's also RTBC IMO.

Rerolled

Thanks, Erik!

+++ b/modules/node/node.module
@@ -3075,6 +3069,20 @@ function node_access($op, $node, $account = NULL) {
+function node_add_node_grants_to_query($node_access_grants) {

Missing docs!

81: drupal-106721-optimize_node_access_queries-81.patch queued for re-testing.

Back to rtbc. Bad bot!

Will it blend?

Closed #2326095: Optimize queries with node grants. as a duplicate.

Sorry, I still had no chance to test this - but it looks good.
A test I ran with a smaller version of this patch reduced the memory load from ~828MB to ~225MB on a particular site, but this was an extreme situation with I think ~500 Organic groups on one user.

@catch (#78)

iirc the results were it made some queries worse, some better, and was neutral for others.

I could imagine that smaller queries with only one or a few gids are better off with OR instead of IN.
If we care about this, we could add a simple if() to distinguish these cases.
Also, I wonder if the "gid IN (..)" should not rather happen AFTER the "realm = .." check, since I imagine the former to be more expensive.
On the other hand, the "gid IN (..)" is more likely to evaluate to FALSE, making it more likely that MySQL can skip the "realm = ...".

  foreach ($node_access_grants as $realm => $gids) {
    if (empty($gids)) {
      continue;
    }
    elseif (count($gids) <= THRESHOLD) {
      foreach ($gids as $gid) {
        $grants->condition(db_and()
          // gid first
          ->condition('gid', $gid)
          ->condition('realm', $realm)
        );
      }
    }
    else {
      $grants->condition(db_and()
        // realm first
        ->condition('realm', $realm)
        ->condition('gid', $gids, 'IN')
      );
    }
  }

We would have to find the ideal number for THRESHOLD. Maybe it is just 1.

This being said: Since this is already committed to D8, we might go ahead as planned and do this fine-tuning in a follow-up.

Bad bot.

I rolled 81 against 7.32. Here is the patch. I suspect many people will like to use this. Please double check my work!

Sorry :-/

Do not use patch in 105, it breaks node access checks.

Looking at #81:

@@ -3402,17 +3403,7 @@ function _node_query_node_access_alter($query, $type) {
       $subquery = db_select('node_access', 'na')
        ->fields('na', array('nid'));
 
-      $grant_conditions = db_or();
-      // If any grant exists for the specified user, then user has access
-      // to the node for the specified operation.
-      foreach ($grants as $realm => $gids) {
-        foreach ($gids as $gid) {
-          $grant_conditions->condition(db_and()
-            ->condition('na.gid', $gid)
-            ->condition('na.realm', $realm)
-          );
-        }
-      }
+      $grant_conditions = node_add_node_grants_to_query(node_access_grants($op, $account));

1. Why is this calling node_access_grants() again rather than using the existing $grants variable? This can potentially result in a significant number of extra calls to node_access_grants(), which invokes hooks, etc.
2. node_add_node_grants_to_query() will not use the 'na' table alias that the previous code did. Is that safe, given that the query which is being altered by this function could essentially be any arbitrary query?

(#1 looks like a D7-only issue, but I think #2 might apply to D8 as well.)

Also, #83 points out the node_add_node_grants_to_query() function needs PHPDoc.... (If that were the only issue I would have fixed it myself on commit or what not, but given the other issues we should take care of this one too.)

Reroll addressing above points

o.O

#15 {main}
Uncaught exception thrown in shutdown function.

PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupaltestbotmysql.simpletest792722cache_block' doesn't exist: DELETE FROM {cache_block}
WHERE (expire <> :db_condition_placeholder_0) AND (expire < :db_condition_placeholder_1) ; Array
(
[:db_condition_placeholder_0] => 0
[:db_condition_placeholder_1] => 1425667806
)
in cache_clear_all() (line 167 of /var/lib/drupaltestbot/sites/default/files/checkout/includes/cache.inc).

---

Uncaught exception thrown in shutdown function.

PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupaltestbotmysql.simpletest792722cache_bootstrap' doesn't exist: DELETE FROM {cache_bootstrap}
WHERE (cid = :db_condition_placeholder_0) ; Array
(
[:db_condition_placeholder_0] => variables

oops, forgot to default new arg for table alias

Need a version that will apply with #1349080: node_access filters out accessible nodes when node is left joined

The changes that need to be done should work fine without #1349080: node_access filters out accessible nodes when node is left joined, so sending this in for testing also (e.g. did not add do-not-test)

Or maybe it does conflict

Sorry for the noise, trying to get this back to needs review

Again, sorry the noise but had an oops above of course

After looking into further, the part that conflicted with this patch was added after a working patch #1349080: node_access filters out accessible nodes when node is left joined to fix an conflict between two modules. It sounds like reason it was added was to fix a bug that that conflict produced, so instead for openatrium went with patch 195 for #1349080: node_access filters out accessible nodes when node is left joined and 115 for this issue.

Patch 115 on this issue applies cleanly and appears to fix the issue. Can we get more reviewers?

Looks fine except:

+++ b/modules/node/node.module
@@ -3083,6 +3077,34 @@ function node_access($op, $node, $account = NULL) {
+ *   The grants to add to the query, usually gotten via: ¶

Trailing whitespace.

I'm not php specialist, but the code for :

function node_add_node_grants_to_query($node_access_grants, $table_alias = '') {
  $grants = db_or();
  $prefix = $table_alias ? $table_alias . '.' : '';

should read :

function node_add_node_grants_to_query($node_access_grants, $table_alias = NULL) {
  $grants = db_or();
  $prefix = $table_alias ? $table_alias . '.' : '';

or :

function node_add_node_grants_to_query($node_access_grants, $table_alias = '') {
  $grants = db_or();
  $prefix = !empty($table_alias) ? $table_alias . '.' : '';

or am I missing something ?

Nope, '' is false if you if around.

For further info, see: http://php.net/manual/en/language.types.boolean.php#language.types.boole...

Ok, thanks, and sorry for the noise...

No problem.

This is identical to #115 with the trailing white space mentioned in #126 fixed.

Anything holding this back?

This fixes performance issue for large sites which live on grants. Can we get more reviewers to review and close this ASAP?

So far so good for me, I just need to apply it on a live project and run its tests to really say it's RTBC but I will need some days maybe weeks to confirm.

I hesitate to discourage additional reviews with performance testing, but I would like to point out that the basic patch was already RTBC and was rejected based on a code review, not because it didn't fix the problem. If someone can verify that:

1. This is the functionally the same fix that went into D8.
2. The issues in #109 have been addressed.

...then I'd say it could go back to RTBC.

From my brief perspective, I'd have to say that the issues have somewhat been addressed. Yes, a docblock was written, but it looks like it was completely written from scratch. This function had a docblock in D8. I think we should re-use it instead, but I don't care so much that I'll set this back to NW for that. @David_Rothstein could change it on commit, if he wanted.

Anything holding this back to be committed in 7.x? Looking at the issue I see some of my earlier comments going as back as 8 years ago. :)

Someone needs to set it to RTBC to get it on a committer's radar, I guess.

@Pounard Is there any chance you tried the patch, as you mentioned? I am in the middle of a migration to 7x for a large install, so can't try till it is completely migrated.

subscribing***EDIT*** Followup soon.

@joseph.olstad: It's no longer necessary to add a new comment to subscribe to an issue. Simply click on the Follow link at the top-right.

RTBC #133

FYI: patch #81 is in use by over 600 acquia commons distribution installs.

Remaining tasks: commit patch #133

RTBC patch 133

FYI: patch #81 is in use by over 600 acquia commons distribution installs.

Remaining tasks: commit patch #133

Bumping to 7.70. This didn't make it into 7.60

Need for speed, this issue needs to go in, is blocking #2204257: Update Views Content access filter per core performance improvements

Patch #133 seems to have problems with the PHP 5.3 and MySQL 8 tests.

We'll need to address those before we can consider committing this.

@mcdruid , all those tests came out green, please refresh this page.
Please have another look at the test results.

back to RTBC
patch #133 is testing all green on all tests after tests were re-triggered. Testbot issue on previous runs is flushed out.

Yup, you're right @joseph.olstad - the test fails were unrelated and they both passed when re-tested.

The patch looks very similar to what's now in 9.2.x here:

https://git.drupalcode.org/project/drupal/-/blob/9.2.0-alpha1/core/modul...

I might tweak the docblock on commit to match the above in order to avoid the "North American past participle of get" :) but then Drupal does use American English...

Anyway, I think the patch looks good, and I'll ask Fabianx for final review in the next few days.

RTBC + 1 -- patch is the same as D9

IMHO it fixes a bug when an empty array is passed to IN() -- it might be that this somehow leads to that condition not being added, because I would have expected a SQL error in that case.

Anyway, this is good to go and I see no harm to match D8/D9/D10 behavior here.

Let's get this in after 14 years ;).

If you're feeling in the node_access mood, also checkout #3176634: [D7] node_access filters out accessible nodes when node is left joined.

Please could you use early return on node_add_node_grants_to_query?

From

foreach ($node_access_grants as $realm => $gids) {
  if (!empty($gids)) {
    $grants->condition(db_and()
      ->condition($prefix . 'gid', $gids, 'IN')
      ->condition($prefix . 'realm', $realm)
    );
  }
}

To:

foreach ($node_access_grants as $realm => $gids) {

  if (empty($gids)) {
    continue;
  } 

  $grants->condition(db_and()
    ->condition($prefix . 'gid', $gids, 'IN')
    ->condition($prefix . 'realm', $realm)
  );
}

Follow the new patch using early return

@RenatoG thanks, but we'll stick with #133 as that's very close to what's in D9 (see #166).

If you feel strongly that this can be improved, you should submit an issue for D9 per the backport policy.

@RenatoG if you do open an issue for D9 please put a link here so we can follow. Thanks.

Makes sense, let's do this. Thank you people

Opened at the 9.x version

ok, is this pending commit? Thanks

Yes, #133 should be committed before the next release.

Thanks, just wasn't sure if it needed the Pending commit tag.

Thank you everybody!

Would be cool to see a performance benchmark of 7.0 vs 7.81
Nice milestone here!