Make WordPress Core

Opened 8 years ago

Closed 3 years ago

Last modified 3 years ago

#37703 closed enhancement (fixed)

Optimise `wp_delete_comment` as called from `wp_delete_post`

Reported by: peterwilsoncc's profile peterwilsoncc Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: Comments Keywords: has-patch has-unit-tests needs-testing dev-feedback
Focuses: performance Cc:

Description

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

Potentially accept an array of comments.

Attachments (3)

37703.unifinished.patch (8.0 KB) - added by Mte90 5 years ago.
Unifinished patch
37703.1.patch (9.1 KB) - added by Mte90 5 years ago.
finished patch
37703-delete-desc.diff (1.2 KB) - added by peterwilsoncc 5 years ago.

Download all attachments as: .zip

Change History (28)

#1 @peterwilsoncc
8 years ago

  • Keywords needs-patch needs-unit-tests added

#2 follow-up: @Mte90
7 years ago

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?

#3 in reply to: ↑ 2 @peterwilsoncc
7 years ago

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.

#4 @Mte90
7 years ago

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

#5 @johnbillion
7 years ago

  • Keywords reporter-feedback added

#6 @johnbillion
5 years ago

@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!

#7 @peterwilsoncc
5 years ago

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

Step 1: Optimise call within wp_delete_post()

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
5 years ago

Unifinished patch

#8 @Mte90
5 years ago

  • Keywords needs-refresh has-patch has-unit-tests added; needs-patch needs-unit-tests reporter-feedback removed

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
5 years ago

finished patch

#9 @Mte90
5 years ago

  • Keywords 2nd-opinion needs-testing added; needs-refresh removed

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

#10 @andraganescu
5 years ago

  • Keywords dev-feedback added; 2nd-opinion removed

@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?

#11 @Mte90
5 years ago

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.

#12 @peterwilsoncc
5 years ago

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.

This ticket was mentioned in Slack in #core by mte90. View the logs.


5 years ago

This ticket was mentioned in Slack in #core by mte90. View the logs.


4 years ago

#16 @peterwilsoncc
4 years ago

@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.

#17 @Mte90
4 years ago

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.

#18 @Mte90
3 years ago

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

#19 @peterwilsoncc
3 years ago

  • Milestone changed from Awaiting Review to 5.9

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.

#20 follow-up: @hellofromTonya
3 years ago

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

#21 in reply to: ↑ 20 @peterwilsoncc
3 years ago

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.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


3 years ago

#23 @peterwilsoncc
3 years ago

  • Owner set to peterwilsoncc
  • Resolution set to fixed
  • Status changed from new to closed

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.

#25 @peterwilsoncc
3 years ago

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.

Note: See TracTickets for help on using tickets.