Ticket #2268 (closed defect: fixed)

Opened 3 years ago

Last modified 2 years ago

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

Reported by: markjaquith Assigned to: markjaquith
Priority: high Milestone: 2.2
Component: Optimization Version: 2.1
Severity: normal Keywords: has-patch needs-testing
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

2268.diff (0.8 kB) - added by davidhouse on 01/14/06 16:09:26.
get_settings_patch.diff (0.8 kB) - added by markjaquith on 01/15/06 04:47:31.
2688.diff (1.1 kB) - added by davidhouse on 01/17/06 20:43:42.
Fixes issue skeltoac pointed out
cache_option_non_existence.diff (1.1 kB) - added by markjaquith on 01/23/07 09:43:40.
Patch for trunk
options-caches.diff (5.9 kB) - added by markjaquith on 02/01/07 08:14:19.
Alloptions and Notoptions caching. Saves a query, and reduces cache misses to zero

Change History

01/14/06 16:09:26 changed by davidhouse

  • attachment 2268.diff added.

01/14/06 16:12:56 changed by davidhouse

  • keywords set to bg|has-patch.

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.

01/14/06 19:49:05 changed by markjaquith 

+	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;

01/15/06 04:47:31 changed by markjaquith

  • attachment get_settings_patch.diff added.

01/15/06 04:48:15 changed by markjaquith

  • keywords changed from bg|has-patch to bg|has-patch bg|commit.
  • 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.

01/17/06 07:36:09 changed by skeltoac

  • keywords deleted.
  • 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.

01/17/06 20:43:42 changed by davidhouse

  • attachment 2688.diff added.

Fixes issue skeltoac pointed out

01/17/06 20:45:40 changed by davidhouse

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.

01/22/06 21:47:02 changed by markjaquith

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.

12/01/06 09:58:10 changed by matt

  • milestone changed from 2.1 to 2.2.

01/23/07 09:43:40 changed by markjaquith

  • attachment cache_option_non_existence.diff added.

Patch for trunk

01/23/07 09:46:32 changed by markjaquith

  • keywords set to has-patch needs-testing.
  • priority changed from normal to high.
  • version changed from 2.0 to 2.1.
  • component changed from Administration to Optimization.

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.

01/25/07 00:12:11 changed by markjaquith

  • status changed from assigned to closed.
  • resolution set to fixed.

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

01/30/07 01:43:43 changed by ryan

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

01/30/07 11:05:25 changed by markjaquith

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.

01/30/07 15:52:57 changed by ryan

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.

01/30/07 16:30:00 changed by markjaquith

  • status changed from closed to reopened.
  • resolution deleted.

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.

02/01/07 08:14:19 changed by markjaquith

  • attachment options-caches.diff added.

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

02/01/07 10:01:48 changed by markjaquith

  • status changed from reopened to closed.
  • resolution set to fixed.

(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.

02/01/07 10:02:29 changed by markjaquith

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.