Don't take a write lock for SET statements (fixes "database is locked" on connect)#443
Open
gcsecsey wants to merge 1 commit into
Open
Don't take a write lock for SET statements (fixes "database is locked" on connect)#443gcsecsey wants to merge 1 commit into
gcsecsey wants to merge 1 commit into
Conversation
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
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.
Related to STU-1821
What changed
Every WordPress request runs
SET SESSION sql_mode = …at bootstrap (viawpdb::set_sql_mode()). This statement changes only session state and writes nothing to the database, but the driver acquires an SQLite write lock for it. Under concurrent access, this makes an otherwise read-only request fatal withdatabase is locked(SQLITE_BUSY) the moment any other connection holds the write lock.This PR fixes the two places the
SETpath takes a write lock (WP_PDO_MySQL_On_SQLite):The wrapper transaction.
query()opensBEGIN IMMEDIATE(a RESERVED/write lock) for every non-SELECTstatement, andSETwas treated as a write. →SETis now detected as read-only, so it opens a deferredBEGIN(SHARED) instead — the same treatmentSHOW/DESCRIBEalready receive (Improve concurrent database access #361).The empty-result builder.
create_result_statement_from_data()fabricates a 0-column result with a no-opINSERT … WHERE FALSEagainst a regular table. SQLite acquires a write lock for any DML, even one that inserts zero rows, so this re-took a write lock right after the wrapper committed. → The 0-column statement is now built with a no-opINSERTinto a connection-privateTEMPtable, whose temp database has no shared lock.Both changes are required: with only (1), the fatal simply moves from the
BEGIN IMMEDIATEto the no-opINSERT.Why
This is a direct follow-up to #361 (which stopped
SELECT/SHOW/DESCRIBEfrom taking write locks and closed #318).SETwas left on the write path. Becauseset_sql_mode()runs on every connection, the impact is broad: a single slow request holding a write transaction (a background WooCommerce/Jetpack write, an import, a long render on a heavy site) makes every concurrent request fail at connect, not only concurrent writers. SQLite is single-writer by design, but statements that perform no writes should never contend for the write lock.Journal mode does not address this on its own: it reproduces identically under both
DELETEandWAL(WAL still serialises writers).Reproduction
Deterministic, single process, two driver connections to one file:
Before:
WP_SQLite_Driver_Exception: … database is locked, first inbegin_wrapper_transaction(), or after fixing only (1), increate_result_statement_from_data().After: the
SETreturns immediately while A still holds its write lock.This also reproduces end-to-end in a multi-worker WordPress install (eg. PHP-FPM with more than one workers) under concurrent requests, where one slow request holding a write transaction makes concurrent page loads fatal at
set_sql_mode.Tests
Two regression tests added to
WP_SQLite_Driver_Concurrency_Tests:testSetQueryOpensReadOnlyTransaction:SETopensBEGIN, notBEGIN IMMEDIATE.testSetQuerySucceedsWhileAnotherConnectionHoldsWriteLock: with another connection holdingBEGIN IMMEDIATEandtimeout = 0,SETcompletes instead of throwingSQLITE_BUSY.Both fail on
trunkand pass with this change.Notes / risk
WP_SQLite_Driver::query()returnsfetchAll()whencolumnCount() > 0, otherwiserowCount(). The replacement empty-result statement keepscolumnCount() === 0, so writes andSETstill return their affected-row count (not an empty array). Verified: running a representativeCREATE/INSERT/SELECT/COUNT/UPDATE/DELETE/SETsequence produces byte-identical results before and after.INSERT/UPDATE/DELETE) also fabricate their empty result through this path, which previously meant a second, redundant write-lock acquisition after the real write. That redundant lock is now gone, slightly reducing lock pressure for all writes.TEMPtable is created once per connection (guarded by a flag);TEMPtables live for the connection's lifetime. A reviewer preferring maximum robustness over the micro-optimisation could drop the flag and always issue the idempotentCREATE TEMP TABLE IF NOT EXISTS.