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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

IceCreamYou’s picture

Status: Active » Postponed
Issue tags: +low-hanging fruit
jonhattan’s picture

I hit an error when deleting a user and I think the attached patch fixes it.

Later I found this issue.

IceCreamYou’s picture

What was the error? The patch in #2 isn't a fix, it just avoids deleting status comments altogether.

jonhattan’s picture

It seemed to me that statuses_user_delete() and fbss_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 and
2/ it is not permitted to do joins in a delete statement.

IceCreamYou’s picture

Ah, you're right actually. Committed your patch, thanks.

IceCreamYou’s picture

Issue summary: View changes

Added proposed solution

IceCreamYou’s picture

Status: Postponed » Active
FileSize
873 bytes

Merging #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.

mathankumarc’s picture

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

IceCreamYou’s picture

As I see it we basically have 3 options:

  1. Just use statuses_delete_status() on every status and hope we don't time out.
  2. Use the method suggested in your StackExchange question to mark statuses as deleted and process them later during cron.
  3. Batch delete statuses when the user is deleted. I don't know whether it is possible to run a batch process at that stage, but this is probably the best choice if it is.
IceCreamYou’s picture

Issue summary: View changes

We should not use transactions here since we are already nested inside the transaction from user_delete_multiple.

SocialNicheGuru’s picture

Status: Active » Needs work