Optimise `wp_delete_comment` as called from `wp_delete_post`

[peterwilsoncc]

Remove the need for reparenting comments in wp_delete_comment by deleting comments in descending order.

Potentially accept an array of comments.

[Mte90]

Looking right now in the code I didn't see forcing of an order on SQL query, maybe in the meantime this issue was fixed?

[peterwilsoncc]

Replying to Mte90:

Looking right now in the code I didn't see forcing of an order on SQL query, maybe in the meantime this issue was fixed?

This is correct, no order is specified in wp_delete_posts (​see wp-includes/post.php#L2490). In most cases the DB defaults to insertion order, in this case ascending by date.

[Mte90]

So what is the purpose of the ticket, is not so much clear right now for me :-)

[johnbillion]

@peterwilsoncc Can you clarify the exact optimisation you'd like to make here? Is it the order in which wp_delete_comment() gets called or is it optimisation within the function? Thanks!

[peterwilsoncc]

@Mjte90 Please accept my apologies for not seeing your follow up question so long ago.

Step 1: Optimise call within wp_delete_post()

• sort $comment_ids from highest to lowest ​before deleting each comment in this loop.
• this allows the ​reparenting step to be skipped when deleting each comment.

Step 2: Accept array of comments when calling wp_delete_comment()

• when deleting an array of comments, calculate reparenting in bulk.
• if a comment's parent is to be deleted, reparent to the first ancestor not being deleted

The SQL queries may become something like this:

-- Comment threads
-- 1 -> 3 -> 5, 6 -> 11
-- 2 -> 7 -> 9
-- 8 -> 4 -> 10
-- 12 -> 13 -> 14

-- wp_delete_comment( [ 5, 6, 7, 8, 11, 12, 13 ] );

-- Determine if a parent comment is being deleted from a comment that is not being deleted

SELECT comment_ID, comment_parent
  FROM wp_comments 
  WHERE comment_parent IN (5, 6, 7, 8, 11, 12, 13) 
    AND comment_ID NOT IN (5, 6, 7, 8, 11, 12, 13);

-- result: (4, 8), (9, 7), (14, 13)
-- Comments 8, 7, 13 are being deleted and parents of comments 4, 9, 14.

-- Determine parents of 8, 7, 13 to determining grandparents of 4, 9, 14
SELECT comment_ID, comment_parent FROM wp_comments WHERE comment_ID IN (8, 7, 13);

-- result (8, 0), (7, 2), (13, 12)
-- No parent comment 8 thus no grandparent for comment 4. Comment 4 should be reparented to 0
-- Parent of comment 7 is 2, thus comment 7 should be reparented to 2
-- Parent of comment 13 is 12 (grandparent comment 14), 12 will be deleted.

-- Repeat above step for comment 12 to determine comment 14's closest ancestor 
SELECT comment_ID, comment_parent FROM wp_comments WHERE comment_ID IN (12);

-- result: (12, 0)
-- 12 has no parent comment, thus comment 14 should be reparented to 0

-- No further looping required to determine reparenting

-- Reparent as follows
-- Comment 4, reparent to 0
-- Comment 7, reparent to 3
-- Comment 14, reparent to 0

Step 2 is the more difficult step, as it requires fired hooks to remain unchanged.

[Mte90]

Unifinished patch

[Mte90]

Attached there is the first part of the patch, unfinished because is missing the reassignment of the parents and so on.
I added also a test for the new function wp_delete_comments that I am using to develop.

[Mte90]

finished patch

[Mte90]

I finished the patch but I am not sure if everything works right, probably require improvements on test side.

[andraganescu]

@Mte90 great work so far!

I think it would be better if we had two different patches one for each of the updates proposed by @peterwilsoncc :

1. Sort $comment_ids from highest to lowest before deleting each comment
2. Accept array of comments when calling wp_delete_comment()

This way it would be easier to review and test.

Speaking of tests, the tests do delete comments, but what else should we expect from these changes, other than the fact that comments still get deleted? wp_reparentering_comment should be tested, and I understood that the main goal of the sorting was to not do reparenting anymore?

Also it appears that we call wp_delete_comments( $comment_ids, true ) twice but do the sorting only once.

@peterwilsoncc , 1st thanks for this proposal! Just to make this clear for posterity: we should do these changes to speed up deleting large amounts of comments, correct?

[Mte90]

Before that I do some tests for the new function can I have some feedback about the wording in the docs and also the code, if it is doing everything right.

So I can moving on what I have already know before to do new stuff and keep bad variable naming or stuff like that.

[peterwilsoncc]

Notes

• (Bug) Rather than the look for parents, look for grandparents approach you'll need a while loop to account for great- and great-great- grandparents, etc, etc.
• (request) I'd prefer to modify wp_delete_comment rather than introduce a second function (we can discuss deprecating the singular and replacing it with the plural if needs be). Combining will allow
  • selecting meta_id to be deleted in one query
  • calling wp_update_comment_count() once (it's a fairly expensive query)

Generally the goal of above comments are to combine as many queries as possible.

---

In 37703-delete-desc.diff​ I've uploaded the initial change to avoid reparenting when deleting posts. @johnbillion are you able to express a view as to whether this can go in to 5.5? I think the bulk delete changes will need to be early.

https://core.trac.wordpress.org/ticket/37703

[peterwilsoncc]

@Mte90 I've pushed the delete in descending order patch ​to GitHub to run the tests on it. The tests pass so let's get this in as initial step in 5.8.

[Mte90]

Seems good :-)
I removed a year ago like 5000 comments and took a while but with wp-cli was less impacting on performances. I hope that this will merged soon to avoid more troubles.

[Mte90]

I don't think that this will make it for 5.8 as now, maybe move to 5.9?

[peterwilsoncc]

Moving on to 5.9 to get the initial step in to avoid reparenting of comments that are about to be deleted.

Reducing the queries with further smarts can be done on a follow up ticket per my comment above.

[hellofromTonya]

Hey @peterwilsoncc, what do you think about the step 1 for 5.9?

[peterwilsoncc]

Replying to hellofromTonya:

Hey @peterwilsoncc, what do you think about the step 1 for 5.9?

Yep, let's aim to do that. I've just updated the PR against trunk to ensure that the tests still pass and there are no merge conflicts.

[peterwilsoncc]

In 52015:

Comments: Avoid reparenting during post deletion.

Delete comments in a descending order by comment ID when deleting a post.

This avoids the expense of additional database queries required to re-parent threaded comments that are themselves about to be deleted.

Props Mte90, andraganescu, johnbillion, hellofromTonya, peterwilsoncc.
Fixes #37703.

[prbot]

​peterwilsoncc commented on ​PR #1253:

Committed in https://core.trac.wordpress.org/changeset/52015

[peterwilsoncc]

I've merged this in [52015].

I've labelled it as fixing this issue despite been only one step in the process, as we won't get the more complicated optimisation done in the current cycle and the ticket needs to remain on the milestone.

I'll create a follow up ticket.