WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 7 weeks ago

#31245 assigned enhancement

Replace alloptions with a key cache

Reported by: rmccue Owned by: rmccue
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Options, Meta APIs Keywords: has-patch needs-testing
Focuses: performance Cc:

Description

Let's talk about alloptions. alloptions is a particularly fragile piece of WordPress, since it's exceptionally vulnerable to accidental concurrency issues due to its design.

A primer on alloptions, for those who haven't dug into it: alloptions is a cache key that stores all the options marked with autoload, thereby reducing the number of external calls out to an object cache from hundreds down to 1. It does this by running SELECT * FROM wp_options WHERE autoload = 'yes', then storing that array (map of option_name => option_value) under the alloptions cache key. (Non-autoloaded options are stored as individual cache items instead, with a key of option_name in the options group.)

When you add an option, you can pick the autoload flag. This flag is 'yes' (true) by default, meaning most calls to add_option will set options to autoload. Additionally, if you add an option via update_option, there's no way to set this flag. These combined mean that the vast majority of options are stored in the alloptions cache.

Why is this an issue?

The alloptions cache is loaded very early in the WordPress request lifetime. From this moment until the end of a request, option reads are taken from this cache rather than the database.

In addition, every time an autoloaded option is updated or deleted, alloptions is similarly updated. However, this means that on any autoloaded option being updated, every autoloaded option has its value set to the value at load time.

This is a huge concurrency bug, as it's very easy to run into accidentally. As a proof of concept, try updating two different options in two different requests at the same time, and you'll see that whichever ran first will lose the changes. (See #25623)

(We're running into this issue a few times a week, due to certain plugins running an update_option on a high percentage of requests. These plugins are obviously being dumb in doing so, but they're revealing the core bug quite nicely.)

How do we fix this?

The biggest issue is that alloptions is a single cache item containing every option. If we reduce this to one cache item per option, the concurrency problem is then reduced to individual options (and can be mitigated in the places it actually matters, typically in plugins). Rather than storing all the options, we can instead cache the keys that matter, then grab the values for all of those at once.

This means that we may still have minor concurrency issues when adding and removing options, however these happen much less often than updating existing ones. In addition, the cache would now only be used to decide which options to load at the start of the request, so worst case scenario, you end up with extra object cache get calls. This is much better than the current case, where a concurrency problem can return incorrect data.

This change would require adding a wp_cache_get_multi to take full advantage of the cache, however compatibility could be shimmed here with a loop and individual calls to wp_cache_get. This would cause worse performance for object caches that don't support multi-get (or haven't been updated), but at the benefit of no longer returning potentially bad data.

There's a few things that need to be done for this:

  • Add wp_cache_multi_get, and talk to object cache maintainers to update the popular ones to support it (Memcache, APC)
  • Change wp_load_alloptions to pull from alloptionskeys, then run a multi-get on those
  • Ensure alloptionskeys is a protected option name
  • Store value caches per-key

Attachments (4)

31245.diff (7.4 KB) - added by rmccue 2 years ago.
Initial patch to replace alloptions with alloptionskeys
31245-1.diff (7.1 KB) - added by tollmanz 20 months ago.
Refresh of rmccue's patch
31245.2.diff (42.9 KB) - added by johnjamesjacoby 3 months ago.
WP_Option, WP_Option_Query, and more
31245.3.diff (4.2 KB) - added by spacedmonkey 3 months ago.

Download all attachments as: .zip

Change History (58)

#1 @rmccue
2 years ago

  • Owner set to rmccue
  • Status changed from new to assigned

@rmccue
2 years ago

Initial patch to replace alloptions with alloptionskeys

#3 @rmccue
2 years ago

  • Keywords has-patch needs-testing added

Added a first patch which:

  • Caches autoloaded option keys in alloptionskeys
  • Adds alloptionskeys to the protected names list
  • Sets both alloptionskeys and the individual option caches if alloptionskeys isn't found during load
  • Changes add_option and delete_option to add/remove keys from alloptionskeys
  • Changes add_option, update_option and delete_option to use individual cache items for each option

This fixes #28701 in the process.

I'm not entirely sure on the _wp_cache_compat_get_multi naming/usage pattern here, but doing it in a compatibility file requires us to either define the function inside wp_start_object_cache or in a new file after wp_start_object_cache but before default-filters.php is loaded. Thoughts?

I'd like to get more testing in on this one too. I've tested locally with WP's default cache, and I have a patched version of Memcache ready to go too, but needs a bit more testing still.

This ticket was mentioned in Slack in #core by rmccue. View the logs.


2 years ago

#5 @rmccue
2 years ago

Also worth noting: the format of wp_cache_get_multi conflicts with rboren's original memcached plugin, wonderboymusic's Memcached Redux as well as tollmanz's PECL Memcached object cache. These are all mutually incompatible as well, so that's fun.

I'm happy to change the format of the proposed wp_cache_get_multi here if we want, but let's pick one and stick with it. :)

#6 @rmccue
2 years ago

I'd just like to note down the formats out there in the wild already.

Note: in the below, group is a normal group name, like options, id is a cache key like alloptions, and key is an automatically generated key like $salt:$db_prefix:$group:$id

Original memcached

Input:

get_multi( array( $group => array( $id, $id2, ... ), $group2 => array( $id3 ), ... ) )

Output:

array( $key => $value, $key2 => $value, $key3 => $value )

Available as: $wp_object_cache->get_multi

PECL Memcache

Input:

get_multi( array( $id, $id2, $id3, ... ), array( $group, $group, $group2, ... ) )

Output:

array( $key => $value, $key2 => $value, $key3 => value )

Available as: $wp_object_cache->getMulti and wp_cache_get_multi

Memcached Redux

Input:

get_multi( array( $id, $group ), array( $id2, $group ), array( $id3, $group2 ) )

Output:

array( $value, $value, $value )

(Note that the output order is identical to the input order, however it is a
list, not an associative array.)

Available as $wp_object_cache->get_multi and wp_cache_get_multi


Anything that uses $key is pretty useless for keying, as they're internal keys
and implementation changes depending on the cache. You'd also need to pull them
back out again. You can almost treat them as ordered to use it, but as far as I
can see, they don't guarantee this.

This ticket was mentioned in Slack in #core by rmccue. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by drew. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by johnbillion. View the logs.


2 years ago

#11 @johnbillion
2 years ago

  • Keywords 4.3-early added
  • Milestone changed from 4.2 to Future Release

This needs to go into a cycle early so we can coordinate with the existing caching plugins which implement get_multi(), and to get some solid testing in lieu of unit tests.

#12 @jipmoors
2 years ago

Suggestions for function name: wp_cache_get_mutiple or wp_cache_get_batch.

The thing that really bugs me about implementing this is the 'default' group that is virtually hidden in the cache API at this point because it's set by default when no group is supplied.

I think that return value should be something in line of $return[$group][$key] = $value which allows for easy identification what data belongs where. Also missing keys would just be missing from the return array.

Though allowing for empty default group would result in 'default' as return group, so not a 1-on-1 expectation.

That said.

I am currently in the process of concepting a WP_Cache_Control handler on which Handlers can be registered. This would make it possible to have cache fallbacks/fallthrough and no longer have the need for wp_using_ext_object_cache.

Note: this concept will remove the need for the _wp_cache_compat_get_multi.

Will post a patch on #22661 in the near future.

Voting for putting this on the Cache API instead of Options, Meta APIs.

This ticket was mentioned in Slack in #core by johnbillion. View the logs.


2 years ago

#14 @obenland
2 years ago

  • Keywords 4.3-early removed
  • Milestone changed from Future Release to 4.3

#15 @obenland
2 years ago

  • Keywords changed from has-patch, needs-testing to has-patch needs-testing

@rmccue, no movement in 6 weeks. Do you think two more weeks will make the difference or should we move it to Future Release?

#16 @rmccue
2 years ago

  • Keywords 4.4-early added
  • Milestone changed from 4.3 to Future Release

As @johnbillion says, we need a full cycle for this to soak. Pains me to say it, but let's bump.

This ticket was mentioned in Slack in #core by helen. View the logs.


2 years ago

#18 @tellyworth
23 months ago

I've been looking into whether we can adapt this for WordPress.com, and I have some concerns, mainly about the wp_cache_set() loop.

The wp_cache_set() loop would cause many set calls instead of one. Depending on traffic patterns that could significantly increase the load on the cache, especially on large multisite installs. Adding a wp_cache_set_multi() function would fix that. Unfortunately the pecl memcache extension only supports single gets http://php.net/manual/en/memcache.set.php, so that's not a solution for us or anyone else who relies on it. (The memcached extension appears to support set multi, but internally it might just iterate over set, see http://php.net/manual/en/memcached.setmulti.php).

I'm looking into whether we can simplify things a bit and make it filterable, perhaps combined with some of the ideas in #23330 (specifically, making notoptions optional).

#19 @tollmanz
20 months ago

I'm looking into this and with some help from @rmccue, I can reliably reproduce this locally. I've seen the results of this bug in production environments, but can now reproduce it locally. I want to document this here for when I forget and for others who venture into this place.

Prerequisites:

  1. Using WP CLI, insert an autoloading option
wp option add this-damn-bug drives-me-nuts
  1. Open two terminal windows and use WP CLI to invoke a WP shell session in both
wp shell
  1. In the first window, update the option:
update_option( 'this-damn-bug', 'is-fixed' );
  1. In the second window, add another, unrelated option:
update_option( 'this-bug-sucks', 'really-hard' );
  1. Close both WP shell sessions.
  2. Get the value of the original option:
wp option get this-damn-bug # Returns "drives-me-nuts"
wp cache get alloptions options | grep this-damn-bug # Returns "drives-me-nuts"
wp dq query
mysql> SELECT option_value FROM wp_options WHERE option_name = 'this-damn-bug' LIMIT 1; # Returns "is-fixed"

The result is that the object cache is unchanged, yet the DB value is changed.

Last edited 20 months ago by tollmanz (previous) (diff)

#20 @rmccue
20 months ago

Note: this is possible to trigger in any context, not just wp-cli. wp-cli makes it easy to test though, since it has long-running sessions: the second "request" (wp shell here) sets alloptions back to whatever it was when the process started.

@tollmanz
20 months ago

Refresh of rmccue's patch

#21 @jeremyclarke
20 months ago

Argh! I guess this isn't going to be fixed in the 4.4? We're using the Redis object cache (just to add a little flavor) and this bug is causing white-screen-of-death on sites that it affects until the cache is cleared (which is a huge hit to our server as it stores multiple sites but you can only clear them en-masse).

Alternate idea

Currently when you change an autoloaded option it updates the option cache with old-alloptions+your-change, and the problem is old-alloptions might already contain out of date values. So here's my question:

Why not just delete the alloptions cache completely, rather than updating it. It's described in this ancient+closed forum thread:

https://wordpress.org/support/topic/caching-of-options?replies=2

It wouldn't be as fast as the current way, but would avoid the race condition (since subsequent pageloads would get a fully fresh copy). The downside is that alloptions needs to get totally reloaded, but loading it in is fast anyway right?

AFAICT the problem is that a system which fetched or saved options to the cache individually would be much much slower, but the system for getting/setting them together is fast. This implies that we should still get decent performance letting the whole cache be reloaded at once even if it happens more than we'd like.

Dumping the whole cache when a field is updated is simple and seems like it would solve the problem without any concerns about compatibility with the "multi" options of other plugins. This is especially important once you factor in non-mecached servers, since e.g. Redis has very different support for bulk handling of keys.

#22 @joehoyle
20 months ago

#25623 was marked as a duplicate.

This ticket was mentioned in Slack in #core by joehoyle. View the logs.


20 months ago

This ticket was mentioned in Slack in #core-restapi by rmccue. View the logs.


20 months ago

#25 follow-up: @joehoyle
20 months ago

The more I think about this, the more I think this won't be suitable for core, as it's going to be a lot slower in situations where the object cache can't handle a get/set multi (as pointed out by @tellyworth).

The approach of allowing the cache drop-in to support it's own alloptions gettings / setting would be better. Right now, this takes a crazy amount of hacking to do.

#26 @jeremyclarke
20 months ago

@joehoyle Don't want to give up on this ticket but willing to rename it/migrate it elsewhere since I agree with you that the current title "Replace alloptions with a key cache" isn't the right approach.

Joe what do you think of solving this by clearing the alloptions cache at the right times (triggering a reload from the db) rather than micro-managing the fields that were just changed?

In our testing it has proved very effective at avoiding the race condition entirely. When a process saves an autoloaded option we run wp_cache_delete('alloptions', 'options'); and the next time an option is fetched WP does the single query to reload them (including any changes made by other processes).

Specifically this solution solves the reproduction steps laid out by @tollmanz (Thanks so much for that BTW, made our testing much easier!).

Here is a filter that solves the problem for us and relies only on what's already in core:

/**
 * Deletes the alloptions cache value whenever an option is modified. 
 * 
 * This code triggers a cache delete even when non-autoloaded options are saved because
 * we don't have easy access to check if the option is autoload=1 or not
 * 
 * Ideally this should be done in core itself, only when alloptions needs to be updated.
 */
 function gv_redis_object_cache_delete_alloptions() {
 	wp_cache_delete('alloptions', 'options');
 }
 add_action('added_option', 'gv_redis_object_cache_delete_alloptions');
 add_action('deleted_option', 'gv_redis_object_cache_delete_alloptions');
 add_action('updated_option', 'gv_redis_object_cache_delete_alloptions');

For anyone suffering from the WSOD bug like we were this is probably a worthwhile solution, but as stated in the phpdoc, it's not at all ideal and will result in resetting the alloptions cache more often than necessary.

Core would be in a MUCH better position to solve this directly while handling the alloptions cache and knowing whether the option being saved is autoloaded or not.

It can also be fixed at the object-cache.php level, which is the solution we're currently using. By adding a filter on the SET command in our Redis object cache we can run wp_cache_delete('alloptions', 'options'); only when the actual alloptions cache is updated. This seems insane/ironic but works very well to fix this problem and seems unlikely to cause any serious performance problems.

Quick note on stupid plugins

FWIW for anyone experiencing problems related to alloptions I recommend doing an audit of the ongoing actions taken by your object cache. I added logging to our object-cache.php and found several plugin features (some by me, some from repo) that were triggering option changes on every pageload for foolish reasons. Cleaning up these noisy/constant updates will probably make a huge difference to the incidence of {{[alloptions}}} bugs and obviously reduce the overall load on your system.

This ticket was mentioned in Slack in #core by mikengarrett. View the logs.


20 months ago

This ticket was mentioned in Slack in #core by jeremyfelt. View the logs.


19 months ago

#29 @dd32
18 months ago

  • Keywords early added; 4.4-early removed

Moving forward here, I think it's going to be fairly tough getting a viable multi_get() cache implementation in place for the majority of cache-enabled sites without significant detriment to the rest of the sites, as @tellyworth points out.

what do you think of solving this by clearing the alloptions cache at the right times

@jeremyclarke Simply deleting the cache bucket isn't entirely viable for some users, as the next page load has the potential to cause a large DB load as many simultaneous requests hit a cold cache. However, that could easily be worked with with the next suggestion:

The approach of allowing the cache drop-in to support it's own alloptions gettings / setting would be better. Right now, this takes a crazy amount of hacking to do.

This seems like a viable option to me, and I'm curious what it'd actually look like @joehoyle - @tollmanz @rmccue what do you think?

#30 @rmccue
15 months ago

  • Keywords early removed
  • Milestone changed from Future Release to 4.6

I am going to tackle this, and we are going to solve it this cycle.

This ticket was mentioned in Slack in #core-multisite by rmccue. View the logs.


14 months ago

#32 @boonebgorges
14 months ago

There is an old ticket for introducing multi-get to the cache API: #20875. I noted in https://core.trac.wordpress.org/ticket/20875#comment:19 that we have use cases for multi-get outside of the 'alloptions' fiasco. So I'd like to suggest that we pursue multi-get support independent of whether and how it's used in the current ticket.

#33 @rmccue
14 months ago

@boonebgorges Along the lines of your "I would hate to see the Cache API improvement held up because we can't come to a consensus about alloptions" comment, @joehoyle and I have been discussing this ticket, and we want to try and push forward on it without the multi-get support. Right now, the ticket is held up because we don't have consensus on multi-get.

I believe @joehoyle was planning on working on a patch for this that doesn't require multi-get support. I'll let him weigh in on that.

(n.b. He is currently in the middle of the Atlantic, so responses may be delayed.)

This ticket was mentioned in Slack in #core by voldemortensen. View the logs.


14 months ago

#35 @voldemortensen
14 months ago

  • Milestone changed from 4.6 to Future Release

Punting. If a working patch is produced so this can be brought back into the milestone.

This ticket was mentioned in Slack in #core by danielbachhuber. View the logs.


9 months ago

#37 in reply to: ↑ 25 ; follow-up: @danielbachhuber
9 months ago

Replying to joehoyle:

The more I think about this, the more I think this won't be suitable for core, as it's going to be a lot slower in situations where the object cache can't handle a get/set multi (as pointed out by @tellyworth).

The approach of allowing the cache drop-in to support it's own alloptions gettings / setting would be better. Right now, this takes a crazy amount of hacking to do.

@joehoyle Did you ever get to a working patch for this? WP LCache exhibits the race condition issue more severely than most drop-ins, so I need to start working on an alternative.

At this point, I'm thinking the alternative alloptions implementation could be a library bundled by the object cache drop-ins that want to support it.

#38 @johnjamesjacoby
9 months ago

Not to hi-jack, but for medium-sized multisite installls, I started using this approach:

https://github.com/stuttter/wp-blog-meta

It has the same scalability concerns as any global table does, but allowed me to circumvent this issue as options can be short-circuited and rerouted ontop of the meta-data API & caching layer.

#39 in reply to: ↑ 37 @dd32
9 months ago

Replying to danielbachhuber:

At this point, I'm thinking the alternative alloptions implementation could be a library bundled by the object cache drop-ins that want to support it.

Honestly, I think that's the correct answer here in the end, although I don't think you should library it, it should just be a standard part of the object cache.
The object cache has freedom to route get/set for alloptions however it wants, it can route it as a standard request, or it can route it through a multiget.
If core adds something in the future that's better, object caches can implement that, or their existing method may continue to work.

#40 @rmccue
9 months ago

We changed our object cache implementations to hijack alloptions (Memcache and Memcached).

The problem with this approach is that it's pretty terrible handling without WordPress helping out more. The object cache receives an opaque heap of data that then needs to be parsed back out, diffed, and saved correctly.

There needs to be changes in WordPress core for this. I can't remember the exact approach we discussed, maybe @joehoyle can.

#41 @danielbachhuber
9 months ago

Thanks @rmccue, those examples were quite helpful.

For the next person to run into this, here's WP LCache's implementation (with tests). I didn't find splitting alloptions to be horrible, but having to keep record of the alloptions keys in a separate cache does seem a bit dodgy.

So it's mentioned, WP LCache doesn't absolutely need to have a multi get implementation because most cache gets will be hitting its L1 cache (APCu). Other drop-ins will need to implement multi get as well to mitigate the performance impact of introducing hundreds of new cache keys.

This ticket was mentioned in Slack in #core-multisite by jjj. View the logs.


6 months ago

#43 @johnjamesjacoby
6 months ago

For multisite folks who stumble in here, consider #25344/#37181 and #37923.

  • Using the meta-data API for network options
  • Using the meta-data API for a shared wp_blogmeta global table, to store meta-data for sites that may be useful to query against (names, descriptions, post/user counts, last activity, etc...)

/tangent

Last edited 6 months ago by johnjamesjacoby (previous) (diff)

This ticket was mentioned in Slack in #core by sergey. View the logs.


5 months ago

#45 @johnjamesjacoby
5 months ago

Another really out-there idea:

  • A WP_Option class
  • A WP_Option_Query class for querying options
  • Introduce a wp_optionmeta database table - key/value storage for every option
  • Use the metadata API for the above (inherit caching, etc...)

Treat options like post objects:

  • Query for them
  • Cache them (locally & persistently)
  • Query for their option-meta as needed
  • Abandon the alloptions logic entirely
  • Let the cache API sort it all out ¯\_(ツ)_/¯

Then core could:

  • Transition some keys from wp_options into a new WP_Option object with related meta
  • Route requests for converted keys from the old option name to the new option name
  • Have a legitimate register_option() API to begin to replace the ailing Settings API

Conversely:

  • Abandon wp_options completely
  • Dream up a completely new and adequate object & schema for today's needs
  • Consider the same object & meta pairing as above

(Edit: typos & grammar)

Last edited 5 months ago by johnjamesjacoby (previous) (diff)

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


3 months ago

@johnjamesjacoby
3 months ago

WP_Option, WP_Option_Query, and more

#47 @johnjamesjacoby
3 months ago

31245.2.diff is my first-pass patch at using option_name as the cache-key. It includes the following changes to accomplish this:

  • A WP_Option class, with variables to match database columns, and magic methods to help with routing variable names around (I found these personally useful during the research/discovery/invention/transition phases)
  • A WP_Option_Query class, used to interact with the wp_options database table in a way that is more like the query classes we are using/building today, vs. the direct bespoke MySQL statements we've relied on forever previously
  • A few common helper functions for interacting with objects in key caches (wp_clean_option_cache(), etc...)
  • Usage of a new notoptions cache group, for queries to wp_options for options that do not exist there.
  • I'm currently caching full option objects inside of the results of a WP_Option_Query. This is not much different than the alloptions cache we have today, only we aren't actually referring back to it for anything directly anymore. We could switch this to only cache option_names or option_ids if we want to limit the memory footprint of that cached item, but since we aren't directly peeking into it, I'm not sure that it matters very much one way or the other.

Notables:

  • A product of all of this is the removal of the need to call wp_load_alloptions() on every call to get_option(). Some changes were made to unit tests to accommodate for this by calling it directly inside those offending tests. It's likely not ideal to do this in our tests, but the significant performance boost seems worth it to leave in for my first pass.
  • All other tests pass for me except test_wp_get_attachment_image_should_use_wp_get_attachment_metadata which somehow seems unrelated
  • I made other code changes to these and related functions, but I did try to be conservative and stick only to lines of code that were directly effected or near lines that were.
  • I've probably spent close to 100 hours massaging this patch, and I ran into many significant snags along the way to get this to work in a backwards compatible way.
  • One design compromise was to mock a WP_Option object to save in a new notoptions cache group. It's /easier/ (and likely makes more sense) to just save the option_value, but doing so increases the complexity of the logic in PHP to a degree that doesn't seem to actually make anything easier or better for ourselves.
  • The lack of continuity of our pre_ filters in these functions makes it completely impossible to improve the alloptions/notoptions cache problem without extremely elaborate filters on query and so on. Normally I'd prefer to make a plugin as a proof-of-concept, but that is unfortunately impossible here as this API is coded today (and my patch does not make any effort to improve that, though it isn't any more difficult either.)
  • Please be open and critical, and scrutinize this deeply. I'm too close to the concept now to make many significant improvements to this patch alone, but I'm convinced that something similar to this approach is the ideal solution today (beyond schema changes, wp_optionmeta, etc...)
  • The way that options.php is included at the top of functions.php is weird to me, but I rolled with it here.

Thank you in advance for taking any time to review my patch. 💜

Last edited 3 months ago by johnjamesjacoby (previous) (diff)

#48 @flixos90
3 months ago

I really like this proposed change. I'm not sure about the usage of metadata for options, but this can be discussed separately. Having distinct classes for options and a better way to query and cache them is a major improvement, not only for the obvious issues, but also for allowing developers to query options more flexibly for all the edge-cases where they currently make direct DB requests. :)

I did a first review on 31245.2.diff and have the following comments:

  • Why does line 236 of class-wp-option-query.php check whether multisite is disabled? Not saying this is wrong, but I'd like to know what the background for doing that is.
  • The query vars for querying by option ID should either be called simply option, option__in and option__not_in (or alternatively id, id__in and id__not_in). This ensures they align with similar names from other Core query classes. I think this change is especially important since the option_id column is not the "actual" property in WP_Option (it is just id which is good!), so I don't think that name should be used anywhere.
  • I'd prefer for WP_Option::get_instance() to have a different name. Other Core object classes already contain a get_instance() method that accepts an ID and returns the instance for that ID. WP_Option::get_instance() is more flexible though, as it behaves more like functions such as get_post() or get_site(). Therefore I think we should give it a different name, so that we would theoretically be able to add a method with similar flexibility to the other Core object classes that has the same name. I'm thinking about simply using WP_Option::get(), just to be future-proof (think about a possible WP_Post::get() as replacement for get_post(), or WP_Site::get() as replacement for get_site()).
  • I don't think we need to support an array being passed to the WP_Option constructor. No other Core object class supports this, and supporting that only adds a bit of unnecessary complexity to the logic.
  • I like the idea of having properties with more sane names in WP_Option, however it appears there are some issues with setting them. The constructor simply sets the get_object_vars() keys from the database result, which would not be the properties that the class declares. We need to make sure the correct properties are set by mapping $option_id to $id, $option_name to $name and $option_value to $value.
  • I'm not sure we should even support the original database column names through magic properties. There are no backward-compatibility concerns as this is new. :) Especially the magic ID property should not be supported. It has never been associated with options, and at some point in the Core chat it was decided to go with id as property name for an ID wherever possible. Due to those reasons, I think no magic methods should be present in the class.
  • Currently full WP_Option objects are stored in cache. I think it's fine to store the full objects, but it should be plain stdClass objects, as storing the entire class instances can cause unexpected problems. Some logic could be added wp_update_option_cache() to ensure that.

#49 @spacedmonkey
3 months ago

My feedback

  • Remove check if is_multisite() in WP_Option_Query
  • wp_option:: get_instance should work like get_instance methods in core using id. Another method called get_by_name should be added.
  • wp_option:: get_instance $use_cache could be removed and a filter of 'option_use_cache'. When installing add_filter( 'option_use_cache', '__return_true');
  • Set no_found_rows to false but default. Pagation will not be needed in most use cases.
  • if ( ( 'home' === $option ) && empty( $value ) ) { should go back if ( 'home' == $option && '' == $value ) . There might be good reason to have a false value in database.
  • populate_options should use WP_option_query and add_option.
  • It is a shame that we have to store the whole object in cache. It would be better to just store the option_ids and use something like wp_cache_get_multi.

Related: #33958 #37930 #40523 #20875

#50 @spacedmonkey
3 months ago

In 31245.3.diff I try something a little different. I really like the changed in 31245.2.diff however, I don't think that is fixes the race condition, where autoloaded options revert. In my patch just deleted the alloptions cache value after add/update/delete options. It treats alloptions cache as poisoned on update and deletes it. I believe this is the best and most simple solution to this error. It is also how the meta api works. https://github.com/WordPress/WordPress/blob/master/wp-includes/meta.php#L107

Delete the alloption cache brings some problems. It does mean that next time you interact with options, it will call load_all_options again, resulting in another query. This added query broke a lot of the tests, so I had to remove tests that do query counting. Query counting as a test feel like a bad way to test.

As for WP_Option and WP_Option_Query, this is great work and should be broken into another ticket.

Last edited 3 months ago by spacedmonkey (previous) (diff)

#51 @rmccue
3 months ago

I am personally strongly -1 on introducing WP_Option. Options are properties of sites, not fully-fledged objects. I am likewise strongly -1 on introducing wp_optionmeta - to me, it'd be similar to introducing a wp_postmetameta.

I'm ambivalent towards introducing a WP_Option_Query; I'm not sure options really need the full complexity that comes with a query class. That said, I think there is some utility in it, so I'm not against it.

I am very strongly against treating options as objects however. Similar to how the meta tables are key-value stores for their parent objects (i.e. postmeta is a key-value store for the parent post), options are a key-value store for the parent site. The only real additional property that options have in the database (autoload) is a concession for performance, not a real additional properties. (The ID property has been functionally useless since 1.5, and apart from upgrade routines for v1.5 and v2.9, is never used.)

#52 @rmccue
3 months ago

(On the ID point: IDs were effectively deprecated in #2699 (2.9), and were only kept around for plugin compatibility, so options has been enforced as a key-value store since then.)

#53 @spacedmonkey
2 months ago

My recommendation is the following.

We use my patch 31245.3.diff , as it fixes the race condition documented at above. It is a tidy solution as it doesn't require the introduction of get_cache_multi and possible breaks. I would like @rmccue & @joehoyle to take a look at my patch, as I know it is issue they have looked and fixed outside of core.

As for WP_Option_Query and WP_Option, I like standardising interfaces in core. WP_Option_Query feels like a good fit for the api, as there is value in search for options by name. WP_Option_Query has some nice filters and should replace the internals of wp_load_alloptions. For that reason, WP_Option_Query work should be moved to #33958 .

#54 @jeichorn
7 weeks ago

My concern with 31245.3.diff is the performance hit. Our redis object cache at Pagely works around the race condition already, this gets rid of my ability to keep our performant implementation.

Object cache authors can already add this solution to the race if they don't' want something fancier.

Note: See TracTickets for help on using tickets.