WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#8949 closed defect (bug) (fixed)

WP creates duplicate options

Reported by: Denis-de-Bernardy Owned by:
Milestone: 2.9 Priority: low
Severity: minor Version: 2.7
Component: Optimization Keywords: needs-patch
Focuses: Cc:

Description

While testing upgrade scripts for very old sites, I've noticed occasional duplicates in the option table. Which ones are quite random from a site to the next -- but they include things like upgrade_core, and so on, i.e. it's definitely not plugin related. I take it that these are created during race conditions, where two separate users browse the site, and the options created twice.

Anyway, these could have been avoided in the first place had WP been enforcing a unique index on option_name. And it's probably worth getting rid of them in the wp upgrade scripts at some point if we enforce such an index.

Change History (25)

comment:1 @GamerZ6 years ago

Unique index on option_name sounds good!

comment:2 follow-up: @mrmist6 years ago

At the risk of teaching granny to suck eggs, you still need to enforce the unqiueness at app level, or at least handle the constraint error in the event of an insert that's non-compliant.

comment:3 in reply to: ↑ 2 @Denis-de-Bernardy6 years ago

Replying to mrmist:

At the risk of teaching granny to suck eggs, you still need to enforce the unqiueness at app level, or at least handle the constraint error in the event of an insert that's non-compliant.

Agreed, but that wouldn't fix race conditions, if two separate processes are trying to insert the same option. When this happens, the sql insert should silently fail, and wp should fallback to an sql update of the same option.

With our current version of MySQL, we could use a replace into statement rather than an insert into statement in add_option(), at the of ending up with an unpredictable option_id.

comment:4 @Denis-de-Bernardy6 years ago

as an aside, there is code in upgrade.php to dump the duplicate options, but it's clearly not working since I ended up with them nonetheless...

comment:5 @Denis-de-Bernardy6 years ago

(it's in the upgrade_130() function)

comment:6 follow-up: @DD326 years ago

(it's in the upgrade_130() function)

That code would only run if you upgraded from WP <1.3.. Surely you don't do those?

Duplicate options should not be possible if your server setup is set correctly, WP trys a get_option first.. and there should only be 1 process running at a time bulk adding options.. hopefully..

Eitherway, A Unique index on the options table, if only for optimization purposes, seems like a good thing..

comment:7 in reply to: ↑ 6 @Denis-de-Bernardy6 years ago

Replying to DD32:

(it's in the upgrade_130() function)

That code would only run if you upgraded from WP <1.3.. Surely you don't do those?

You'd be surprised... As I write, I've someone with a 1.5 site who is requesting an upgrade in my ticketing system.

comment:8 follow-up: @donncha6 years ago

FYI - I think it's a good idea too but will have to be implemented carefully. Duplicates certainly exist and if the key value is different we have to be careful about which key to delete before adding the UNIQUE index.

http://trac.mu.wordpress.org/ticket/334 covers a similar issue but with the MU sitemeta table. I added the UNIQUE index to schema.php in WordPress MU trunk for people to test but haven't received any feedback yet as it's a new change.

comment:9 in reply to: ↑ 8 ; follow-up: @jamescollins6 years ago

Replying to donncha:

FYI - I think it's a good idea too but will have to be implemented carefully. Duplicates certainly exist and if the key value is different we have to be careful about which key to delete before adding the UNIQUE index.

http://trac.mu.wordpress.org/ticket/334 covers a similar issue but with the MU sitemeta table. I added the UNIQUE index to schema.php in WordPress MU trunk for people to test but haven't received any feedback yet as it's a new change.

FYI, donncha's change for WPMU has now been reverted (see http://trac.mu.wordpress.org/ticket/334#comment:11).

comment:10 in reply to: ↑ 9 @Denis-de-Bernardy6 years ago

Replying to jamescollins:

Replying to donncha:

FYI - I think it's a good idea too but will have to be implemented carefully. Duplicates certainly exist and if the key value is different we have to be careful about which key to delete before adding the UNIQUE index.

For what it's worth, update_option() updates by key rather than by ID, so when an option with a dup gets updated, all of those that have the same name get updated as well. Thus, deleting all by the one with the highest ID works. It's usually how I clean things up.

comment:11 @Denis-de-Bernardy6 years ago

but the one with the highest ID, even

comment:12 @Denis-de-Bernardy6 years ago

  • Milestone changed from 2.8 to 2.9

moving to 2.9, from lack of a patch and given the potential implications.

comment:13 @vladimir_kolesnikov6 years ago

  • Cc vladimir@… added

Re: race conditions:

why not use a statement like this:

INSERT INTO `wp_options` (`blog_id`, `option_name`, `option_value`, `autoload`) VALUES
('blog_id', 'key', 'value', 'autoload'),
ON DUPLICATE KEY UPDATE `option_value`=VALUES(`option_value`); 

(see http://dev.mysql.com/doc/refman/5.0/en/insert-on-duplicate.html)

Thus, if two scripts are trying to insert the same option into the table, the first one will insert it, the second one will update it.

Thoughts?

comment:14 follow-up: @mrmist6 years ago

Unfortunately ON DUPLICATE KEY UPDATE is MySQL 4.1.0 which is higher than the current requirements of WordPress.

comment:15 in reply to: ↑ 14 @vladimir_kolesnikov6 years ago

Replying to mrmist:

Unfortunately ON DUPLICATE KEY UPDATE is MySQL 4.1.0 which is higher than the current requirements of WordPress.

I hate to disappoint, but WordPress (at least 2.8) already uses ON DUPLICATE KEY UPDATE:

$wpdb->query("INSERT INTO $wpdb->term_relationships (object_id, term_taxonomy_id, term_order) VALUES " . join(',', $values) . " ON DUPLICATE KEY UPDATE term_order = VALUES(term_order)");

in /wp-includes/taxonomy.php, function wp_set_object_terms().

comment:16 @mrmist6 years ago

OK, I guess that means 1 of 2 things -

  1. The requirements page needs updating to say 4.1
  2. taxonomy.php is broken.

comment:17 @vladimir_kolesnikov6 years ago

Personally I like №1 more, as MySQL 4.0 is not supported anymore and won't receive any critical updates (see http://www.mysql.com/about/legal/lifecycle/#calendar)

comment:18 @ryan6 years ago

That particular bit of code is not used by default. Only a custom taxonomy that turns on special sorting will hit that code.

We should be able to bump the requirement for 2.9.

comment:19 @Denis-de-Bernardy6 years ago

cross-referencing: #2699

comment:20 @ryan6 years ago

2.9 makes option_name unique. We still want ON DUPLICATE?

comment:21 @Denis-de-Bernardy6 years ago

that would depend on how we update the option. if it bails with a WP error on fail, we probably do. if it just ignores the db error and moves on, we don't.

comment:22 @janeforshort6 years ago

  • Milestone changed from 2.9 to Future Release

Still being discussed, no clear resolution or patch, punting.

comment:23 @Denis-de-Bernardy6 years ago

  • Milestone changed from Future Release to 2.9
  • Resolution set to fixed
  • Status changed from new to closed

Closing as fixed, since we now have a unique index. Let's re-open if error messages creep up.

comment:24 @dd325 years ago

(In [13071]) Do not poison query vars with /&foo=bar/ in requested URL. See #8949

comment:25 @dd325 years ago

(In [13071]) Do not poison query vars with /&foo=bar/ in requested URL. See #8949

Sorry, That was meant for #8948

Note: See TracTickets for help on using tickets.