WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#34304 closed feature request (wontfix)

get_and_update_post_meta for atomic post meta operations

Reported by: lpghatguy Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.4
Component: Database Keywords: close
Focuses: Cc:

Description

Assuming I have a function on the server that pulls post meta using get_post_meta and then writes it back, there's a relatively wide window for another request to compete with database resources and write an incorrect value.

I'm going to call this function F, separated into two parts, read (R) and write (W). The result of F depends on the current value of the meta.

As an example, say that I have two requests, R1 and R2, both competing to perform operation F. Ideally, this would happen like so:
R1: R
R1: W
R2: R
R2: W

In the wild, sometimes an awful race happens:
R1: R
R2: R
R1: W
R2: W

This can result in a catastrophically wrong value written to the database (usually as if request R1 never happened). What I'm proposing is a way to lock a specific row (or piece of post meta) for a defined period of time.

The model of flow would then change to the following:
R1: R & lock
R1: W & release lock
R2: R & lock
R2: W & release lock

In this case, '&' denotes that the operation should be totally atomic (one database query).

A possible API for this might be:

get_and_update_post_meta($postID, $metaName, $callback);

Then, assuming PHP 5.4 or newer, it could be used to create an atomic increment operation like so:

get_and_update_post_meta($ID, "some_meta_field", function($value) {
	return parseInt($value) + 1;
});

In older PHP releases, this could be done with a predefined function and a string-based function name.

As far as implementation goes, MySQL 5.0+ supports GET_LOCK, RELEASE_LOCK, and IS_FREE_LOCK (http://dev.mysql.com/doc/refman/5.0/en/miscellaneous-functions.html#function_get-lock) that would be perfect for this purpose.

get_and_update_post_meta would be functionally equivalent to the following two:

  1. IS_FREE_LOCK & GET_LOCK & get_post_meta
  2. RELEASE_LOCK & update_post_meta

The reasoning for the callback is to prevent accidental locks without corresponding unlocks. Other systems totally unrelated to WordPress use a similar model, like LOVE: https://love2d.org/wiki/Channel:performAtomic

Attachments (1)

proof-of-concept.php (1.9 KB) - added by lpghatguy 5 years ago.
Proof of concept implementation providing get_and_update_*_meta and more

Download all attachments as: .zip

Change History (7)

@lpghatguy
5 years ago

Proof of concept implementation providing get_and_update_*_meta and more

#1 @lpghatguy
5 years ago

I've attached a proof-of-concept implementation that uses MySQL GET_LOCK and RELEASE_LOCK without checking timeouts right now. It handles no edge cases but is otherwise functional for single-value post and user metadata.

#2 @swissspidy
5 years ago

Why limit this to post meta? The same race conditions can happen with every table and entry.

WordPress currently doesn't use table/row locks as these are dependant on the storage engines and not guaranteed to be available all the time. Also, using multiple database servers needs to be considered.

All in all, it's not as trivial as it sounds.

#3 @lpghatguy
5 years ago

Atomic operations on every type of data would be pretty rad.

From what I can tell with the MySQL 5.0+ manuals, the named locking mechanisms (GET_LOCK and friends) are supported under all storage engines. They have the downside that you have to explicitly check them before writing to some data, which could create some overhead for every database read/write. I'd be interested to see just how big of a hit atomic read/write/update would cause.

Actual table/row locks (eg LOCK TABLE) *do* seem to depend heavily on storage engine, and they'd certainly be a better solution in a controlled environment, but I don't think they'd be practical for WordPress in particular.

MySQL tries to preserve full ACID compliance across shards, so I don't know how named locks are shared between multiple servers. It'd be worth looking into further, I imagine.

#4 @rmccue
5 years ago

  • Keywords close added

update_post_meta has a $prev_value parameter that you can pass, which gives you a sort of concurrency protection, although it's not locking. For this example, you could do:

$value = $old = get_post_meta( $id, 'some_meta_field', true );
$value++;
$success = update_post_meta( $id, 'some_meta_field', wp_slash( $value ), $old );

(Obviously, this is not exactly equivalent, and locking does have benefits.)

I'd be in favour of a full locking API throughout WP, similar to the object cache API, but I think we should go wholesale into it if so. I don't see a need for one specifically for post meta though.

#5 @lpghatguy
5 years ago

  • Resolution set to wontfix
  • Status changed from new to closed

I opened a much more broad ticket at #34330. Thanks!

#6 @swissspidy
5 years ago

  • Milestone Awaiting Review deleted
Note: See TracTickets for help on using tickets.