Node access rebuilds should go newest to oldest

What about forcing a maintenance mode in that case? Changing from oldest to newest to newest to oldest feels like it's only solving the symptom without solving the underlying issue.

Totally agreeing with Webchick here, sometimes sites simply can not afford this. Having a database with 40K+ nodes this process will run for over 4 hours on the command line alone and thus having the first 500 nodes processed in the first 30 minutes means a great deal to the perception of stability of a website.

Processing newest nodes first would definitively be an improvement for most installations, however as Damien states it the real issue is that node access rebuild doesn't scale.

Just thinking out loud here. I see 2 obvious ways to help it scale better:

• Node access partial rebuilds: modules that are controlling access of only a subset of nodes have a way to indicate that only these nodes needs to be processed (I remember seeing an issue about that but I can't find it anymore)
• Node access incremental rebuilds: node access is not rebuilt in one pass, but incrementally when nodes are loaded / viewed. But it would make things more complicated when node data is retrieved without doing node_load, such as through Views or custom db queries with the node_access tag.

For me, this is a great improvement. I can't think of any downside, even for D7. Patch attached.

The desire for perfect should not block incremental improvement.

Committed to 8.x. Moving to 7.x.

Just saw this in the issue queue so sorry for the late response...

For large sites with lots of content, as a short term fix, wouldn't it make sense to sort on a) promote column and b) status column before the c) nid column? I do not foresee this being called that often, but several sites I am aware of do not necessarily have most recent nodes on the front page, which I would think should be first thing available.

That also makes sense.

Moving to 'needs work' as I committed the patch in #5.

Looking at the implemntation in D7 I think the main issue is the delete at the start of node_access_rebuild:

 db_delete('node_access')->execute();

I haven't tested this thoroughly but wouldn't it make more sense to remove entries for nodes from the node_access table incrementally?

Something like this

  foreach ($nids as $nid) {
    db_delete('node_access')
      ->condition('nid', $nid)
      ->execute();
    $node = node_load($nid, NULL, TRUE);
    // To preserve database integrity, only acquire grants if the node
    // loads successfully.
    if (!empty($node)) {
      node_access_acquire_grants($node);
    }
}

I guess it's not complete because you still have the issue of modules storing data in other tables.

I see @Dries' comment in #7 to backport to 7.x, but the version on this issue queue was not updated.

Also, weighing in to say: I like the thinking behind deletes as we go (@miiimooo in #11) and great, harmless but likely effective considerations of more creative sorting (@Lars #8), but the patch as applied to 8 was already the simple sort. But along the lines of "let's not block progress by getting caught in the weeds" (@agentrickard in #6), let's try different stories for each:
#2292113: Improve node access rebuild immediate effect by further improving the sort critera
#2292117: Improve node access rebuild by deleting record-by-record instead of the delete-all at the beginning each time it's run
and move forward with commit of this current improvement to 7.x-dev.

For the core issue and originally committed patch to review, see @moshe's comment in #5.

EDIT:
Oh wow. And after all that, I still appear to see that merely changing the sort of the select did not affect the order of creation of records: Why is that? Here's what I see on 30+k nodes site:
drush sqlq "select max(nid) from node_access"
shows numbers in the 100s and then low 1000s after a little while. It should be just the opposite, and in fact, it should always return the max nid in the node table. :( Any ideas what's going on?
EDIT 2:
Ah. It's because when running node_access_rebuild() from the browser, you're in batch mode. When executed directly from code, like via drush, only then do you get the specific select that's been ordered DESC. Cool.

And so, to be clear, Moshe's fix in #5 works great for directly executed node_access_rebuild() but is wholly ineffective against batch (browser-clicked Rebuild). The batch bit is a bit more spidery: not just a matter of sorting in one place bc of the way it monitors progress and progress completion with a ++ and a max(nid) I think.

Considering that this went into D8 almost two years ago, I'd agree that #5 should be backported into D7 sooner rather than later despite the stagnant discussion surrounding additional performance optimization in D8.

I'm setting the status as Patch (to be ported) so that it will hopefully have higher visibility for any novices looking for easy things to port.

The issue should probably be set back to active for D8 after the patch is ported.

Updated patch for D7 to rebuild access for new content first.
Please review.

16: D7-order-node-access.patch queued for re-testing.

This is the exact same change that went into D8. After applying, rebuilding node access permissions still worked on my D7 sites. RTBC.

Committed to 7.x - thanks!

And back to Drupal 8 to fix it for the batch situation also.

As far as I can tell, actually, the code in Drupal 8 was already changed to try to make this work at some point, but wasn't done correctly... see #2331113: Node access rebuilds are completely broken when being rebuilt through a batch process?. That may need to be resolved before proceeding here.

I just gave this a try... I liked the incremental approach from #11 and i figured out that \Drupal::entityManager()->getAccessControlHandler('node')->writeGrants(); already provides a delete parameter, so that old records of a node are deleted before saving new ones.
In addition to that I extended the following items to allow easy deletion of the default node grant records:

• NodeGrantDatabaseStorageInterface
• NodeGrantDatabaseStorage
• NodeAccessControlHandler
• NodeAccessControlHandlerInterface

... and of course, I preserved the node order change (nid DESC) for the batch processing.

Attached you can find a patch with all these changes. Please have a look and tell me what you think.

Damn... I forgot to execute the deletion of the default grant! Forget about the patch in #35 - here is a new patch. Sorry for the failure ;)

+++ b/core/modules/node/node.module
@@ -1302,7 +1304,7 @@ function _node_access_rebuild_batch_operation(&$context) {
     ->condition('nid', $context['sandbox']['current_node'], '>')
-    ->sort('nid', 'ASC')
+    ->sort('nid', 'DESC')

This won't fly and desperately needs a test to not regress again:

You test for current_node >, but sort DESC ...

See #2331113: Node access rebuilds are completely broken when being rebuilt through a batch process? for more information.

Thanks for the patch!

Whoops... of course you are right. Attached is a new patch and an interdiff. This fixes the sorting/selection problem, but I was not able to create a full test for node_access_rebuild() - perhaps someone else is more into this and may help out on that. As far as I see, only the process of clicking "Rebuild permissions" is covered with a test right now, or did I miss something?

Re-roll. Patch applies cleanly at commit f07bcff .

Installs fine in SimplyTest.me - there's a big error there, but thats been there for maybe a week now.

I am removing the Novice tag from this issue. It looks like it was added in comment #15 for backporting to D7. It looks like that was completed on comment #31.

I’m using this documentation as a source: https://www.drupal.org/core-mentoring/novice-tasks#avoid

I'm also removing the 'needs backport to D7' tag as it looks like that is completed.

I'm not sure what's remaining on this issue. @mgifford, looks like you said there's a big error? Could you give more information on that. I think we should update the issue summary.

Think we might backport the batch fix to Drupal 7 too (depending on exactly what it looks like in the end).

This is a challenging issue to triage.

This was committed to 8.x, then moved to D7 and committed with a different commit message, then moved back to D8 for 'the batch situation'. So, what to do?

I am going to open a new issue for the batch work. I'll upload the latest patch to that issue and link to this issue so anyone can read the comments. And add credit too. For this issue, I'm restoring the meta data to the time of the D8 commit and setting the status to fixed.

New issue created to look into the batch part of this, #3270646: Node access rebuilds should go newest to oldest (fix for rebuilds that are done via a batch).