Repository navigation
Conversation
Statements with an empty result set use a TEMP table that is created lazily, the first time such a result is needed. When that happened inside a transaction, for example for START TRANSACTION itself, a rollback dropped the table, while the driver still assumed it existed. ROLLBACK and every later statement with an empty result set then failed with "Table '_wp_sqlite_empty_result' doesn't exist". When the table is missing, create it again and retry, as the information schema builder already does for temporary information schema tables. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou'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. View limit details
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. Comment |
JanJakes
reviewed
Oct 15, 2026
| 'CREATE TEMP TABLE IF NOT EXISTS %s ( placeholder_for_empty_insert )', | ||
| $this->quote_sqlite_identifier( self::EMPTY_RESULT_TABLE_NAME ) | ||
| ) | ||
| ); |
Member
There was a problem hiding this comment.
Oh, this is becoming kind of complex. I guess we could just always do:
CREATE TEMP TABLE IF NOT EXISTS %s ( placeholder_for_empty_insert );
INSERT INTO %s ( placeholder_for_empty_insert ) SELECT NULL WHERE FALSE;Or maybe there's a way to make an empty statement without any queries. Checking 👀
JanJakes
added a commit
that referenced
this pull request
Oct 15, 2026
Empty result sets used a no-op INSERT into a TEMP table that was created on first use. When the first use happened in a transaction, for example for START TRANSACTION itself, a rollback dropped the table, while the driver still assumed it existed. Every later statement with an empty result set then failed with "Table '_wp_sqlite_empty_result' doesn't exist". Use an unknown pragma instead. It has no columns, SQLite ignores it, and it needs no table and takes no locks. PDO reports the row count of the last write for it, but the driver reports 0 affected rows for all statements other than DML. Add regression tests for the rollback, and a test for the empty result statement API. See #535
JanJakes
added a commit
that referenced
this pull request
Oct 15, 2026
Empty result sets used a no-op INSERT into a TEMP table that was created on first use. When the first use happened in a transaction, for example for START TRANSACTION itself, a rollback dropped the table, while the driver still assumed it existed. Every later statement with an empty result set then failed with "Table '_wp_sqlite_empty_result' doesn't exist". Use an unknown pragma instead. It has no columns, SQLite ignores it, and it needs no table and takes no locks. PDO reports the row count of the last write for it, but the driver reports 0 affected rows for all statements other than DML. Add regression tests for the rollback, and a test for the empty result statement API. See #535
JanJakes
added a commit
that referenced
this pull request
Oct 16, 2026
Empty result sets used a no-op INSERT into a TEMP table that was created on first use. When the first use happened in a transaction, for example for START TRANSACTION itself, a rollback dropped the table, while the driver still assumed it existed. Every later statement with an empty result set then failed with "Table '_wp_sqlite_empty_result' doesn't exist". Use an unknown pragma instead. It has no columns, SQLite ignores it, and it needs no table and takes no locks. PDO reports the row count of the last write for it, so result sets that the driver builds itself now default to 0 affected rows. Add regression tests for the rollback, and a test for the empty result statement API. See #535
Member
|
Closing in favor of #537. Thank you for catching this! |
JanJakes
added a commit
that referenced
this pull request
Oct 16, 2026
## Summary On a fresh connection, rolling back the first transaction breaks the connection: ```sql START TRANSACTION; ROLLBACK; -- SQLSTATE[42S02]: Base table or view not found: 1146 -- Table '_wp_sqlite_empty_result' doesn't exist ``` Statements with an empty result set (`SET`, `CREATE TABLE`, …) used a no-op `INSERT` into a TEMP table that was created on first use. When the first use happened inside a transaction, the rollback dropped the table, but the driver still assumed it existed. This is an **alternative to #535**. Instead of creating the table again, it **removes the need for the table**. ## Fix PDO needs a statement object even when there is nothing to return. This PR uses an **unknown pragma** for it. SQLite ignores unknown pragmas, so the statement has no columns and no rows. It **needs no table and takes no locks**, so a rollback has nothing to drop, and statements like `SET` still work while another connection writes. ## Row counts PDO SQLite's `rowCount()` reports affected rows only for `INSERT`, `UPDATE`, and `DELETE`. For other statements, it can report the row count of the **last write** on the connection, including the driver's internal writes. The old no-op `INSERT` reset that count after every empty result set, which hid most of these cases. The pragma doesn't reset it. The driver now **reports `0` affected rows for all statements other than `INSERT`, `REPLACE`, `UPDATE`, and `DELETE`**. This also fixes cases that were already broken on `trunk`: - **`SELECT` or `SHOW` without rows** right after a write reported the written rows. - **`DROP VIEW` or `DROP TRIGGER`** right after a write reported the written rows. - **`TRUNCATE TABLE`** on a table with `AUTO_INCREMENT` reported `1`. It now reports `0`, as in MySQL. ## Why not keep the table? The other options are worse than the pragma: - **Create the table again and retry (#535).** It keeps a flag that can go stale, and depends on SQLite's "no such table" error message. - **Run `CREATE TEMP TABLE IF NOT EXISTS` before every `INSERT`.** It costs about 0.5 µs per empty result, and it doesn't fix the row counts. - **Use another no-op statement.** PDO can't execute a statement with only a comment or `;`. `DROP … IF EXISTS` for a missing table can need a lock to read the schema. ## Tests The new tests cover the bug, the row counts, and the empty result statement: - **Two rollback regression tests**, adapted from #535. They fail on `trunk` with the error above. - **Two row count tests** for statements that don't write. They fail on `trunk`. - **`test_empty_result_statement`** checks the PDO API of the empty result statement, and that holding an unread one doesn't block `COMMIT` or `DROP TABLE`. Supersedes #535. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Non-data-changing statements, including `TRUNCATE`, now report zero affected rows rather than carrying over a previous write count. INSERT, REPLACE, UPDATE, and DELETE continue to report their affected-row counts. * Empty-result statements now return consistent results and counts, and no longer block transaction commits or schema changes. * **Tests** * Added coverage for empty results, affected-row counts, and rolling back the first transaction on a connection. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
On a fresh connection, a transaction that is rolled back before any other statement with an empty result set breaks the connection:
Every later statement with an empty result set (
CREATE TABLE,SET, ...) fails the same way. The same happens withPDO::beginTransaction()andPDO::rollBack().Statements with an empty result set use a TEMP table that is created lazily, the first time such a result is needed (#449). When that happens inside a transaction, as for
START TRANSACTIONitself, the rollback drops the table, while the driver still assumes it exists. WordPress is unlikely to hit this because it runsSETstatements right after connecting, but the PDO API and the MySQL proxy do.Fix
When the table is missing, it is created again and the statement is retried. This follows the existing handling of temporary information schema tables in
record_create_table(), and it covers every way the creation can be rolled back (ROLLBACK,PDO::rollBack(),ROLLBACK TO SAVEPOINT, and a failed statement).Tests
test_rollback_statement_as_first_transaction_on_connectionandtest_rollback_method_as_first_transaction_on_connectionroll back the first transaction of a fresh connection, using SQL statements and the PDO methods, and check that later statements still work. Both fail ontrunkwith the error above.packages/mysql-on-sqlite: 1045 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 the "Keep the user transaction active after a failed statement" PR 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