node_delete "leaks memory" due to deleted nodes cached

I found a related feature request and bug report but without patches:
#285484: Adjust node_delete
#181816: node_delete should invoke hooks before deleting rows

Attached you'll find a patch against drupal 6.3 because the same issues still exist there.

But it would be nice if you also accept the patch for drupal 5.x because this is still our current production environment ;-)

+1 I want this fix as well.

+1 as well.

Given what I wanted too, this seems more of a feature request. That is why in my original post I think this should be farmed out to D7. New features are usually not put onto stable releases, unless absolutely necessary.

Just make a 7.x patch and have the community review it. Hopefully it'll get in.

+1. I independently made the node_load($nid, NULL, TRUE) change and found it made a huge difference when deleting thousands of nodes in bulk (from a rate of about 2000 nodes deleted/minute to 45,000).

+1, changed version so it has a small chance of being seen by the core developers

The last submitted patch failed testing.

My thoughts:

• The memory leak and the ordering of DELETE queries are really two separate issues - putting them in the same patch is not conducive to getting the memory leak fixed in core.
• This is not a feature request - it is a performance bug, a significant one for anyone doing bulk deletes of nodes. I think it should be considered for 6.x.

In that light, I'm providing a patch against 6.10 that addresses the memory leak only. This change is small and safe - all it does is prevent caching of nodes that are about to be deleted anyway.

You stand a whole lot better chance of having this see the light of day by posting it against 7.x and noting that is also a serious bug in 6.x, but it's you call.

Subscribed

The patch is against Drupal 6, I haven't set up a Drupal 7 environment yet... Could someone on Drupal 7 see if the patch applies against it (or generate a patch for Drupal 7)?

Subscribed

Bugs are fixed in the current development branch first, then backported.

The last submitted patch failed testing.

In Drupal 7 this is not really fixable as we load all nodes at once with node_load_multiple and there is only one copy of them and then we pass around objects. For Drupal 6 we could try fixing it but given the behaviour in D7 I am inclined to won't fix this... if you want to delete that many nodes, batch it. Otherwise when updating to D7 your code will break again.

actually, i see this as completely harmless for d6 and it fixes a bug. why would you ever cache a node that you are deleting? d7 has some big changes here but they aren't really relevant for this patch. lets let gabor and others decide.

The patch actually clears off the whole runtime node cache on each node_load() called from node_delete(). The node is still cached, but all previous nodes caches are removed. So on subsequent node_delete()'s, the only cached node will be the one cached in the previous node_delete() (if there are no other node operations inbetween). Although Dave Reid suggests this as a workaround to fix memory issues at http://api.drupal.org/api/function/node_load/6#comment-69 I'm not sure people on this issue fully understand that this is what happens.

Retitled to the current focus of the issue.

I ran into this quirk in D6.

Besides the performance issues, it also seems to me to be incorrect behavior that it's possible to load a node which should have been explicitly deleted earlier in the session. In other words:

…gives you a node! Is it possible that there's security implications to that?