Skip to content

Track transactions opened by SQL savepoints - #496

Open
chubes4 wants to merge 4 commits into
WordPress:trunkfrom
chubes4:fix-495-savepoint-write-transactions
Open

Track transactions opened by SQL savepoints#496
chubes4 wants to merge 4 commits into
WordPress:trunkfrom
chubes4:fix-495-savepoint-write-transactions

Conversation

@chubes4

@chubes4 chubes4 commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

What?

Track transactions opened by translated SQL SAVEPOINT statements so write wrappers use an internal savepoint instead of attempting a nested top-level BEGIN IMMEDIATE.

Add adapter and WordPress integration regressions for standalone write wrapping, release/commit, rollback, nested and duplicate savepoints, quoted/case-varied identifiers, affected-row behavior, and failure cleanup.

Why?

On PHP versions before 8.4, PDO SQLite does not reliably expose transactions opened through SQL via PDO::inTransaction(). The adapter already maintains an internal transaction-state polyfill, but translated SAVEPOINT, ROLLBACK TO SAVEPOINT, and RELEASE SAVEPOINT statements did not update it. A SAVEPOINT issued without an existing transaction therefore opened an SQLite transaction that remained invisible to the write wrapper, which then failed on BEGIN IMMEDIATE before executing the translated write.

The transaction-state invariant after this change is: the adapter's polyfilled state remains true from any successful top-level BEGIN or outermost SQL SAVEPOINT until the corresponding top-level commit, rollback, or outermost release. A normalized savepoint stack preserves nested, duplicate-name, and case-insensitive rollback/release semantics while standalone writes retain their atomic BEGIN IMMEDIATE wrapper.

Testing Instructions

  1. Run composer run test tests/WP_MySQL_On_SQLite_PDO_API_Tests.php from packages/mysql-on-sqlite.
  2. Run composer run test from packages/mysql-on-sqlite.
  3. Run composer run check-cs from the repository root.
  4. Run composer run wp-test-sqlite-plugin-php and composer run wp-test-sqlite-plugin-php-multisite in a Docker-enabled environment.

Local results:

  • Focused PDO API suite: 97 tests, 297 assertions, passing.
  • Full MySQL-on-SQLite suite: 882 tests, 1,428,775 assertions, passing (15 skipped, 2 incomplete pre-existing).
  • Full PHPCS gate: passing.
  • The $wpdb->update() integration regression uses PHPUnit_Adapter_TestCase, avoiding the transaction automatically opened by WP_UnitTestCase; it asserts no transaction is active before the standalone SQL SAVEPOINT and after outermost release.
  • The local WordPress Docker suite cannot run because Docker is unavailable on the execution host; the equivalent single-site and multisite jobs run in PR CI.

Extra Chill Reproduction Evidence

The original failure was retained from the deterministic Extra Chill/Data Machine reproduction:

  • Runtime: runtime-mt6bqbcc-dfymmz
  • Artifact: artifact-bundle-sha256-38b3a05a217740e7b6801de40989b68ad518c5fa789c97c5a683ea9ae0df5161
  • Artifact path: /mnt/extrachill-workspace/tmp/opencode/verify-2578-after-3346-filesystem-trace-wp692/runtime-mt6bqbcc-dfymmz
  • WordPress 6.9.2 / PHP 8.3.32
  • Caller result: wpdb->update() returned false, rows_affected=0, with cannot start a transaction within a transaction in last_error.
  • Adapter trace stopped after Executing: BEGIN IMMEDIATE; the translated UPDATE never executed.

Fixes #495

@JanJakes JanJakes left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chubes4 Thanks for the PR! This looks good overall. I left one small simplification suggestion.

* @var bool
*/
private $transaction_started_by_savepoint = false;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use a single property by tracking only savepoints in a transaction opened by SAVEPOINT? Savepoints inside a transaction opened by BEGIN do not need tracking because releasing them cannot end the outer transaction. This would remove the need for $transaction_started_by_savepoint.

Let’s also document why this state is needed. Something like:

	/**
	 * User savepoints in a transaction opened by a SAVEPOINT statement.
	 *
	 * On PHP < 8.4, PDO SQLite cannot detect transactions opened with raw SQL.
	 * Tracking the savepoint stack keeps the inTransaction() polyfill accurate
	 * when the outermost savepoint is released.
	 *
	 * Savepoints inside a transaction opened by BEGIN are not tracked because
	 * releasing them cannot end the outer transaction.
	 *
	 * @var string[]
	 */
	private $savepoint_transaction_stack = array();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thank you!

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.

Write inside SAVEPOINT attempts nested BEGIN IMMEDIATE

2 participants