WP creates duplicate options

[Denis-de-Bernardy]

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.

[GamerZ]

Unique index on option_name sounds good!

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

[Denis-de-Bernardy]

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.

[Denis-de-Bernardy]

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

[Denis-de-Bernardy]

(it's in the upgrade_130() function)

[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?

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

[Denis-de-Bernardy]

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.

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

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

​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).

[Denis-de-Bernardy]

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.

[Denis-de-Bernardy]

but the one with the highest ID, even

[Denis-de-Bernardy]

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

[vladimir_kolesnikov]

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?

[mrmist]

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

[vladimir_kolesnikov]

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

[mrmist]

OK, I guess that means 1 of 2 things -

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

[vladimir_kolesnikov]

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)

[ryan]

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.

[Denis-de-Bernardy]

cross-referencing: #2699

[ryan]

2.9 makes option_name unique. We still want ON DUPLICATE?

[Denis-de-Bernardy]

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.

[janeforshort]

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

[Denis-de-Bernardy]

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

[dd32]

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

[dd32]

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

Sorry, That was meant for #8948