WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#9422 closed enhancement (fixed)

Allow for start transaction/rollback/commit sql statements in WP

Reported by: Denis-de-Bernardy Owned by:
Milestone: 2.9 Priority: normal
Severity: major Version: 2.8
Component: Optimization Keywords: needs-patch
Focuses: Cc:

Description

It would be sweet if WP were to become 100% transaction safe. I wondered if it was, while developing a plugin that required InnoDB tables and transactions for its own tables, as well as InnoDB on the users and posts table in order to enforce foreign key constraints.

Things like:

do_action('delete_user', $id);

$wpdb->query( $wpdb->prepare("DELETE FROM $wpdb->users WHERE ID = %d", $id) );
$wpdb->query( $wpdb->prepare("DELETE FROM $wpdb->usermeta WHERE user_id = %d", $id) );

Should really be:

do_action('delete_user', $id);

$wpdb->query( $wpdb->prepare("DELETE FROM $wpdb->usermeta WHERE user_id = %d", $id) );

$wpdb->query( $wpdb->prepare("DELETE FROM $wpdb->users WHERE ID = %d", $id) );

That way, if one enforces the relevant foreign key and deletes a user as part of a transaction, the database won't complain. Along the same lines, the delete post procedure is particularly ugly...

Attachments (6)

9422.diff (2.3 KB) - added by Denis-de-Bernardy 5 years ago.
fix delete statements to ensure data integrity when innodb and foreign keys are used
9422-ids.diff (5.3 KB) - added by Denis-de-Bernardy 5 years ago.
enforce consistent ID types to allow for foreign keys to be defined between tables
9422-ids-extra.diff (1.1 KB) - added by Denis-de-Bernardy 5 years ago.
missed a few
7422-deleted-fix.diff (671 bytes) - added by Denis-de-Bernardy 5 years ago.
revert deleted_post to its initial location
9422-user-delete-transaction.diff (1.0 KB) - added by Denis-de-Bernardy 5 years ago.
wrap wp_delete_user into hooks to allow for being/commit sql statements
9422-new-wpdb-methods.diff (2.9 KB) - added by Denis-de-Bernardy 5 years ago.
begin(), commit(), rollback() methods for wpdb

Download all attachments as: .zip

Change History (40)

Denis-de-Bernardy5 years ago

fix delete statements to ensure data integrity when innodb and foreign keys are used

comment:1 Denis-de-Bernardy5 years ago

  • Keywords has-patch tested dev-feedback added
  • Milestone changed from Unassigned to 2.8
  • Owner changed from anonymous to Denis-de-Bernardy
  • Severity changed from normal to major
  • Version changed from 2.7.1 to 2.8

The attach patch, against WP 2.8 latest trunk:

  1. switches the order of the delete statements where needed
  2. moves the delete_post hook call to *before* things are deleted (as is done for users)

The first point has zero impact on the way things already are.

The only point worth considering is whether calling the delete_post before or after makes any difference. The only place where this hook is used in WP is to flush the recent posts widget cache, and it indeed makes no difference whether it's done before or after.

Side note: In SQL terms, this would correspond to turning an ON AFTER DELETE trigger into an ON BEFORE DELETE trigger. It's not a benign difference, but it would make the various delete_* hooks consistent throughout WP (for attachments, links, terms, etc, the delete hook is called before the actual query).

An alternative patch would leave the current hook as is, and add a before_delete_post hook, e.g. before_delete_post, before the post gets deleted (so that tables with foreign key constraints don't complain).

comment:2 Denis-de-Bernardy5 years ago

Somewhat related question. Are there any reasons for not making MySQL use InnoDB and enforcing foreign keys by default?

comment:3 Denis-de-Bernardy5 years ago

Err, I was of course meaning, for not making WP use InnoDB. :P

comment:4 ryan5 years ago

(In [10851]) Fix delete statements to ensure data integrity when innodb and foreign keys are used. Props Denis-de-Bernardy. see #9422

comment:5 hakre5 years ago

You can setup the DB sothat it will delete all foreigned-keyed data on delete of the metadata. Integrity is yours without any patch then.

comment:6 Denis-de-Bernardy5 years ago

Thanks Ryan!

comment:7 Denis-de-Bernardy5 years ago

@Hakre: Not really, no. In MySQL, triggers are not called for statements that are the result of a cascade.

comment:8 follow-up: Denis-de-Bernardy5 years ago

Slightly related: While we're at it, could we change the IDs to always use the bigint type, so as to be able to add foreign keys between the WP tables?

comment:9 in reply to: ↑ 8 ryan5 years ago

Replying to Denis-de-Bernardy:

Slightly related: While we're at it, could we change the IDs to always use the bigint type, so as to be able to add foreign keys between the WP tables?

Sure, as long as dbDelta handles the transition nicely during upgrade.

Denis-de-Bernardy5 years ago

enforce consistent ID types to allow for foreign keys to be defined between tables

comment:10 Denis-de-Bernardy5 years ago

There you go.

I tried to make dbdelta enforce foreign keys while I was at it, but that function needs some serious reengineering for that to work. :-P

So sticking to adding the things manually for now...

comment:11 follow-up: ryan5 years ago

(In [10852]) Enforce consistent ID types to allow for foreign keys to be defined between tables. Props Denis-de-Bernardy. see #9422

comment:12 Denis-de-Bernardy5 years ago

I think we can close this one, for now anyway.

Opening a separate ticket for schema clean-ups.

D.

Denis-de-Bernardy5 years ago

missed a few

comment:13 Denis-de-Bernardy5 years ago

clean-ups are at #9435

comment:14 azaozz5 years ago

Do we really need bigint(20) unsigned for all of these fields? Seems excessive.

`"INT[(M)] [UNSIGNED] [ZEROFILL]
A normal-size integer. The signed range is -2147483648 to 2147483647. The unsigned range is 0 to 4294967295.

BIGINT[(M)] [UNSIGNED] [ZEROFILL]
A large integer. The signed range is -9223372036854775808 to 9223372036854775807. The unsigned range is 0 to 18446744073709551615"`

Don't think there would be any post_id, user_id, term_id, etc. over 4.2 billion. That would mean a table with 4,200,000,000 rows. Using "normal" int instead of bigint would make the db size smaller and all indexes faster. Are there any other reasons why we use bigint everywhere?

Denis-de-Bernardy5 years ago

revert deleted_post to its initial location

comment:15 Denis-de-Bernardy5 years ago

Quite honestly, I agree. I think int(11) would be large enough. Much like text (or even smalltext) would be large enough for a user meta value. But that is why I opened #9435. :-)

In this ticket, I'd rather focus on making sure transactions work. The DB schema changes were necessary for the foreign keys, but it's still missing a plugin hook here and there to toss in BEGIN and COMMIT statements where needed.

For most things, foreign keys with on delete cascade statements are fine. But if you've two or more blogs with a shared, custom users and usermeta table, you need a means to make an entire transaction fail if it so happens that things might get cascaded a bit too much.

Along the same lines, if money transactions are tied to user IDs, the last thing you'd want are a cascade of events that ultimately delete important rows that belong in an accounting software. (It's important to keep in mind that triggers are *not* called in MySQL on cascade deleted rows.)

Can anyone commit 9422-ids-extra.diff and 9422-deleted-fix.diff? Once that's done, I'll add a few clean-up diffs in #9435 against the resulting schema.

Denis-de-Bernardy5 years ago

wrap wp_delete_user into hooks to allow for being/commit sql statements

comment:16 jacobsantos5 years ago

You know, I was going to point out that mysql extension doesn't support transactions natively. Then I realized that this isn't about real mySQL transactions, but imaginary ones.

I still think this ticket is poorly named for the summary. Well, you can send the MySQL Tranaction begin and commit statements manually, but these don't really handle errors well.

comment:17 Denis-de-Bernardy5 years ago

  • Summary changed from Make WP transaction safe to Allow for start transaction/rollback/commit sql statements in WP

Yeah, I'm quite aware. I ended up working around this by using savepoints and a global variable to make sure no errors occurred, so as to determine if a commit or a rollback was needed. :D

D.

comment:18 Denis-de-Bernardy5 years ago

... but then again, that only works for my own statements, not those from WP. :P

comment:19 ryan5 years ago

(In [10858]) Move deleted_post back. Props Denis-de-Bernardy . see #9422

comment:20 Denis-de-Bernardy5 years ago

@Ryan: I'd be curious to have your thoughts on the following:

wp_delete_user() has multiple calls to wp_delete_post(), which then fire wp_delete_attachment() etc. in a cascaded manner. if an error occurs due to tables that got added, we could potentially have stuff that is only partially deleted (see code below).

to work around this, we'd need, at minimum, and in each of the wp_delete_* functions, a hook like delete_* at the very beginning, and a hook like deleted_* at the very end. then, we've two options.

either WP sets a global variable that contains the name of a savepoint (set by the initial call to whichever wp_delete_* function got called), or the plugin takes care of doing it itself. both options have pros and cons.

the pro for WP managing it is that plugins can rely on it being set where appropriate, the con being that this adds some code.

the pro for the plugin managing it adds no code to WP, the con being two plugins that rely on it may end up conflicting, due to the following:

start transaction;
select * from table; -- returns 2 rows
delete from table where foo -- trigger error due to check or foreign key
delete from table where bar -- 1 row affected
commit;
select * from foo; -- returns 1 row in mysql, as opposed to the expected 2 in pgsql

that can lead to plugin A sending a commit before plugin B has only partially validated a transaction.

my own preference would be to use something like:

function wp_start_transaction($transaction_id) {
  global $wp_transaction_id;
  if ( !isset($wp_transaction_id) ) {
    $wp_transaction_id = $transaction_id;
    $wpdb->query("START TRANSACTION;");
  }
}

function wp_rollback_transaction($wp_error) {
  global $wp_transaction_id;
  unset($wp_transaction_id);
  $wpdb->query("ROLLBACK;");
  wp_die($wp_error);
}

function wp_commit_transaction($transaction_id) {
  global $wp_transaction_id;
  if ( isset($wp_transaction_id) && $wp_transaction_id == $transaction_id ) {
    unset($wp_transaction_id);
    $wpdb->query("COMMIT;");
  }
}

If the code is unwanted most of the time, we could add some kind of define (e.g. WP_TRANSACTIONS) that adds them as needed to the various plugin hooks when a plugin needs them.

Thoughts?

D.

comment:21 Denis-de-Bernardy5 years ago

Actually, a better option would be to do the above with three extra methods to wpdb, i.e. wpdb->begin(), wpdb->rollback() and wpdb->commit() that do mostly the same as the functions described above.

D.

Denis-de-Bernardy5 years ago

begin(), commit(), rollback() methods for wpdb

comment:22 ryan5 years ago

(In [10894]) Add deleted_user action. Props Denis-de-Bernardy. see #9422

comment:23 Denis-de-Bernardy5 years ago

thanks Ryan!

comment:24 Denis-de-Bernardy5 years ago

9422-new-wpdb-methods.diff could be committed as a bonus, but it's definitely optional.

suggesting fixed, since delete users now have a deleted_user hook.

comment:25 ryan5 years ago

I think we should consider the new methods, but not for 2.8. Postpone to 2.9?

comment:26 Denis-de-Bernardy5 years ago

  • Keywords early added; dev-feedback removed
  • Milestone changed from 2.8 to 2.9

sure

comment:27 Denis-de-Bernardy5 years ago

  • Keywords commit added

comment:28 Denis-de-Bernardy5 years ago

we may also want to dig into the other insert/update/delete functions, and possibly patch the code base so as to get, each time:

  • delete_$foo and deleted_$foo
  • save_$foo and saved_$foo

the hooks could be changed to ensure backwards compat, but the general idea would be there. and with a little define, we'd fire the wpdb methods automatically.

comment:29 Denis-de-Bernardy5 years ago

  • Keywords needs-patch added; has-patch tested commit early removed

comment:31 Denis-de-Bernardy5 years ago

  • Status changed from new to accepted

comment:32 in reply to: ↑ 11 Otto425 years ago

Replying to ryan:

(In [10852]) Enforce consistent ID types to allow for foreign keys to be defined between tables. Props Denis-de-Bernardy. see #9422

This changeset broke XML-RPC image attachments. See ticket #10521.

comment:33 Denis-de-Bernardy5 years ago

  • Owner Denis-de-Bernardy deleted
  • Status changed from accepted to assigned

comment:34 ryan4 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.