WordPress.org

Make WordPress Core

Opened 2 months ago

Last modified 2 months ago

#43813 new enhancement

Meta API should set `last_changed` cache key internally

Reported by: johnjamesjacoby Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: General Keywords:
Focuses: Cc:

Description

See #40669, r41848, r41849.

Sorry, but I completely missed this work being done for the 4.9 release cycle.

I think the individual post/comment/term/user checks should be moved directly into the appropriate meta API function calls.

I can imagine 2 ways of doing this:


Add a $cache_key parameter to the following functions:

  • add_metadata()
  • update_metadata()
  • delete_metadata()

When $cache_key is not empty, it will internally call:

wp_cache_set( 'last_changed', microtime(), $cache_key );


Infer the cache-key using $meta_type in the following function:

  • update_meta_cache()

Then call:

wp_cache_set( 'last_changed', microtime(), $meta_type . 's' );

(We already use $meta_type to infer things like primary keys, so it's not great, but not new.

If we went this route, I believe we'd probably want to do a better job of priming meta-data caches the same way we prime primary object caches (users and posts specifically prime themselves on update, etc...)

cc @boonebgorges @spacedmonkey for consult & discussion

Change History (4)

#1 @boonebgorges
2 months ago

Infer the cache-key using $meta_type in the following function

I like this for its simplicity, but I'm concerned about third-party uses of update_metadata(). This change would mean that we're setting last_changed for any object type foo that may be passed in. More conservatively, we could work from a whitelist:

// meta_type => cache_group
$cache_groups = array(
    'post'    => 'posts',
    'comment' => 'comments',
    // etc
);

if ( isset( $cache_groups[ $meta_type ] ) ) {
    wp_cache_set( 'last_changed', microtime(), $cache_groups[ $meta_type ] );
}

Of course, it could be that I'm too conservative here. The only real harm that could be caused if WP set last_changed for arbitrary cache groups is if those plugins were expecting last_changed to be in a format other than microtime(). We could probably look through the plugin repo and maybe do some other environment scans to see if this is a real concern. If not, I think something like the above could be modified to:

$cache_group = isset( $cache_groups[ $meta_type ] ) ? $cache_groups[ $meta_type ] : $meta_type;
wp_cache_set( ... );

(rather than mucking about with 's')

If we went this route, I believe we'd probably want to do a better job of priming meta-data caches the same way we prime primary object caches (users and posts specifically prime themselves on update, etc...)

I'm not sure I understand. Are there specific places where we're not doing a good job of this? How does the refactoring of how the last_changed incrementor gets bumped - the subject of this ticket - affect the priming of metadata caches?

#2 @johnjamesjacoby
2 months ago

I'm not sure I understand. Are there specific places where we're not doing a good job of this? How does the refactoring of how the last_changed incrementor gets bumped - the subject of this ticket - affect the priming of metadata caches?

Sorry; I was implying/speculating there may be more here, because meta caches are primed opposite of the last_changed key.

For example, when using the meta-data API directly (not the per-object helpers) calls to update_meta_cache() happen inversely to when the last_changed cache key gets updated. It's hard to tell (without testing for the condition) but looks like it's possible for an "updated meta cache" to have an out of date last_changed key.

That makes me think, the meta-data API could use a way to relate meta cache-groups to object cache-groups, like you mentioned. If we need that, maybe this turns into a bigger, different issue. Would be nice if that whitelist had a filter, so plugins can just add their relationship to it.

(Separately, BuddyPress doesn't appear to update any last_changed keys, so while our meta-cache implementation is improved over WordPress core, it probably needs the same treatment WordPress needs here.)

The only real harm that could be caused if WP set last_changed for arbitrary cache groups is if those plugins were expecting last_changed to be in a format other than microtime()

A helper function like wp_cache_set_last_changed() would help there, so there is some functional format enforcement in core.

See: https://core.trac.wordpress.org/ticket/37464#comment:17

#3 follow-up: @flixos90
2 months ago

  • Type changed from defect (bug) to enhancement

While I'm generally in favor of this change, I think we might wanna hold off this for a bit. Something to consider here is that moving this logic into the actual metadata API functions will make it harder to make adjustments specific to only a subset of these object types - which is generally nothing we should strive for, but something that may be necessary sometimes.

  • For example experimenting with a meta-related caching improvement should rather happen on a less-used component like site meta first before shooting it out there to be used with posts, terms etc immediately.
  • Another example is that user meta right now does not invalidate cache on meta changes - which may have a background, I'm not sure. Just highlighting this as another example where all object types (unfortunately) are not the same.

A specific idea I had in mind was to introduce meta-specific last changed keys for every object type (see #43818), to make cache invalidation less aggressive which would result in less unnecessary cache misses for queries not using any metadata. Something like this I think should rather be tested with something like site meta before applying it to the really frequently used post meta - just so that we can detect most basic issues (if any) immediately before testing it under heavier load.

#4 in reply to: ↑ 3 @boonebgorges
2 months ago

Replying to flixos90:

While I'm generally in favor of this change, I think we might wanna hold off this for a bit.

Yes, this seems fine to me. @johnjamesjacoby 's other points notwithstanding, the main idea here was to cut down on code reduplication. But if the general feeling is that we're not ready for this yet because the patterns aren't well-enough established, then let's wait.

Note: See TracTickets for help on using tickets.