WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 9 years ago

Last modified 9 years ago

#2268 closed defect (bug) (fixed)

get_option() on non-existent option always invokes new query

Reported by: markjaquith Owned by: markjaquith
Milestone: 2.2 Priority: high
Severity: normal Version: 2.1
Component: Optimization Keywords: has-patch needs-testing
Focuses: Cc:

Description

get_option() aka get_settings() does not work well when the option queried does not exist. The result ( FALSE ) gets cached to the options cache, but every subsequent time the option is called, the cache is ignored, and the query is re-run. I've seem people run up over a hundred queries on some pages because of multiple calls to get the same option.

Note my comment in this code:

function get_settings($setting) {
	global $wpdb;

	$value = wp_cache_get($setting, 'options');

// !!!!!!!!!!!!!!!!!!!!
// this line below is the problem.  An option that does not exist will === false, because it is stored
// in the cache as false.  And so, the query is run again and again

	if ( false === $value ) {
		if ( defined('WP_INSTALLING') )
			$wpdb->hide_errors();
		$row = $wpdb->get_row("SELECT option_value FROM $wpdb->options WHERE option_name = '$setting' LIMIT 1");
		if ( defined('WP_INSTALLING') )
			$wpdb->show_errors();

		if( is_object( $row) ) { // Has to be get_row instead of get_var because of funkiness with 0, false, null values
			$value = $row->option_value;
			wp_cache_set($setting, $value, 'options');
		} else {
			return false;
		}
	}

Basically, we need a way to discriminate between "option isn't in cache" and "option's non-existence has been cached." I'm open to suggestions.

Attachments (5)

2268.diff (798 bytes) - added by davidhouse 10 years ago.
get_settings_patch.diff (804 bytes) - added by markjaquith 10 years ago.
2688.diff (1.1 KB) - added by davidhouse 10 years ago.
Fixes issue skeltoac pointed out
cache_option_non_existence.diff (1.1 KB) - added by markjaquith 9 years ago.
Patch for trunk
options-caches.diff (5.9 KB) - added by markjaquith 9 years ago.
Alloptions and Notoptions caching. Saves a query, and reduces cache misses to zero

Download all attachments as: .zip

Change History (20)

@davidhouse10 years ago

comment:1 @davidhouse10 years ago

  • Keywords bg|has-patch added

Attached patch is a simple way of doing this just for options: if the option isn't found it just caches it in the notoptions group, which is checked before doing a query. We might need something more generalised, but its a start.

comment:2 @markjaquith10 years ago

+	if (wp_cache_get($setting, 'notoptions')) {
+		return;

This should return false;

check out the corresponding code from 1.5.2:

		if ( isset($cache_nonexistantoptions[$setting]) )
			return false;

comment:3 @markjaquith10 years ago

  • Keywords bg|commit added
  • Owner changed from anonymous to markjaquith
  • Status changed from new to assigned

My patch is the same, but returns false when the option does not exist.

comment:4 @skeltoac10 years ago

  • Keywords bg|has-patch bg|commit removed
  • Milestone changed from 2.0.1 to 2.1

This optimization needs much testing. It is too much for a bugfix.

And let me be the first to find fault! This would happen if an option is checked, set and checked again in the same load:

get_option('foo') returns false, sets 'foo'=>'foo' in 'notoptions'

update_option('foo', 'bar') save in db but leaves 'notoptions'

get_option('foo') finds 'foo' in 'notoptions', returns false

Moving to 2.1 and suggesting a rework.

@davidhouse10 years ago

Fixes issue skeltoac pointed out

comment:5 @davidhouse10 years ago

I don't think an entire rework is necessary. Patch attached to fix issue you pointed out. Only the third patch needs to be applied.

Agreed with the 2.1 milestone, it's not a massive issue anyway.

comment:6 @markjaquith10 years ago

What we should probably do, until this is fixed, is educate plugin authors about their settings. When their plugin management page loads, it should check all the plugins options and add_option() for any of its options that does not yet exist, populating it with a default value. Too many plugin authors have been using options as on/off switches, and assuming "does not exists" == false.

comment:7 @matt9 years ago

  • Milestone changed from 2.1 to 2.2

@markjaquith9 years ago

Patch for trunk

comment:8 @markjaquith9 years ago

  • Component changed from Administration to Optimization
  • Keywords has-patch needs-testing added
  • Priority changed from normal to high
  • Version changed from 2.0 to 2.1

Oldie but a goody! I've freshened this up, and it seems to work just fine. Let's get some testing in on this.

This saves us at least one query for virgin WP installs (Default theme queries options that don't exist at first), and as I stated a year ago, certain themes or plugins can generate dozens and dozens of redundant queries without this fix.

comment:9 @markjaquith9 years ago

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

(In [4798]) Cache the non-existence of options to prevent redundant queries. props davidhouse. fixes #2268

comment:10 @ryan9 years ago

(In [4831]) Remove notoptions caching. Multile rewrite_rules options were being created. See #3692 #2268

comment:11 @markjaquith9 years ago

Ryan, are you sure this was after [4820]? Are the rewrite rules circumventing the options API or was there a bug in the notoptions management? I'd like to get this fixed.

comment:12 @ryan9 years ago

Actually, this may have been due to wpcom specific code. I thought I saw it on WP core, but now I'm thinking it must have been fever delerium. Symptom was rewrite_rules entries being written once for every page in the system. 10 pages would result in 10 rewrite_rules in the options table.

comment:13 @markjaquith9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Maybe Donncha synced wpcom to a revision inbetween [4798] and [4831]

That might do it.

I have another fix for this issue that might be more foolproof. It's certainly easier to code:

Simply use a special string to designate that an option does not exist. e.g. WP_OPTION_DOES_NOT_EXIST. You cache that in the options cache. That could be done purely in get_option(). update_option() and add_option() could stay the same, because they'd just overwrite the WP_OPTION_DOES_NOT_EXIST string when they run wp_cache_set() on that option.

It's not elegant, but it's the KISS solution.

I'll make a patch [4798] + [4831] and test locally to see if saving the rewrite rules causes any funkiness and let you know what I find.

@markjaquith9 years ago

Alloptions and Notoptions caching. Saves a query, and reduces cache misses to zero

comment:14 @markjaquith9 years ago

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

(In [4855]) Introduce Notoptions and Alloptions caching, so that all options (and previously attempted Notoptions) are read from the cache in one go. Should reduce cache misses to zero or close to it.

comment:15 @markjaquith9 years ago

To explain this a bit:

  • Two pseudo-options are stored in the object cache (just there, not in the DB)
  • 'notoptions' stores all attempted non-options so that the DB isn't accessed more than once trying to load these options that don't exist
  • 'alloptions' stores all autoloaded options. The first time any option is queried, 'alloptions' will be populated (either from the DB, or from a persistent object cache). If the option is in there (likely, most options are autoloaded), it'll be read from there. So you just have one cache read for all your autoloaded options.
  • Non-autoloaded options revert to the options cache as before (one item per cache entry).
  • Since the loading of the autoloaded options is used by get_option(), people using alternative object caches will no longer see individual queries for each option on the first load (i.e. with an empty object cache)

All of this should be transparent. As long as you use get_option(), add_option(), update_option(), and delete_option(), it should be like nothing ever happened.

This should reduce cache misses to zero (or close to it), and it should reduce redundant queries.

Note: See TracTickets for help on using tickets.