Opened 16 years ago
Closed 15 years ago
#9422 closed enhancement (fixed)
Allow for start transaction/rollback/commit sql statements in WP
Reported by: |
|
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)
Change History (40)
#1
@
16 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:
- switches the order of the delete statements where needed
- 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).
#2
@
16 years ago
Somewhat related question. Are there any reasons for not making MySQL use InnoDB and enforcing foreign keys by default?
#5
@
16 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.
#7
@
16 years ago
@Hakre: Not really, no. In MySQL, triggers are not called for statements that are the result of a cascade.
#8
follow-up:
↓ 9
@
16 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?
#9
in reply to:
↑ 8
@
16 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.
#10
@
16 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...
#12
@
16 years ago
I think we can close this one, for now anyway.
Opening a separate ticket for schema clean-ups.
D.
#14
@
16 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?
#15
@
16 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.
#16
@
16 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.
#17
@
16 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.
#20
@
16 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.
#21
@
16 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.
#24
@
16 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.
#26
@
16 years ago
- Keywords early added; dev-feedback removed
- Milestone changed from 2.8 to 2.9
sure
#28
@
16 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.
fix delete statements to ensure data integrity when innodb and foreign keys are used