Make WordPress Core

Opened 8 months ago

Closed 5 months ago

#47884 closed defect (bug) (wontfix)

Unable to remove "counts" from non persistent cache groups

Reported by: Maciej Laskowski Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.6
Component: Cache API Keywords:
Focuses: performance Cc:



I am using https://github.com/tillkruss/redis-cache as a Redis adapter and a persistent object cache plugin. I would like to cache "counts" cache group in Redis as well but even if I remove it from non persistent groups in redis-cache Wordprpress adds it back in https://core.trac.wordpress.org/browser/branches/5.2/src/wp-includes/load.php#L586. Commenting out this line solves the problem but it is not a solution and I cannot find any way of disabling this default WordPress behaviour.

It looks like a bug that whenever object-cache dropin defines wp_cache_add_global_groups() function (like redis-cache does) it is impossible to remove "counts" and "plugins" from non persistent cache groups.

Change History (7)

#1 @mukesh27
8 months ago

  • Focuses performance added
  • Keywords dev-feedback added
  • Version changed from 5.2.2 to 3.0

#2 @Maciej Laskowski
7 months ago

Hi guys,

Any news on this issue?

#3 @desrosj
6 months ago

  • Keywords 2nd-opinion added
  • Version changed from 3.0 to 2.6

@maciej-laskowski Welcome to Trac!

For the time being, I think this could be solved by modifying the drop-in. Looking at the drop-in you linked to, it seems that you could just check if counts is a value in WP_Object_Cache::add_non_persistent_groups() (this line) and remove it before updating the list of groups.

As for fixing this in Core, I don't know much about the history of the groups coded to not persist with wp_cache_add_non_persistent_groups(). I'd love to get some feedback from others a bit more experienced with the history (maybe @boonebgorges, @spacedmonkey?), but I believe that the cache entries in these groups are not reliably invalidated. Looking back, this function was added in [7986] (see #6740). My hunch is that you will have issues with cache invalidation if you are not very careful with this.

Side note: the code block you linked to is in a conditional that checks for the existence of wp_cache_add_global_groups(), but assumes the existence of wp_cache_add_non_persistent_groups(). It may be good to add a function_exists( 'wp_cache_add_non_persistent_groups' ) check there as a precaution (maybe with a _doing_it_wrong()?).

#4 @boonebgorges
5 months ago

The changesets that @desrosj linked to are all I'm able to find about the history of the 'counts' group, and the fact that it's nonpersistently cached.

You'll notice that [7986] made the 'comment' group non-persistent. We reversed this in #36906. To fix it, we needed a thorough audit of the ways in which the cache is used throughout core, and then a sweep through to ensure invalidation, as appropriate. Something similar would need to happen for 'counts' if we wanted to make that group persistent as well.

Since wp_cache_add_non_persistent_groups() doesn't actually do anything in WP itself - each cache drop-in has its own implementation of it - I don't think there's anything we can do about the fact that WP "adds it back". I guess I can see a place for wp_cache_remove_non_persistent_groups() or something similar, but this would only be a stub function in WP; it'd need to be implemented in the Redis drop-in, and then you'd need to call it *after* WP establishes its default non-persistent cache groups. As @desrosj, you can already do a version of this in your own drop-in, without any changes in WordPress. The main consequences of adding such a stub to WP would be to encourage dropin maintainers to implement the functionality in their drop-ins, and to encourage developers to make non-persistent into persistent caches. But this doesn't feel like the kind of thing we should encourage, since it's an invitation to introduce cache bugs - non-persistent caches are that way for a reason. To me it feels wiser to leave it out of WP, while acknowledging that developers like yourself who are aware of the potential fallout from improper invalidation are able to roll your own implementation in the cache drop-in.

#5 @desrosj
5 months ago

  • Keywords close added; dev-feedback 2nd-opinion removed

Thanks for checking into this, @boonebgorges!

Do you have any opinion on the side note about missing function_exists check for wp_cache_add_non_persistent_groups?

Marking to close as wontfix. If it's decided the function_exists check should be added, I'll open a new ticket.

#6 @boonebgorges
5 months ago

Do you have any opinion on the side note about missing function_exists check for wp_cache_add_non_persistent_groups?

Looking at the function_exists() call in [7986], I'm guessing the thought was: Any drop-ins existing before [7986] would not have the two new functions, but any new drop-ins created afterward presumably would, because they'd be created from the up-to-date cache.php. Based on this assumption, the single function_exists() check is enough, since the functions were introduced at the same time. The only way to trigger a fatal here would be through an incorrectly built drop-in, and if it hasn't cropped up in the past 11 years, I'd say it's not worth guarding against now.

#7 @desrosj
5 months ago

  • Keywords close removed
  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

Works for me. Thanks!

Note: See TracTickets for help on using tickets.