Skip to content

Keep the user transaction active after a failed statement - #536

Open
Nandan108 wants to merge 1 commit into
WordPress:trunkfrom
Nandan108:fix/keep-transaction-after-failed-statement
Open

Nandan108 wants to merge 1 commit into
WordPress:trunkfrom
Nandan108:fix/keep-transaction-after-failed-statement

Conversation

@Nandan108

Copy link
Copy Markdown
Contributor

Summary

Part of #473 (the "Explicit transactions" section).

When a statement fails inside a transaction, the driver rolls back the whole transaction and ends it. MySQL undoes only the failed statement and keeps the transaction active:

CREATE TABLE t (id INT PRIMARY KEY);
START TRANSACTION;
INSERT INTO t VALUES (1);
INSERT INTO t VALUES (1);  -- fails with a duplicate key error
INSERT INTO t VALUES (2);
COMMIT;
SELECT id FROM t;
-- Driver: 2     MySQL: 1, 2

The work before the error is lost, and the work after it runs in autocommit mode. A savepoint created before the error is gone too, so ROLLBACK TO SAVEPOINT fails with error 1305.

Fix

Each statement already runs in a wrapper: a transaction when none is active, or a savepoint inside a user transaction. On failure, only that wrapper is now rolled back. This:

  • keeps the user transaction and its savepoints active, and still undoes all changes of the failed statement, including the earlier rows of a multi-row INSERT;
  • keeps the transaction opened by LOCK TABLES, so UNLOCK TABLES commits the rows written before the error, as in MySQL;
  • replaces the special case for error 1305 (missing savepoint), since savepoint statements are not wrapped and have nothing to roll back.

Without a user transaction, the behavior is unchanged: the wrapper transaction is rolled back.

Some SQLite errors roll back the whole transaction automatically (SQLite docs). Then rolling back to the savepoint fails, and the driver ends the transaction as before. This path has no test, because those errors (such as SQLITE_FULL) are hard to trigger in a unit test.

The rest of #473 (error modes and diagnostics) is not part of this PR.

Tests

  • testFailedStatementKeepsTransactionActive checks that a failed write and a failed read keep the transaction active, and that COMMIT keeps the work before and after them.
  • testFailedStatementIsRolledBackInTransaction checks that a failed multi-row INSERT leaves none of its rows.
  • testRollbackToSavepointAfterFailedStatement checks that a savepoint survives a failed statement.
  • testFailedStatementKeepsTableLock checks that UNLOCK TABLES keeps the rows written before a failed statement.
  • test_failed_query_keeps_transaction_active checks the same through PDO::beginTransaction() and PDO::commit().

All five fail on trunk. The existing testMissingSavepointDoesNotRollbackTransaction still passes.

packages/mysql-on-sqlite: 1048 tests pass (PHP 8.3, SQLite 3.45.1); PHPCS is clean.

Use of AI

This change was prepared with Claude Code (Claude Opus 5.5). I reviewed the code, the tests, and this description, and I take responsibility for the change.

Note: this PR and #535 both add tests at the same spot of WP_MySQL_On_SQLite_PDO_API_Tests.php, so whichever merges second needs a trivial rebase.

🤖 Generated with Claude Code

When a statement failed, the driver rolled back the whole user
transaction, including the work done before the failed statement, and
ended it. MySQL undoes only the failed statement and keeps the
transaction active, so the caller can decide whether to commit or roll
back the rest. A later COMMIT kept only the work done after the error,
and a savepoint created before the error could no longer be used.

Roll back only the wrapper transaction of the failed statement. Inside a
user transaction, the wrapper is a savepoint, so the transaction stays
active. This also keeps an active LOCK TABLES transaction, and replaces
the special case for a missing savepoint, since savepoint statements are
not wrapped.

Part of WordPress#473.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 13, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 0588afe0-c4d7-4faf-ae55-3c9fb9b2687e
📥 Commits

Reviewing files that changed from the base of the PR and between 8dc5c27 and 3221612.

📒 Files selected for processing (3)
  • packages/mysql-on-sqlite/src/sqlite/class-wp-mysql-on-sqlite.php
  • packages/mysql-on-sqlite/tests/WP_MySQL_On_SQLite_PDO_API_Tests.php
  • packages/mysql-on-sqlite/tests/WP_MySQL_On_SQLite_Tests.php
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant