Add upgrade path to clean up deleted revisions from node_field_revision

Thanks. Bumping this to major and bug report since it's at least possible that entity queries with allRevisions() will return invalid results.

Here's a first patch that does this.

Tested this locally with a node with some deleted revisions that also had paragraphs, so two entity types with revisions. I also set the batch size to 1 to test that this works.

Downside is that could be fairly slow on a site with a lot of nodes, I'll test it soon on a big site.

wrong core version.

This is a bit interesting, actually, wondering how we're going to add this to 8.3 and 8.4, due to the different update function number, so most/all sites would end up running it twice? We could add it as the same 83xx update in 8.4 as well maybe?

And here's a basic test.

This should fix that test fail.

Did test the update function an site with 600k nodes. Worked fine but only found a handful of rows to delete, clearly deleting revisions is not done often on that site.

The query is pretty slow, took about 2.4s, so if you have a look of deleted revisions on a site, this might take a considerable amount of time. Without the order, it was about 0.9s, with just the sort on nid, it was about 1.1s. Which is actually enough for what's needed here (optimize the deletions. It might also be possible to write a delete query which uses a set of OR conditions but not sure if that's worth the complexity.

+++ b/core/modules/system/system.install
@@ -1927,3 +1927,68 @@ function system_update_8400(&$sandbox) {
+    $sandbox['current'] = 0;
...
+  $entity_type_id = $sandbox['entity_type_ids'][$sandbox['current']];
...
+    $sandbox['current']++;

Using a number to reference to an entity type is a bit weird, to say the least. Maybe renaming 'current' to something more descriptive could help.

This is exactly the same logic as in system_update_8400(). Agreed that it's a bit weird, but it's a list of entity type ids with keys and this is the key. We could do something like current_key. But I think it's a pretty nice way of looping through that list I think.

Thanks for the reviews. Cleaned that up and added some more comments.

Testbot, I'm so very sorry.

#NotMyDay ;)

It works for most tests as they only have one matching entity type, so we're finished after one round anyway. But yeah, obviously it is going to cause another endless loop on at least some tests.

1. Yeah, all those properties and the trait was copied, not needed indeed.
2. 9 is actually not the default revision, ther's also 10. I've added asserts for 10 now as well, which also has translations.
3. Not exactly sure what the to change, this is basically exactly the same pattern as in the update function above, I added some more comments for now.

That fail was unrelated.

This is getting embarassing, thanks for all the reviews.

Fixed 1 and 2, I'm not sure that your suggestion for 3 is simpler, I kind of like the additional structure of my approach and it is the same as the one from the update function above.

I also talked to @alexpott and he said we only need to add this to 8.4.x and not backport to 8.3.x unless we have reports about those inconsistencies breaking sites.

1. +++ b/core/modules/system/src/Tests/Entity/Update/SqlContentEntityStorageRevisionDataCleanupTest.php
   @@ -0,0 +1,49 @@
   +    $result = $connection->query('SELECT nid, vid FROM {node_field_revision} WHERE nid = :nid', ['nid' => 8])->fetchAll();
   +    $result = $connection->query('SELECT nid, vid FROM {node_field_revision} WHERE nid = :nid AND vid = :vid', ['nid' => 8, 'vid' => 8])->fetchAll();
   +    $this->assertEqual($result, [(object) ['nid' => '8', 'vid' => '8']]);
   

   Only one of these is actually being asserted on.

2. +++ b/core/modules/system/system.install
   @@ -1927,3 +1927,81 @@ function system_update_8400(&$sandbox) {
   +        return $entity_type->getKey('revision') && $entity_type->getRevisionDataTable() && $entity_type->getRevisionTable();
   

   Not sure this is correct due to the fallback we do in SqlContentEntityStorage. I would think that entity types that do not specify a revision data table but are revisionable and translatable still suffer from this.

3. +++ b/core/modules/system/system.install
   @@ -1927,3 +1927,81 @@ function system_update_8400(&$sandbox) {
   +  // Get the current entiy type. Fetch 100 records that should be deleted. Order
   ...
   +    ->range(0, 100)
   

   entiy -> entity

   Also, shouldn't this use $settings['entity_batch_updates'] instead of hardcoding 100?

4. +++ b/core/modules/system/system.install
   @@ -1927,3 +1927,81 @@ function system_update_8400(&$sandbox) {
   +      ->condition($id_field, $id)
   

   Why do we need the ID? Shouldn't revision IDs be unique regardless of IDs?

Well, as I said in #30, embarassing ;)

1. The first line was a copy & paste error, removed.
2. I was wondering about the falback, yes. I rewrote it a bit looking for revisionable and translatable but also making sure that those tables really exist in case your site is in a weird state.
3. .. entity is the hardest word...
4. Somehow that fact doesn't want to stay in my brain. Of course and that also means we can considerably simplify the update function.

Especially given 4, I'm not sure we should respect entity_update_batch_size. We are technically not doing entity updates but just raw delete queries and now only a single delete query for those 100 records. I'm even wondering if we want to increase that amount further now to 200 or so. I suppose we could use the setting with a different default but new sites have it by default in settings.php. Thoughts?

Now I'm bold and I'll upload the patch without re-running the tests locally, lets see how that goes :)

I knew that was a mistake.

+++ b/core/modules/system/system.install
@@ -1943,14 +1943,27 @@ function system_update_8401(&$sandbox) {
+      return $database->schema()->tableExists($revision_table) && $database->schema()->tableExists($revision_data_table));

Nice!!!

Yeah you're right regarding entity_update_batch_size. Not sure if we should increase it to 200, but as you already point out above, while a lot of sites may have thousands or millions of revisions, I don't think many sites will have thousands or millions of deleted revisions. So I don't think it's a big deal either way.

Thanks!!

Random fail?

1. Did test the update function an site with 600k nodes. Worked fine but only found a handful of rows to delete, clearly deleting revisions is not done often on that site.

   :O \o/ Thanks!

2. Patch looks great.
3. Updated IS.
4. I wonder if this needs a CR, because for some people, this could mean a huge drop in database size (or at least table size), which can be … pretty scary?

I don't think this needs a CR, the table size is going to be significantly larger than the data tables after the update which is the main thing.

I thought about test coverage with a non-node entity type, but I think it's reasonable to test just with node here.

Committed 9a16601 and pushed to 8.4.x. Thanks!

This is failing on postgres (or it did at least once), so I've reverted it:
https://www.drupal.org/pift-ci-job/693265

Also, I had to go back and find and check on https://www.drupal.org/pift-ci-job/675973 as well. It's not the same but it might have been and would have meant a different way of approaching the fail. :) (@Berdir, when there are suspected random fails, please link the failing result and check #2829040: [meta] Known intermittent, random, and environment-specific test failures.)

In general, we really should test things that touch storage tables against all databases before commit. I'll queue a postgres test.

Thanks!

Also, should this issue be critical?

Had to reroll the patch again 8.5.x and it is green fails on PHP 7 & PostgreSQL 9.1

Entity.Drupal\system\Tests\Entity\Update\SqlContentEntityStorageRevisionDataCleanupTest
✗	
testRevisionDataCleanup
fail: [Other] Line 45 of core/modules/system/src/Tests/Entity/Update/SqlContentEntityStorageRevisionDataCleanupTest.php:
Value array (
  0 => 
  stdClass::__set_state(array(
     'nid' => '8',
     'vid' => '10',
     'langcode' => 'en',
  )),
  1 => 
  stdClass::__set_state(array(
     'nid' => '8',
     'vid' => '10',
     'langcode' => 'es',
  )),
) is equal to value array (
  0 => 
  stdClass::__set_state(array(
     'nid' => '8',
     'vid' => '10',
     'langcode' => 'es',
  )),
  1 => 
  stdClass::__set_state(array(
     'nid' => '8',
     'vid' => '10',
     'langcode' => 'en',
  )),
).
✓		- runUpdates

1. +++ b/core/modules/system/src/Tests/Entity/Update/SqlContentEntityStorageRevisionDataCleanupTest.php
   @@ -0,0 +1,48 @@
   +   * Tests that stale rows in the revision data table are deleted.
   +   */
   +  public function testRevisionDataCleanup() {
   

   We could put a @see system_update_8404() in this method's doc block.

2. +++ b/core/modules/system/system.install
   @@ -2042,3 +2042,86 @@ function system_update_8403() {
   +      if (!$entity_type->getKey('revision') || !$entity_type->isRevisionable() || !$entity_type->isTranslatable()) {
   

   The first check here for getKey() seem unnecessary because isRevisionable() does the same check internally.

3. +++ b/core/modules/system/system.install
   @@ -2042,3 +2042,86 @@ function system_update_8403() {
   +  // Get the current entity type. Fetch 100 records that should be deleted. Order
   

   Nit: over 80 chars.

4. +++ b/core/modules/system/system.install
   @@ -2042,3 +2042,86 @@ function system_update_8403() {
   +  // If there are less than 100 rows, the entity type was cleaned up, move on
   +  // to the next one.
   

   Another nit: It looks like "to" can be moved above without exceeding the 80 chars limit.

5. +++ b/core/modules/system/tests/fixtures/update/drupal-8.entity-revision-data-cleanup-2869568.php
   @@ -0,0 +1,32 @@
   + * the upgrade path of https://www.drupal.org/node/2869568.
   

   I think we should mention the actual name of the update function here (system_update_8404()) instead of a d.o link.

Perfect! :)

Since it's a batched update, do we want to add 101 test rows to the data for the update test to double check the batching works?

Removed irrelevant comments.

> Please do the needful.

From https://en.wiktionary.org/wiki/needful:

Commonly found in phrases such as "please do the needful", which occur commonly in Indian English but are held as archaic in other dialects. Global interactions between English speakers have to some extent led to these phrases being seen as stereotypical of Indian English and parodied by speakers of other dialects.

I'd suggest to avoid that sentence, it sounds very demanding and "not nice" to me.

I see how it would be a problem for you, but this is a patch that was not committed, so not sure why you even noticed this, there isn't really a reason to apply this to a site?

Kindly excuse me. I have removed my comments, will post it in relevant issue.

Just updating the patch.

Added another 100 rows to the test data (#53) and more assertions on the data.

Applied patch #69
Ran updb and that ran without issue.

Tests all green and was previously RTBC #52 so restoring that.

Changes in #69 look good, committed/pushed to 11.x, thanks!

Small follow-up. The test added here doesn't run #3394186: SqlContentEntityStorageRevisionDataCleanupTest does not run.