Make WordPress Core

Opened 17 years ago

Closed 2 years ago

Last modified 22 months ago

#4476 closed enhancement (fixed)

Delete Cache by Group

Reported by: filosofo's profile filosofo Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.1 Priority: normal
Severity: normal Version: 2.2
Component: Cache API Keywords: has-patch has-unit-tests commit needs-dev-note
Focuses: Cc:

Description

Currently, you have to specify the id of the cached item in order to delete it via wp_cache_delete, or you have to flush the entire cache via wp_cache_flush.

Instead, it would be helpful if one were able to delete an entire group without having to flush all of the cache.

For example, term-related plugins could expect their particular caches to be deleted whenever changes to any terms were made, instead of having to use callbacks for every term-related action.

Attachments (1)

cache.php.diff (1.1 KB) - added by filosofo 17 years ago.

Download all attachments as: .zip

Change History (52)

@filosofo
17 years ago

#1 @filosofo
17 years ago

  • Keywords has-patch added

#2 @ryan
17 years ago

  • Milestone changed from 2.3 to 2.4 (next)

#3 @ryan
16 years ago

  • Resolution set to invalid
  • Status changed from new to closed

Cache has been gutted, so this is no longer relevant.

#4 @Nazgul
16 years ago

  • Milestone 2.5 deleted

#5 @sc0ttkclark
12 years ago

  • Cc lol@… added
  • Keywords dev-feedback added
  • Resolution invalid deleted
  • Status changed from closed to reopened
  • Version set to 2.2

Looks like the reason this was closed is no longer the case. What's the plan here? I want to delete all items for a specific group.

For now, I'll just work with the object itself, but there's a definite need for a function like wp_cache_delete_group or the patch as attached.

#6 @SergeyBiryukov
12 years ago

  • Component changed from Administration to Cache
  • Keywords cache wp_cache_delete removed
  • Milestone set to Awaiting Review

#7 @scribu
12 years ago

The reason this is not part of the API is that most persistent caching backend such as memcache don't support deleting an entire group.

What's done instead is increment the group prefix and let memcache's garbage collector remove the items from the old group, which are never accessed anymore.

#8 @scribu
12 years ago

  • Keywords close added; dev-feedback removed

#9 @sc0ttkclark
12 years ago

  • Resolution set to wontfix
  • Status changed from reopened to closed

Okay, sorry for my n00bness as I'm diving deeper into WP caching than ever before, but how would I increment the group prefix? Feel free to reply with a *slap*

#10 @scribu
12 years ago

Actually, you don't change the prefix of the group, but add a prefix to each individual key:

$ns_key = wp_cache_get( 'foo_namespace_key' );

// if not set, initialize it
if ( $ns_key === false )
  wp_cache_set( 'foo_namespace_key', 1 );

$my_key = "foo_".$ns_key."_12345";
$my_value = wp_cache_get( $my_key, 'some_group' );

To clear the namespace do:

wp_cache_incr( 'foo_namespace_key' );

Source: https://groups.google.com/forum/?fromgroups#!topic/memcached/Izov0cFjBXI

#11 @scribu
12 years ago

  • Milestone Awaiting Review deleted

#12 follow-up: @Ste_95
7 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

Why hasn't this received any more attention? I still think it's relevant. The invalidation method posted by @scribu is good with memcached, but not if cached data goes to the database as a transient:

It is important to note that if you are not careful about how you are using this design pattern, you could accidentally consume all of your cache storage space. This strategy relies on changing the key that is used to access the cached object; it never removes the previously cached versions of the objects. It leaves it in the cache's storage. If your caching backend relies on a caching technology that does not automatically handle cache evictions, you may completely fill that storage. I tend to work with memcached on large projects that need efficient caching. Memcached implements an aggressive (sometimes way too aggressive) eviction policy whereby cached objects are frequently removed from cache. You can change the keys for the cached objects and the older objects will be removed from the cache, making room for newer versions of the objects. As such, the incrementor invalidation strategy is an effective tool in a memcached environment.

https://www.tollmanz.com/invalidation-schemes/

Why not implement the patch from @filosofo? Or at least provide a method to access the $cache class var, so that we could iterate through the category array and manually delete each key?

Last edited 7 years ago by Ste_95 (previous) (diff)

#13 @Ste_95
7 years ago

Any news on thys guy? @SergeyBiryukov tagging you as you seem to be the only one with recent activity.

#14 in reply to: ↑ 12 @dd32
7 years ago

  • Resolution set to wontfix
  • Status changed from reopened to closed

Replying to Ste_95:

Why hasn't this received any more attention? I still think it's relevant.

This was made irrelevant when WordPress removed persistent caching.
File-based Persistent caching was removed in WordPress 2.5 and switched to a non-persistent per-request cache instead, which removed the need to clear the cache like this.

The cache is designed to be replaced by a persistent in-memory cache such as memcache, redis, or any other number of cache backends. These do not generally allow deletion of an entire cache group, adding support to WordPress for it is therefor not exactly useful.

Why not implement the patch from @filosofo? Or at least provide a method to access the $cache class var, so that we could iterate through the category array and manually delete each key?

While we could enable this, it would cause your code to be not-very-portable, as soon as you used it on any site that had an external Object Caching dropin enabled, you'd see the code completely fail to operate as expected.

The invalidation method posted by scribu is good with memcached, but not if cached data goes to the database as a transient:

Transients are completely different to the wp_cache_*() functions, although transients do utilise them if an external cache is in use.
If you've got wp_cache_*() setup through a drop-in to use transients, then that's probably a bad move - it's really not designed to be used like that, and you'd be better off with that disabled in many scenario's I would assume.

You may want to look at how you're using transients if that's the case, and reconsider their usage. It sounds like you're using them in a way they weren't designed, or something similar. #20316 may interest you.

I'm re-closing this ticket as wontfix, you may comment here while the ticket is closed, but for the reasons outlined above, we won't be adding a Delete Cache by Group functionality right now, and probably not anytime soon.

#15 @dhilditch
5 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

Instead of adding 'delete by group', to achieve the same desired functionality - namely, the ability to 'delete' chunks from the cache without wiping the entire cache - how about this approach:

  1. Add an extra parameter to wp_cache_get and wp_cache_set called 'namespace'
  2. When the namespace parameter exists, use the code given by @scribu to append a number to the $key param
  3. Add a new function wp_cache_clear_namespace with the namespace parameter - this function increments the namespace number so - in-effect - all items for this namespace are 'deleted'.

Otherwise EVERY developer has to keep coding their own versions of the code from @scribu both for getting items from the cache and for setting items in the cache, and most won't bother and instead will just flush the entire cache when their settings pages change which is entirely unnecessary and detrimental to the performance of large websites.

#16 @dougal
5 years ago

I have a current use-case where deleting by group would be useful -- a long-running report job where I want to track whether I've already processed certain items in the current queue run, then delete the cached hits after the report is done.

But yeah, since not all external caching mechanisms provide a mechanism for that functionality, and the dropins would have to be updated with the new method, I can see why it's wontfix. :-/

#17 @SergeyBiryukov
5 years ago

  • Milestone set to Awaiting Review

#18 @lucasbustamante
4 years ago

It would be nice if WordPress could enforce an object-cache API that the drop-ins could follow so we could use it reliably in plugin development.

For instance, Pantheon's Redis drop-in supports deleting a group of cache keys with wp_cache_delete_group( $group ):

https://github.com/pantheon-systems/wp-redis/blob/master/object-cache.php#L98
https://github.com/pantheon-systems/wp-redis/blob/master/object-cache.php#L547

While this other Redis drop-in with 60k+ installations does not:
https://wordpress.org/plugins/redis-cache/ (https://github.com/devgeniem/wp-redis-object-cache-dropin/blob/master/object-cache.php)

Automattic WP Memcached doesn't support groups, either: https://github.com/Automattic/wp-memcached/blob/master/object-cache.php

The solution I propose is that WordPress enforces an interface on cache drop-ins, so we can be sure that they will allow deleting groups. If Memcached needs some namespace hacks to work with groups, then that's handled internally by the drop-in.

#19 @SergeyBiryukov
4 years ago

  • Keywords close removed

Just noting that wp_cache_get_multiple() was added to wp-includes/cache-compat.php in [47939] / #20875 as a compat function for drop-ins that don't support it natively.

It seems like wp_cache_delete_group() could be added in the same way too.

#20 @dg12345
3 years ago

  • Keywords needs-patch added; has-patch removed

The main reason I can think to do this is that, currently to delete targeted cache you must use the "last_changed" method which essentially just adjust the keys used but that would also leave the old keys in the cache, so your redis or memcache will slowly get bigger.

The only other way is for you to keep track of all the keys used and write a script to delete each one when you want to delete the cache.

I agree with @lucasbustamante, simply putting it in as an official core function you will force the hand of the plugin writers to implement it in the main redis and memcache plugins.

I would think its a five minute job to add a delete by group function, the core doesn't even need to use it (although give the cache size issue it would be nice if it did).

This ticket was mentioned in PR #2368 on WordPress/wordpress-develop by pbearne.


2 years ago
#21

  • Keywords has-patch has-unit-tests added; needs-patch removed

#22 @spacedmonkey
2 years ago

I understand the desire for this ticket to go into core. But apart from the redis examples seen in this ticket, how would this work in other object cache drop-ins. How would this work for a memcache implementation? If someone could link to or provide a code snippet for a memcache or other object cache implement, that would be helpful.

If we can reliably have a way for keys in a group to be in core, then sadly, this function can not live core. The worry is that someone calls wp_cache_delete_group and it simply does not work. If this function existing in core, we could need some way to backfill / pollyfil, like done in wp_cache_get_multiple patch. Again, a can't think of a good way to do this without massively refactoring the internals of the object cache.

spacedmonkey commented on PR #2368:


2 years ago
#23

If this to live in core there needs to be a compat function found in core in cache-compat.php. This function would need to work for ALL object cache implementation, including and not limited to memcached.

I personally think this is going be nearly impossible, as it is hard to know the implementation details of every object cache. But without that compatible layer, this can not live in core 😞

tillkruss commented on PR #2368:


2 years ago
#24

This function would need to work for ALL object cache implementation, including and not limited to memcached.

It's a FAQ and the suggested approch is using prefixes, which I believe `wp-memcached` is already using heavily.

spacedmonkey commented on PR #2368:


2 years ago
#25

Please use remove test for wp_cache_get_linked_meta and revert changes to comments and spacing that are not required for this change.

spacedmonkey commented on PR #2368:


2 years ago
#26

If this is going to make it into core, then we are going to need some constant, like WP_ALLOW_CACHE_GROUPS. Drop-in providers, will have to define this. That way, developers / core can work out if we can even use this function.

Thoughts?

tillkruss commented on PR #2368:


2 years ago
#27

If this is going to make it into core, then we are going to need some constant, like WP_ALLOW_CACHE_GROUPS. Drop-in providers, will have to define this. That way, developers / core can work out if we can even use this function.

Reminds me of get_theme_support() and add_theme_support().

If we default to flushing the entire cache (in the compatibility method), can you give an example _how and where_ WP_ALLOW_CACHE_GROUPS would be used?

pbearne commented on PR #2368:


2 years ago
#28

I am not sure we do need the global WP_ALLOW_CACHE_GROUPS as I test for a flush_group function
method_exists( $wp_object_cache, 'flush_group' )
and return false if not available so the calling code and handle it

This ticket was mentioned in Slack in #core-committers by spacedmonkey. View the logs.


2 years ago

#30 @spacedmonkey
2 years ago

  • Milestone changed from Awaiting Review to Future Release
  • Owner set to spacedmonkey
  • Status changed from reopened to assigned

#31 @spacedmonkey
2 years ago

Even through the PR at #2368 looks promising, it is clear from the backwards and forwards on the code review, that this ticket needs a little more consideration.

I am hopeful this can be resolve and make it into a future release.

spacedmonkey commented on PR #2368:


2 years ago
#32

There seems to be unit tests failing now and coding standards need fixing.

aksld commented on PR #2368:


2 years ago
#33

Hello,

What is the next step ?

Do you need help for something ?

tillkruss commented on PR #2368:


2 years ago
#34

@aksld The memcache object cache used by the tests needs support for this feature. Likely by using prefixes that increment.

spacedmonkey commented on PR #2969:


2 years ago
#36

This is already looking good. Added some early feedback. The unit test failure, you may want to look into that.

#37 @tillkruess
2 years ago

I've opened a refreshed PR for this. Feedback greatly appreciated.

https://github.com/WordPress/wordpress-develop/pull/2969

This ticket was mentioned in PR #2996 on WordPress/wordpress-develop by peterwilsoncc.


2 years ago
#39

@spacedmonkey @tillkruss

Is this too silly to even consider? I suspect it might be.

WIP -- needs some docs updated, etc.

peterwilsoncc commented on PR #2996:


2 years ago
#40

Nevermind, this won't work.

The wp_cache_* functions are effectively pluggable and it is expected the drop-ins will define them. A persistent drop-in that doesn't support group flushing would need to be updated to match hacking the groups with the last_changed key.

I'll continue investigating using an action on the last_changed key to allow drop-ins to hook in to that to trigger a group flush.

{{{php
add_action(

'wp_cache_set_last_changed',
function( $group ) {

global $object_cache;
$object_cache->flush_group( $group );

}

);
}}}

#41 @spacedmonkey
2 years ago

  • Keywords commit added

spacedmonkey commented on PR #2969:


2 years ago
#42

Marking a ready to commit.

spacedmonkey commented on PR #2368:


2 years ago
#43

Committed

spacedmonkey commented on PR #2969:


2 years ago
#44

Committed

spacedmonkey commented on PR #2996:


2 years ago
#45

Committed.

#46 @SergeyBiryukov
2 years ago

  • Milestone changed from Future Release to 6.1
  • Resolution set to fixed
  • Status changed from assigned to closed

Fixed in [53763].

#47 @SergeyBiryukov
2 years ago

In 53767:

Cache API: Make the placement of wp_cache_flush_group() more consistent.

Includes:

  • Placing WP_Object_Cache::flush_group() next to ::flush().
  • Placing wp_cache_supports_group_flush() next to wp_cache_flush_group().
  • Placing the wp_cache_flush_group() unit test next to the ::flush() method test.
  • Removing test name from assertion messages, as it is already mentioned directly above in case of failure.
  • Adjusting function descriptions per the documentation standards.

Follow-up to [52706], [53763].

See #55647, #4476.

This ticket was mentioned in Slack in #forums by davehilditch. View the logs.


2 years ago

#49 @milana_cap
23 months ago

  • Keywords add-to-field-guide added

#50 @desrosj
23 months ago

  • Keywords needs-dev-note added

There are a lot of caching API related changes in 6.1 and a dev note should be written collecting them all. Marking this for inclusion.

#51 @milana_cap
22 months ago

  • Keywords add-to-field-guide removed
Note: See TracTickets for help on using tickets.