#37703 closed enhancement (fixed)
Optimise `wp_delete_comment` as called from `wp_delete_post`
Reported by: | peterwilsoncc | Owned by: | 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)
Change History (28)
#3
in reply to:
↑ 2
@
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.
#6
@
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
@
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()
- 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.
#8
@
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.
#9
@
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
@
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 :
- Sort $comment_ids from highest to lowest before deleting each comment
- 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
@
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
@
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)
- selecting
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
This ticket was mentioned in PR #1253 on WordPress/wordpress-develop by peterwilsoncc.
4 years ago
#15
#16
@
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
@
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.
#19
@
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.
#21
in reply to:
↑ 20
@
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
@
3 years ago
- Owner set to peterwilsoncc
- Resolution set to fixed
- Status changed from new to closed
In 52015:
peterwilsoncc commented on PR #1253:
3 years ago
#24
Committed in https://core.trac.wordpress.org/changeset/52015
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?