Deleting 100M content entities is slow.
1. Create 100M content entities on an entity with 100 fields
2. Delete all the content entities
3. Look at query log and see unoptimized deletion:
In SqlContentEntityStorage::doDeleteFieldItems we delete data from the base and revision tables in bulk, but we call ::deleteBulkFromDedicatedTables one at a time:
if ($this->revisionDataTable) {
$this->database->delete($this->revisionDataTable)
->condition($this->idKey, $ids, 'IN')
->execute();
}
foreach ($entities as $entity) {
$this->deleteFromDedicatedTables($entity);
}
That results in a ton of separate SQL queries that could be grouped into one.
It seems pretty straightforward to do, as deleteFromDedicatedTables() is specific to SqlContentEntityStorage.
Making this change resulted in the duration going from several days to hours.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | 2024-06-17_17-30.png | 113.22 KB | djdevin |
| 2024-06-17_17-08.png | 280.52 KB | djdevin |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
djdevinComment #3
djdevinComment #4
djdevinComment #6
djdevinComment #7
smustgrave commentedMR should be against 11.x as the latest development branch
Feels like a change that needs some kind of test coverage also.
Comment #8
djdevinRebased against 11.x (applies to 10.3.x cleanly as well)
deleteFromDedicatedTables() doesn't have any test coverage - since this isn't a bug I wonder if having coverage through SqlContentEntityStorageSchemaTest is enough?
Comment #10
vidorado commentedI've added a kernel test to ensure that a single delete query is executed against both the data table and the revision data table.
I'm not sure if directly testing the protected
deleteFromDedicatedTables()method is the best approach, for the sake of simplicity, or if I should have mocked even more components to test it only through the public interface ofSqlContentEntityStorage.Comment #11
smustgrave commentedLeft comments on MR. Thanks!
Comment #12
nicxvan commentedAny chance you can compare deleting that many entities with and without this change?
Also memory usage would be helpful.
Also 100M, is that 100 million?
Comment #13
vidorado commentedThanks for the review, @smustgrave! I've applied your suggestions.
@nicxvan: I haven't performed the test suggested in the IS, but I believe it refers to 100 million. In my opinion, that’s an extremely unlikely case. In most scenarios, there would be only a few bulk deletions, and the gain would be minimal, but I still think it’s worth doing things the right way when the fix is so simple.
Comment #14
djdevinIt was part of a multifaceted approach so I'm not sure exactly how much this was part of the gained performance. I used a Drupal queue and ran multiple instances of queue-process to delete a batch of 1000 entities at a time in parallel. A custom progress bar running periodically looked at the number of items at the start and the number of items left in the queue.
Didn't have significant issues with memory, I believe it was solely the efficiency loss from running thousands of delete queries instead of 1.
I was exaggerating a little bit and don't have hard benchmarks, but at 10M (million) it went from ~200 hours to 6 hours or so. They were also entities with ~200 fields (paragraphs) so there were a ton of single field delete queries which is where I identified the issue. So deleting a single entity cost 402 queries. Deleting 1000 of them cost 402,000 (2000 entity, revision, and 2*200*1000 field queries).
With the fix, deleting 1000 entities cost 2400 (1000*2 entity, revision and 200*2 fields and revisionqueries)
At 402,000 queries per batch of 1000, the issue is exacerbated if you are using something like AWS Aurora or MySQL with replication in a durable ACID configuration, and if the database isn't local to the network. Both contribute to delay when writing tons of data.
tl;dr: Bulk queries are recommended anyway.
Comment #16
nicxvan commentedwow!
Comment #17
nicxvan commentedI did a quick pass all a couple of comments.
One is addressed, one has not.
Test looks good and test only fails.
Needs work for the last comment.
Comment #18
vidorado commented@nicxvan, GitLab may have misleaded you (and me too, for a moment). In the MR overview it doesn't say that the code was changed, but it indeed was! :)
Comment #19
smustgrave commentedBelieve all feedback on this one has been addressed.
Comment #20
catchOverall this looks great and I couldn't find a single override or call to the method in contrib, however just in case we should add a change record here since it will be a hard break if someone is overriding or calling this method in custom code.
Comment #21
vidorado commentedDone! :)
Comment #22
nicxvan commentedI updated the CR for clarity, I agree this is RTBC again.
Comment #24
catchCommitted/pushed to 11.x, thanks!