Right now, when a recipient is deleted, we manually delete associated statuses from the database instead of loading them and calling statuses_delete_status()
. The reason for doing this was that loading all the statuses and deleting them one by one would be slow. However, this judgment was a premature optimization -- I haven't done any profiling to see whether this is actually a performance concern, and anyway core does fully invoke all of the relevant node deletion hooks when a user is deleted for each individual node (node_delete_multiple() called by node_user_delete()). Deleting a node is a lot more expensive than deleting statuses -- but on the other hand there could be a lot more statuses attached to an entity than nodes. And deleting a status is probably about as fast as queuing it to process later during cron, so I'd be surprised if there was much advantage there.
Solution:
Let's just fully load each status and call statuses_delete_status() on each when we delete a sender or recipient. Using the try-catch model that node_delete_multiple() uses should prevent errors from screwing things up. We should not use transactions here since we are already nested inside the transaction from user_delete_multiple. There is still the threat of timeouts, but let's just hope that no one deletes an object with 500,000 statuses attached to it.
Comment | File | Size | Author |
---|---|---|---|
#6 | statuses-delete-node-term-1261716-1.patch | 873 bytes | IceCreamYou |
#2 | fbss_comments-user_delete.patch | 839 bytes | jonhattan |
Comments
Comment #1
IceCreamYou CreditAttribution: IceCreamYou commentedThis should happen at the same time as #1261716: Delete associated statuses when a node or taxonomy term is deleted
Comment #2
jonhattanI hit an error when deleting a user and I think the attached patch fixes it.
Later I found this issue.
Comment #3
IceCreamYou CreditAttribution: IceCreamYou commentedWhat was the error? The patch in #2 isn't a fix, it just avoids deleting status comments altogether.
Comment #4
jonhattanIt seemed to me that
statuses_user_delete()
andfbss_comments_user_delete()
delete each one its records and the code I suppress in #2 was a left over but I may be wrong.The error is just that the query is invalid. It is shocking (that this line existed in the code) because AFAIK this kind of query is invalid, at least for mysql, that is,
1/ it is not permitted to use an alias to a table:
delete from {table} t
and2/ it is not permitted to do joins in a delete statement.
Comment #5
IceCreamYou CreditAttribution: IceCreamYou commentedAh, you're right actually. Committed your patch, thanks.
Comment #5.0
IceCreamYou CreditAttribution: IceCreamYou commentedAdded proposed solution
Comment #6
IceCreamYou CreditAttribution: IceCreamYou commentedMerging #1261716: Delete associated statuses when a node or taxonomy term is deleted into this issue (the relevant patch from it is attached).
Still need to switch manual status deletion to using the API function when a recipient of any type is deleted.
Comment #7
mathankumarc CreditAttribution: mathankumarc commentedSorry for the late replay. I asked the same question in drupal.stackexchange.com - http://drupal.stackexchange.com/questions/51172/how-to-delete-the-bulk-o...
I think this will help us to make better decision.
Comment #8
IceCreamYou CreditAttribution: IceCreamYou commentedAs I see it we basically have 3 options:
Comment #8.0
IceCreamYou CreditAttribution: IceCreamYou commentedWe should not use transactions here since we are already nested inside the transaction from user_delete_multiple.
Comment #9
SocialNicheGuru CreditAttribution: SocialNicheGuru commented