Make WordPress Core

Opened 8 years ago

Last modified 18 months ago

#37181 reopened enhancement

Use metadata api in *_network_options

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile spacedmonkey
Milestone: Future Release Priority: normal
Severity: normal Version: 4.4
Component: Networks and Sites Keywords: ms-roadmap
Focuses: multisite, performance Cc:

Description

The network (site) options are stored in the database as sitemeta. The table is formatted as a meta table. However the CRUD of this data in get_network_option, update_network_option, add_network_option and delete_network_option doesn't use the metadata api. Using the metadata api has many advantages, such as filters and a more consistent caching api.

Attachments (15)

37181.diff (9.9 KB) - added by spacedmonkey 8 years ago.
37181.1.diff (9.9 KB) - added by spacedmonkey 8 years ago.
37181.2.diff (14.0 KB) - added by spacedmonkey 7 years ago.
37181.3.diff (14.2 KB) - added by spacedmonkey 7 years ago.
37181.4.diff (18.1 KB) - added by spacedmonkey 7 years ago.
37181.5.diff (17.9 KB) - added by spacedmonkey 7 years ago.
37181.6.diff (23.8 KB) - added by spacedmonkey 7 years ago.
37181.7.diff (23.6 KB) - added by spacedmonkey 7 years ago.
37181.8.diff (23.6 KB) - added by jeremyfelt 6 years ago.
37181.9.diff (20.1 KB) - added by spacedmonkey 6 years ago.
37181.10.diff (22.2 KB) - added by spacedmonkey 6 years ago.
Screenshot 2022-04-07 at 14.30.45.png (3.4 MB) - added by spacedmonkey 2 years ago.
Before patch applied
Screenshot 2022-04-07 at 14.31.47.png (3.4 MB) - added by spacedmonkey 2 years ago.
After patch is applied
Screenshot 2022-04-07 at 14.36.47.png (17.3 KB) - added by spacedmonkey 2 years ago.
Before patch ( 14 queries to site table )
Screenshot 2022-04-07 at 14.37.10.png (47.0 KB) - added by spacedmonkey 2 years ago.
After patch ( 14 queries replaced with 1 query )

Change History (100)

@spacedmonkey
8 years ago

@spacedmonkey
8 years ago

#1 @swissspidy
8 years ago

  • Keywords has-patch needs-unit-tests added

#2 @swissspidy
8 years ago

Note that the default_site_option_ filter needs to be kept in order to not break BC. This includes the documentation for this hook.

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


8 years ago

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


8 years ago

#5 @sc0ttkclark
8 years ago

This could utilize the stuff in WP 4.6 for register_meta() now too:

register_meta( 'site', 'my_site_option_name', array( … ) );

For testing, you'd also need to add 'site' to the array in the new wp_object_types filter. However, in the patch, you'd add 'site' in the wp_object_types array within wp_object_type_exists() in the wp-includes/functions.php file.

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


8 years ago

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


8 years ago

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


8 years ago

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


7 years ago

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


7 years ago

#12 @spacedmonkey
7 years ago

I have been looking at this ticket again. There seems to be existing unit tests for the *_network_option functions. I believe me and @jeremyfelt worked on those original tests. The tests seem good to me. We will need some more test, but I would like some advise on what those tests look like.

Related: #28290

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


7 years ago

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


7 years ago

#15 @flixos90
7 years ago

Briefly summarizing today's office-hours proposed goals:

  • Introduce *_network_meta() functions using the Metadata API for direct interaction with the sitemeta table.
  • Add support for meta_query in WP_Network_Query.
  • Use the new functions in the *_network_option() functions. This gives performance benefits and allows us getting rid of the alloptions mess. The *_network_option() functions will continue to maintain their current hooks for BC, while the *_network_meta() functions will be clean as other object metadata functions.
  • Encourage using the new functions and use *_network_meta() in Core going forward. The *_network_option() functions will stay around for BC though, and should not be deprecated.
  • This will also benefit a future /networks REST API endpoint.

See #37923 for related plans of introducing a blogmeta table and surrounding *_site_meta() functions.

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


7 years ago

#17 follow-up: @johnjamesjacoby
7 years ago

I'd like to make a motion to close this ticket in favor of recent activity on #25344.

I believe doing so will concentrate our efforts and be a bit simpler for everyone to pay attention to.

#18 in reply to: ↑ 17 @flixos90
7 years ago

  • Keywords close added

Replying to johnjamesjacoby:

I'd like to make a motion to close this ticket in favor of recent activity on #25344.

I believe doing so will concentrate our efforts and be a bit simpler for everyone to pay attention to.

+1

#19 @spacedmonkey
7 years ago

  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #25344.

#20 @swissspidy
7 years ago

  • Milestone Awaiting Review deleted

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


7 years ago

#22 @spacedmonkey
7 years ago

  • Keywords ms-roadmap added; close removed
  • Resolution duplicate deleted
  • Status changed from closed to reopened

#23 @spacedmonkey
7 years ago

  • Milestone set to Future Release

@spacedmonkey
7 years ago

#24 @spacedmonkey
7 years ago

In 37181.2.diff are the following changes

  • wp_load_core_site_options now uses get_current_network_id to bring into line with _network_option functions.
  • serialize functions returned to _network_option functions which was stopping tests from passing.
  • delete_network_option uses delete_metadata and passes 4 param as an empty string, as per meta api.
  • meta.php and formatting.php moved to earlier in bootstrap process because network options loaded ealier in bootstrap.
  • Test added to prove that first value in meta store is honored.
  • New update_network_meta_cache param added to wp_network_query. Network options are primed in cache, like other query classes.
  • Removed test_site_notoptions test, as no longer valid.
  • Documentation. Poke @DrewAPicture for feedback.

#25 @spacedmonkey
7 years ago

  • Keywords dev-feedback has-unit-tests added; needs-unit-tests removed

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


7 years ago

@spacedmonkey
7 years ago

@spacedmonkey
7 years ago

#27 @spacedmonkey
7 years ago

In 37181.4.diff

  • Introduce the cache group of site_meta as a global group
  • Include site_meta in clean_network_cache

@spacedmonkey
7 years ago

@spacedmonkey
7 years ago

#28 @spacedmonkey
7 years ago

37181.6.diff

Fixes some styling issues with the comments and the code. It now is WP code standards. Also there has been changed to variables since original patch.

One thing to note about this change how the meta api handles caching. The meta api loads all the meta for an object into one cache key. This means, when you request one option, it then loads all options in memory / object cache. Personally, I think this is a good thing, you are effectively autoloading all options, similar to how options works in core. But it may have effects on performance in different use cases. In WP_Network_Query, the network option (Meta) cache is primed. It is reasonable to guess, that if you are query networks that you will likely want the meta as well. As Network query is used in the bootstrap, the network option will be cache will be primed much easier.

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


7 years ago

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


7 years ago

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


7 years ago

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


7 years ago

#34 @spacedmonkey
7 years ago

  • Milestone changed from Future Release to 5.0

@spacedmonkey
7 years ago

#35 @spacedmonkey
7 years ago

In the last patch, is a small performance improvement.

Unsetting the following
update_network_cache
update_network_meta_cache

Means cache key is unchanged on these query variable

#36 @spacedmonkey
7 years ago

  • Component changed from Options, Meta APIs to Networks and Sites

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


7 years ago

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


7 years ago

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


6 years ago

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


6 years ago

@jeremyfelt
6 years ago

#41 follow-up: @jeremyfelt
6 years ago

Thanks for your work on this, @spacedmonkey.

37181.8.diff is updated against current trunk to account for the changes in [42343].

A couple notes so far:

  • We're switching from site-options to site_meta as the cache group, but still using site-options for a handful of other things—current_network, networks_have_paths, etc... This is probably okay because we weren't ever storing those "options" in the DB.
  • We'll want to make sure once this is in that external object cache drop-ins are aware/updated with the new global cache group.
  • In general, I think our network option tests could use some work. For something like this we should write some more tests that rely on an external object cache.
  • Is it possible that anyone is relying on notoptions right now for some reason? May need to do some digging through plugins briefly to see if anything stands out.

I'll spend some more time with this and think through the testing a bit more.

#42 @jeremyfelt
6 years ago

One more - it'd also be helpful to know if anyone is relying on the site-options cache group for one of these values in their sunrise.php or elsewhere with a direct call to cache.

#43 in reply to: ↑ 41 @spacedmonkey
6 years ago

Replying to jeremyfelt:

  • We're switching from site-options to site_meta as the cache group, but still using site-options for a handful of other things—current_network, networks_have_paths, etc... This is probably okay because we weren't ever storing those "options" in the DB.

I think we need to support the site-options cache group forever more. It is used else where in core and I am personally use it for some plugin. I see no harm in leaving it.

  • We'll want to make sure once this is in that external object cache drop-ins are aware/updated with the new global cache group.

Most modern object cache drops implement the wp_cache_add_global_groups, which should do this for us. Examples, can found, here, here, here, here and here. These are the largest plugins that I know of. Older ones may not have it implement it, but that will break other things and is not a core problem.

  • In general, I think our network option tests could use some work. For something like this we should write some more tests that rely on an external object cache.

Any ideas of this @jeremyfelt ?

  • Is it possible that anyone is relying on notoptions right now for some reason? May need to do some digging through plugins briefly to see if anything stands out.

A quick look of github, shows all the refrences to it, are people checking core into GH. I havent found a plugin in the wild that uses

Last edited 6 years ago by spacedmonkey (previous) (diff)

#44 @spacedmonkey
6 years ago

Some things to not

  • All options will be loaded in one cache key. This may effect people with lots of network options, however, not many people use network options, it is unlike to be a problem.
  • Even through all options are loaded at once, it is not effected by the alloptions issue, detailed in #31245 . This because the meta api, does a delete instead of update. See here
  • Network options will now be primed in WP_Network_Query, meaning network options will be loaded

in the bootstrap, which I think is is a good thing.

  • In my patch, I run maybe_serialize before passing to the meta api. Howeve the meta api also serialize. I dont think there is a problem with double serializing something?
  • Many of the existing filters could be moved out of the filters / actions in these could be moved out to there own functions. Consider the following example.
    add_filter('get_site_metadata', function($value, $network_id, $option){
       $pre = apply_filters( "pre_site_option_{$option}", false, $option, $network_id );
       if ( false !== $pre ) {
            $value = $pre;
       }
       return $value;
    }, 5, 3);
    
    

This allows for backward compat, but also if to remove those filters if you don't wish to use them anymore. This may over complex things.

  • clean_network_cache now clears network options, which I think it should be doing already. This means that the delete_network function in wp-multi-network, it work better. ( Good work @johnjamesjacoby on that one ).

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


6 years ago

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


6 years ago

@spacedmonkey
6 years ago

#48 @spacedmonkey
6 years ago

In 37181.10.diff I updated the patch after coding standard changes.

There are now, 3 tests that pass. If you could take a look, @jeremyfelt that would be great :D

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


6 years ago

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


6 years ago

#51 @spacedmonkey
6 years ago

As part of the slack chat on this ticket, I have added a related ticket #43941 for meta data default values.

#52 @flixos90
6 years ago

  • Milestone changed from 5.0 to 5.1

#53 @pento
5 years ago

  • Milestone changed from 5.1 to Future Release

This ticket needs reviewing and a decision.

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


4 years ago

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


4 years ago

#57 @spacedmonkey
3 years ago

  • Focuses performance added

#60 @spacedmonkey
2 years ago

I spent some time on this ticket refreshing it so that is applies clearly to core. There is a new ready for review at #2478.

@spacedmonkey
2 years ago

Before patch applied

@spacedmonkey
2 years ago

After patch is applied

@spacedmonkey
2 years ago

Before patch ( 14 queries to site table )

@spacedmonkey
2 years ago

After patch ( 14 queries replaced with 1 query )

#61 @spacedmonkey
2 years ago

Spent some more time on this ticket lately. I think this patch should go into core for the following reasons.

  • Priming caches for all network options in one query, reduces page load time and number of queries run on the page. See attached screenshots, that removed up to 14 queries per page ( in network admin ). This is a massive performance win.
  • With improvement to the register_meta function in earlier versions of WP, it now means that default values for a meta key can defined globally. Meaning that a network option could be register with a default value and type. There are a series of filter in the meta api that would be extremely useful for handling network options ( meta ). This opens up option in the future to have functions like register_network_setting ( a wrapper around register_meta ). This will bring network options more inline with options, see register_setting.
  • Remove the need for the 'site-options' cache group.
  • Remove need for notoptions cache.
  • Improve consistency with how other meta types handle meta data.
  • Removal of lots of code.
  • Open door to reuse meta REST API controller, to handle CRUD in network options.

It is worth noting that is change does not close the door to #25344 in the future and is a good first step towards its.

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


2 years ago

#63 @spacedmonkey
2 years ago

  • Milestone changed from Future Release to 6.1

#64 @peterwilsoncc
2 years ago

This may seem tangentially related but I'm going to mention it anyway...

Something I've noticed with using the *_network_option() functions on a single site is that they fall back to the *_option() function. Which makes some sense.

Where it becomes problematic on a single site install is that there is no way to set an option to be autoloaded via these functions. As a result anything using this shortcut ends up requiring a database query per option (again, on a single site install).

A good example of this is the salting values when stored in the database rather than within wp-config.php: this results in four extra database queries on each page load.

My question is, does making this change lock this problem in as unable to be solved?

spacedmonkey commented on PR #2478:


2 years ago
#65

@peterwilsoncc Will confused what what you are getting out there. One mentions blogmeta. There is network options, which is a little different. That difference is important here.

I understand not want to put everything in once cache key. But network options are a little more controlled than options and alloptions. Autoloading options on sites is a performance issue. But that is when you have 500+ options on a site. See this link for more information. As network options are not used as much as they only for network options, network options do not have the same problems as per site options. I think the many benefits here, overcall, this small pitfall.

peterwilsoncc commented on PR #2478:


22 months ago
#66

@spacedmonkey Did you see my note clarifying the backward compatibility break?

On trunk get_site_option() always* returns a string but on this branch numeric values are always returned as an integer.

Notes:
\* Not actually always, see my comment above for the full details
\ always always

#67 @spacedmonkey
22 months ago

  • Keywords commit added
  • Owner set to spacedmonkey
  • Status changed from reopened to assigned

#68 @mukesh27
21 months ago

@spacedmonkey I left nit-pick comments on docblocks that needs to address.

JJJ commented on PR #2478:


21 months ago
#69

tyvm @mukeshpanchal27 💪

(I could've batched these commits, but I wanted to review them manually for my own sake while using the GitHub.com UI to view the diffs.)

#70 @spacedmonkey
21 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 54080:

Networks and Sites: Use metadata api in *_network_options` functions.

Replace logic found in get_network_option, update_network_option and delete_network_option to use the metadata api. Using the metadata api has a number of benefits, such as consistency, default values and useful filters. This change also improves performance by priming the caches of all network options in a single database request.

Props spacedmonkey, swissspidy, sc0ttkclark, johnjamesjacoby, flixos90, jeremyfelt, pento, peterwilsoncc, mukesh27, desrosj.
Fixes #37181

#72 @milana_cap
20 months ago

  • Keywords add-to-field-guide added

#73 @spacedmonkey
20 months ago

Made start on the dev note here.

#74 @milana_cap
20 months ago

  • Keywords needs-dev-note added

#75 @milana_cap
20 months ago

  • Keywords add-to-field-guide removed

#76 follow-up: @milana_cap
20 months ago

  • Keywords has-dev-note added; needs-dev-note removed

#77 @johnbillion
19 months ago

  • Keywords has-patch dev-feedback has-unit-tests commit has-dev-note removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening based on #56845.

#78 @davidbaumwald
19 months ago

  • Keywords revert added

After some discussion, prepping a full revert for now. For the full discussion in Slack, see https://wordpress.slack.com/archives/C03LZ88NX6G/p1666104060129079

#79 @davidbaumwald
19 months ago

In 54637:

Networks and Sites: Revert the use of the metadata API for *_network_options functions.

[54080] refactored the logic in get_network_option(), update_network_option() and delete_network_option() to use the metadata API. However, this change resulted in issues with large multisite installs that utilize memcached having network options > 1MB in size.

This change reverts [54080] and all related follow-up changes.

Reverts [54080], [54081], and [54082]. Partially reverts [54267] and [54402].

Props pavelschoffer, rebasaurus, johnbillion, spacedmonkey, desrosj, rinatkhaziev.
Fixes #56845.
See #37181.

#80 in reply to: ↑ 76 @pbiron
19 months ago

Replying to milana_cap:

Dev note: https://make.wordpress.org/core/2022/10/10/multisite-improvements-in-wordpress-6-1/

I think the Dev note should be updated to mention the revert.

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


19 months ago

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


19 months ago

#83 @desrosj
19 months ago

  • Keywords revert removed
  • Milestone changed from 6.1 to 6.2

The dev note has been updated to include a callout at the top.

Since the 6.1 aspects of this have been addressed, I'm going to leave this open punt to 6.2. There was agreement that this could still be beneficial and deserves further exploration, but would need other related problems to be solved at the same time to be feasible.

Just to bring some thoughts @johnjamesjacoby shared that summarize a great path forward over from Slack for easier reference:

Hello! I’m onboard with reverting for 6.1, and suggest:

  • keeping it open
  • moving to the 6.2 milestone
  • landing it simultaneously with some future meta data cache key improvement to address the forever-old 1MB memcached issue

The major (future) benefit of getting network options using the meta data API is it inherits all improvements from it (in addition to removing the bespoke cache-key implementation of options)

Would love to see (and participate in) a collaborative effort from cache, options, meta, multisite, & performance contributors (of which there is some overlap) to identify, document, and make progress on a roadmap.

#84 @mehulkaklotar
18 months ago

As per discussion in monthly bug scrub for performance team, we need to rethink the solution for this.

#85 @mukesh27
18 months ago

  • Milestone changed from 6.2 to Future Release
Note: See TracTickets for help on using tickets.