Opened 7 years ago
Last modified 7 years ago
#43813 new enhancement
Meta API should set `last_changed` cache key internally
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | Awaiting Review | Priority: | normal |
Severity: | normal | Version: | |
Component: | General | Keywords: | |
Focuses: | Cc: |
Description
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)
#2
@
7 years 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:
↓ 4
@
7 years 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
@
7 years 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.
I like this for its simplicity, but I'm concerned about third-party uses of
update_metadata()
. This change would mean that we're settinglast_changed
for any object typefoo
that may be passed in. More conservatively, we could work from a whitelist: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 expectinglast_changed
to be in a format other thanmicrotime()
. 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:(rather than mucking about with
's'
)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?