WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 10 days ago

#37703 new enhancement

Optimise `wp_delete_comment` as called from `wp_delete_post`

Reported by: peterwilsoncc Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Comments Keywords: has-patch has-unit-tests needs-testing dev-feedback
Focuses: performance Cc:
PR Number:

Description

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

Potentially accept an array of comments.

Attachments (2)

37703.unifinished.patch (8.0 KB) - added by Mte90 6 weeks ago.
Unifinished patch
37703.1.patch (9.1 KB) - added by Mte90 6 weeks ago.
finished patch

Download all attachments as: .zip

Change History (13)

#1 @peterwilsoncc
3 years ago

  • Keywords needs-patch needs-unit-tests added

#2 follow-up: @Mte90
2 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
2 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
2 years ago

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

#5 @johnbillion
2 years ago

  • Keywords reporter-feedback added

#6 @johnbillion
3 months 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
3 months 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
6 weeks ago

Unifinished patch

#8 @Mte90
6 weeks 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
6 weeks ago

finished patch

#9 @Mte90
6 weeks 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
2 weeks 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
10 days 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.

Note: See TracTickets for help on using tickets.