recover_deadlock: sync deferred data cursors before non-SI lock release - #6212
dorinhogea wants to merge 1 commit into
Conversation
roborivers
left a comment
roborivers
left a comment
There was a problem hiding this comment.
Cbuild submission: Success ✓.
Regression testing: Success ✓.
The first 10 failing tests are:
analyze_partial_index_off_generated [failed with core dumped] **quarantined**
analyze [failed with core dumped] **quarantined**
tsa
consumer_non_atomic_default_consumer_generated **quarantined**
tunables
sc_downgrade [timeout] **quarantined**
skipscan [timeout] **quarantined**
|
/plugin-branch sqlqueries |
roborivers
left a comment
roborivers
left a comment
There was a problem hiding this comment.
Cbuild submission: Error ⚠.
Regression testing: Success ✓.
The first 10 failing tests are:
analyze_partial_index_off_generated [failed with core dumped] **quarantined**
analyze [failed with core dumped] **quarantined**
sc_resume_logicalsc_generated **quarantined**
consumer_non_atomic_default_consumer_generated **quarantined**
tunables
sc_downgrade [timeout] **quarantined**
skipscan [timeout] **quarantined**
roborivers
left a comment
roborivers
left a comment
There was a problem hiding this comment.
Cbuild submission: Success ✓.
Regression testing: Success ✓.
The first 10 failing tests are:
analyze_partial_index_off_generated [failed with core dumped] **quarantined**
analyze [failed with core dumped] **quarantined**
comdb2sys **quarantined**
consumer_non_atomic_default_consumer_generated **quarantined**
tunables
sc_downgrade [timeout] **quarantined**
skipscan [timeout] **quarantined**
A non-snapshot read on a non-covering index defers its data lookup to a later seek (OP_DeferredSeek). If the scan releases locks in the meantime to let a waiting replication thread through, a concurrent DELETE can commit underneath it and the seek then fails with "Dta lookup lost the race". The same window loses out-of-line blobs, which is worse: no layer below sql reports a missed blob, so the row comes back with a silently zero-length column. The fix is to pre-fetch before releasing. Each index cursor's paired data cursor -- the pairing is recorded when the vdbe emits the hint -- is read to its current row, blobs included, so the deferred seek is served from the captured row instead of going back to the btree. This lets the release itself widen to where the race actually lives: previously only SERIAL released here, now every non-SNAPISOL mode does, including the default read-committed path. To keep that safe the new release drops page locks only, holding the curtran and its table read locks so a schema change still cannot slip in; a waiter parked too long falls back to the old full release rather than starve behind a long scan. For the paths that do drop table locks, cursors re-resolve their table through the thread's rootpage map and compare table version, so a query racing a DROP reports CDB2ERR_SCHEMA instead of touching freed memory. The visible cost is that readers now hand page locks back under contention where they used to hold them for the length of a scan. That is the intended trade, but it is a real behaviour change: rep_blocker.test measures exactly how long a reader can block replication and turns the feature off. Tunables: recover_deadlock_sync_dta (on) enables the sync; pagelock_release_interval_ms (100) paces re-releases while a waiter persists; pagelock_release_max_wait_ms (60000) is the full-release fallback, 0 to disable. Adds tests/recovdlock.test. Signed-off-by: Dorin Hogea <dhogea@bloomberg.net>
9551810 to
418aa72
Compare
roborivers
left a comment
roborivers
left a comment
There was a problem hiding this comment.
Cbuild submission: Success ✓.
Regression testing: Success ✓.
The first 10 failing tests are:
analyze_partial_index_off_generated [failed with core dumped] **quarantined**
analyze [failed with core dumped] **quarantined**
sc_resume_logicalsc_generated **quarantined**
consumer_non_atomic_default_consumer_generated **quarantined**
sc_downgrade [timeout] **quarantined**
skipscan [timeout] **quarantined**
|
cdb2test Sep 17 11:53:59 2026 success recovdlock-sync-dta-reland.R20260917.7 |
2 participants
The original merge of this PR (#6035) was not passing legacy regression tests due to changes in sqlite plan operation id numbers.
It was reverted here #6210.
We'll submit again here and take care of still failing legacy tests before merging.
FTR , the original PR description:
A non-snapshot read on a non-covering index defers its data lookup to a later seek (OP_DeferredSeek). If the scan releases locks in the meantime to let a waiting replication thread through, a concurrent DELETE can commit underneath it and the seek then fails with "Dta lookup lost the race". The same window loses out-of-line blobs, which is worse: no layer below sql reports a missed blob, so the row comes back with a silently zero-length column.
The fix is to pre-fetch before releasing. Each index cursor's paired data cursor -- the pairing is recorded when the vdbe emits the hint -- is read to its current row, blobs included, so the deferred seek is served from the captured row instead of going back to the btree. This lets the release itself widen to where the race actually lives: previously only SERIAL released here, now every non-SNAPISOL mode does, including the default read-committed path. To keep that safe the new release drops page locks only, holding the curtran and its table read locks so a schema change still cannot slip in; a waiter parked too long falls back to the old full release rather than starve behind a long scan. For the paths that do drop table locks, cursors re-resolve their table through the thread's rootpage map and compare table version, so a query racing a DROP reports CDB2ERR_SCHEMA instead of touching freed memory.
The visible cost is that readers now hand page locks back under contention where they used to hold them for the length of a scan. That is the intended trade, but it is a real behaviour change: rep_blocker.test measures exactly how long a reader can block replication and turns the feature off.
Tunables: recover_deadlock_sync_dta (on) enables the sync; pagelock_release_interval_ms (100) paces re-releases while a waiter persists; pagelock_release_max_wait_ms (60000) is the full-release fallback, 0 to disable. Adds tests/recovdlock.test.