#2268 closed defect (bug) (fixed)
get_option() on non-existent option always invokes new query
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (20)
#2
@
19 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;
#3
@
19 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.
#4
@
19 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.
#5
@
19 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.
#6
@
19 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.
#8
@
18 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.
#11
@
18 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.
#12
@
18 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.
#13
@
18 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.
#14
@
18 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.
#15
@
18 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.
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.