Unable to remove "counts" from non persistent cache groups

[Maciej Laskowski]

Hi,

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.

[Maciej Laskowski]

Hi guys,

Any news on this issue?

[desrosj]

@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()?).

[boonebgorges]

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.

[desrosj]

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.

[boonebgorges]

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.

[desrosj]

Works for me. Thanks!

[tha_sun]

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

<?php
        $query = "SELECT post_status, COUNT( * ) AS num_posts FROM {$wpdb->posts} WHERE post_type = %s";


        if ( 'readable' === $perm && is_user_logged_in() ) {
                $post_type_object = get_post_type_object( $type );
                if ( ! current_user_can( $post_type_object->cap->read_private_posts ) ) {
                        $query .= $wpdb->prepare(
                                " AND (post_status != 'private' OR ( post_author = %d AND post_status = 'private' ))",
                                get_current_user_id()
                        );
                }
        }


        $query .= ' GROUP BY post_status';

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

<?php
        if ( $new_status !== $old_status ) {
                wp_cache_delete( _count_posts_cache_key( $post->post_type ), 'counts' );
                wp_cache_delete( _count_posts_cache_key( $post->post_type, 'readable' ), 'counts' );
        }

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

[tillkruess]

@tha_sun: The wp_cache_flush_group() function was added to 6.1: ​https://github.com/WordPress/wordpress-develop/pull/2969