Opened 5 years ago
Last modified 9 months ago
#37703 new enhancement
Optimise `wp_delete_comment` as called from `wp_delete_post`
Reported by: |
|
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: |
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 (16)
#3
in reply to:
↑ 2
@
4 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
@
18 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
@
18 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()
- 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
@
17 months 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
@
17 months 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
@
16 months 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
@
15 months 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
@
12 months 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.
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?