Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WP_REDIS_IGNORED_GROUPS: Why is counts in there? I want to cache that group #74

Closed
jerclarke opened this issue May 4, 2018 · 2 comments · Fixed by #345
Closed

WP_REDIS_IGNORED_GROUPS: Why is counts in there? I want to cache that group #74

jerclarke opened this issue May 4, 2018 · 2 comments · Fixed by #345

Comments

@jerclarke
Copy link
Contributor

jerclarke commented May 4, 2018

WP_REDIS_IGNORED_GROUPS (formerly ->no_redis_groups) controls the part of the object cache related to the wp_cache_add_non_persistent_groups function in core.

Groups added to this constant and thus registered in the plugin don't get cached ever.

On our site, this ended up being the biggest slowdown, because wp_count_posts uses that group to cache it's query, and for us that query is really slow because we have sooo many posts. Because that query runs on basically every page on the site, it would be enormously valuable to have it in the cache.

So the question is this: Why is it in WP_REDIS_IGNORED_GROUPS and can it be removed?

FWIW based on our research I suspect this is a question with a perfectly good answer, but I wanted to create this ticket anyway so there's at least a reference. I'll update this original post with as much info as I can find.


History of inclusion in this plugin

So it goes all the way back and was never something this plugin made as a conscious decision. Good to know.

In any event, digging deeper into the core code we find that the core pluggable function wp_start_object_cache() (which is overridden by this plugin's function) is using the same defaults anyway:

	wp_cache_add_non_persistent_groups( array( 'counts', 'plugins' ) );

And, basically, the object caching plugins are just re-implementing this blacklist from core.

Okay so to me this implies that most likely it just isn't appropriate to have the counts group ever get persistently cached, and that core never really promised they would be.

Specifically, the logic here seems to be that they want the counts cached WITHIN each page, but to have them re-generated on each new pageload.

The problem for me still is I don't know why counts can't be persistently cached. wp_start_object_cache() has no comments anywhere near the call to wp_cache_add_non_persistent_groups so it's not at all clear why this hardcoded list is present.

@tillkruss
Copy link
Member

Yeah, I don't know either. I just followed the WordPress defaults. I'd say remove it and see what happens. Might it's just a relic.

lots0logs added a commit to lots0logs/redis-cache that referenced this issue Apr 4, 2022
Don't allow groups that were manually removed using `WP_REDIS_IGNORED_GROUPS` constant to be added to the ignored groups list.

* FIxes rhubarbgroup#114
* Fixes rhubarbgroup#74
* Ref: https://core.trac.wordpress.org/ticket/47884#ticket
tillkruss pushed a commit that referenced this issue Aug 11, 2022
* Update add_non_persistent_groups()

Don't allow groups that were manually removed using `WP_REDIS_IGNORED_GROUPS` constant to be added to the ignored groups list.

* FIxes #114
* Fixes #74
* Ref: https://core.trac.wordpress.org/ticket/47884#ticket

* Fix error in logic

* Incorporate pr review feedback

* improved implementation

* fix typo

* Added new filter: redis_add_non_persistent_groups

This makes it possible to override the default ignored groups set by WordPress core without any complex, hard to follow logic attempting to filter out the default groups only when they were removed using `WP_REDIS_IGNORED_GROUPS` constant.

* update filter name to use existing naming scheme
@sun
Copy link

sun commented Aug 25, 2022

Cross-posting from https://core.trac.wordpress.org/ticket/47884#comment:8:

The group "counts" is marked as non-persistent, because wp_count_posts() is creating user-specific cache items, counting the amount of private posts the user is able to access:
https://github.com/WordPress/WordPress/blob/668a2ec9ffbbdbda006c257b7a21ea12273b6082/wp-includes/post.php#L3036-L3048

As there are unlimited possible user IDs, it is impossible to invalidate all cache items when posts are saved.

_transition_post_status() invalidates the non-user-specific cache item:
https://github.com/WordPress/WordPress/blob/668a2ec9ffbbdbda006c257b7a21ea12273b6082/wp-includes/post.php#L7647-L7650

It also invalidates the cache item of the currently logged-in user. But it does not invalidate the cache items for other users.

A solution to this would be to introduce a new Object Cache API method wp_cache_flush_group() that allows to flush all items in the given group. Some backends would support this natively, some others would need to loop over (all) keys to find the ones to remove. wp_count_posts() would then use a combined group name of counts:{$post_type}.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants