[CBRD-27034] Serialize DELETE write-write conflicts on the MVCC stamp under a transient row lock - #7698
Open
hgryoo wants to merge 20 commits into
Open
[CBRD-27034] Serialize DELETE write-write conflicts on the MVCC stamp under a transient row lock#7698hgryoo wants to merge 20 commits into
hgryoo wants to merge 20 commits into
Conversation
…val meets an in-progress DELID A transient-row-lock DELETE releases its per-row lock once the delete is published, so a later writer that acquires the row lock can find an active other transaction's DELID in the record header -- a state the X-lock world never produced. Teach every late arrival to settle against the deleter's transaction self-lock (CBRD-26942) instead of misreading the row as deleted or spinning on a lock the deleter no longer holds: - locator_lock_and_get_object_internal: on an active-other DELID, wait on the owner's self-lock while holding the row lock (no third writer can re-stamp the row, so at most one wait is needed), then re-read and re-classify. A grant while the owner is still active is an invariant breach; fail the statement rather than spin. - btree unique probes (unique / non-unique): extend the insert-MVCCID wait to DELETE_RECORD_DELETE_IN_PROGRESS -- the row-lock fallback cannot serialize with a deleter that already released its lock. - FK existence check (btree_fk_object_does_exist): same extension; the object-lock suspend would be granted at once and busy-spin through the re-check loop until the deleter ends. logtb_is_active_other_mvccid is ported as the shared "is there anything to wait on" predicate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lete phase under a statement-snapshot pin Plan generation stops forcing PT_SELECT_INFO_MVCC_LOCK_NEEDED on DELETE, so the select phase reads with the snapshot only and the delete phase acquires the per-row X-lock itself (the existing need_locking mode; GROUP BY and derived-table plans keep the select-phase lock as before). Predicate reevaluation stays disabled: its machinery is unfinished -- the DELETE loop never binds a per-tuple inst_oid, and the reevaluation filters point into the select scan that is already closed by force time (measured: V_FALSE regardless of the predicate). Instead, a last version that is not visible to the statement snapshot was never checked against the predicate, and mvcc_update_reev_data::skip_unevaluated_version now skips such a row at the delete phase rather than deleting an unevaluated version. Under RR / SERIALIZABLE the existing isolation-conflict classification is unchanged; under READ COMMITTED the skip surfaces as 0 rows for that row (measured at ~0.1% under a predicate-invariant update race, matching the prior baseline of this delta). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Write-write ownership of a deleted row is the MVCCID stamp: once DELID is stamped on the record header and the row's index entries, a later writer settles against the deleter's transaction self-lock, and the per-row lock's remaining job -- atomicity of the claim across the row's replicas -- is done. Release it right after the row force instead of holding it to commit; the uncommitted write-lock footprint becomes O(transactions), not O(rows). - heap_delete_logical ensures the deleter's self-lock before the stamp can become observable (same pattern as the insert and update paths). - qexec_execute_delete releases via lock_unlock_object_donot_move_to_non2pl (the same discipline the reevaluation V_FALSE path already uses); the count-based unlock preserves grants taken by earlier statements of the same transaction. - The row is tracked like a lockless insert so 2PC prepare re-locks it (CBRD-27079 fallback); if the self-lock or the tracking cannot be ensured, the row lock is simply kept (baseline behavior). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… changed version DELETE joins the condition-only side of the reevaluation machinery that the UPDATE extension completed: plan generation keeps the condition-reevaluation flags, and the executor binds the tuple's instance OID per row -- the one piece the DELETE loop never had. When a row's version changed after the statement snapshot, the delete phase now re-checks the predicate against the latest version and deletes it on a match, exactly what the baseline's select-phase lock-and-wait produced. The silent skip of an updated-but- still-matching row is gone (measured: 0/2000 under a predicate-invariant update race, previously ~0.1%); a version that stops matching is skipped as before. Plans that generate no reevaluation data keep the snapshot-pin skip as the conservative path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…and drop the superseded 2PC tracking The early unlock was gated on skip_unevaluated_version as a stand-in for "the select phase did not lock"; with the condition-only reevaluation that flag is now false whenever reevaluation data exists, which would keep every reevaluating DELETE at the O(rows) footprint. Gate on need_locking directly -- it is true exactly when the transient protocol ran the delete-phase lock. logtb_track_lockless_insert is gone since CBRD-27079: the prepare record persists the MVCCID self-lock, so an in-doubt transaction keeps serializing late arrivals without per-row tracking. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
❌ TC Merge Gate — Merge BlockedOne or more TC PRs are still open. Please merge or close them before merging this PR. TC Repositories & Branches:
Steps to unblock:
|
🧪 TC Test Environment ReadyCircleCI Testing:
TC Repositories & Branches:
Next Steps:
|
Contributor
|
Reviews (1): Last reviewed commit: "[CBRD-27034] Gate the delete early unloc..." | Re-trigger Greptile |
…end and trim comments to their invariants The S_LOCK-then-release wait protocol existed three times (btree helper, locator inline, and the btree key-level wrapper via the helper); since it now serves deleters as well as inserters, the btree names also lied (btree_wait_for_inserter_end waiting out a deleter). One logtb function carries the protocol; btree_key_wait_for_tran_end keeps only the latch/lock-release choreography, and the duplicate is_active predicate is gone. Comments now state the invariant or constraint and drop the narration; two grown stale (they still said DELETE has no reevaluation) are corrected. The wait sequence itself is unchanged: uncond S_LOCK on the owner's MVCCID self-lock, release, then fail the statement if the owner is somehow still active. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
/run all |
…e DELETE path The condition-only reevaluation reads MVCC_UPDDEL_REEV_DATA fields that only the UPDATE path fills (prepare_mvcc_reev_data): curr_assigns, curr_attrinfo, vd, cons_pred, the extra-assign pair, and the lazily built mvcc_cond_reev_list. qexec_execute_delete left them as stack garbage, so on release builds a DELETE with reevaluation data could take the assignment branch on a garbage pointer and fail the statement with the generic "Query execution failure" (-495). Debug builds masked it with zeroed stack. Caught by the medium suite's path-expression DELETEs (del1/del2/del4/del6/del7); all five pass on debug and release after the fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…faults Field-by-field zeroing at the call sites is how the DELETE path came to read stack garbage in the first place; the empty, condition-only state now lives on mvcc_update_reev_data itself, so every present and future stack instance starts safe and a call site only binds what it actually uses (the executor's value descriptor, and the UPDATE path's assignment fields via prepare_mvcc_reev_data). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
/run all |
Giving mvcc_update_reev_data member defaults made it non-trivial, so clearing it with memset is rejected (-Werror=class-memaccess). Assign a value-initialized instance: same empty state, now defined by the type rather than restated at the call site. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
/run all |
Member
Author
|
/run all |
… re-check the predicate The delete phase resolves each reevaluation class to its access spec by exact class OID (qexec_upddel_mvcc_set_filters). That resolution only holds for a spec the statement deletes from that scans a single class: a class the condition merely references is fetched rather than scanned, and a hierarchy scan reports subclass OIDs no spec carries. In both cases the helper finds no spec and fails the statement -- and it returns ER_FAILED with no error set, so the statement surfaces the generic "Query execution failure" (-495) instead of a diagnosis. Enabling DELETE reevaluation made that dormant path reachable: the medium suite's path-expression and hierarchy deletes (del1, del2, del4, del6, del7 on joe.shipment_c / joe.inventory_c) failed. Those statements now keep the select-phase lock, as they did before, so the delete phase never has to re-check what it cannot evaluate. The transient row lock still covers a DELETE from a single non-hierarchy class, which is what the footprint and reevaluation gates measure. Reproduced by running the medium suite the way CTP does -- one JDBC connection, _01_fixed then _02_xtests in name order -- where the five cases fail and, with this change, pass; all 26 del/updat cases match their answer files, and the write-write, footprint, reevaluation (skip 0/2000) and full-mix gates stay green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
/run all |
qexec_upddel_mvcc_set_filters returns ER_FAILED when it cannot match a reevaluation class to an access spec, without setting an error, so the statement surfaces the generic "Query execution failure" (-495) and the cause cannot be told from the message. Set ER_QPROC_INVALID_XASLNODE there, so this failure names itself. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e cannot re-check the predicate" This reverts commit 63088c3.
…ass no filters qexec_upddel_mvcc_set_filters matches a reevaluation class to an access spec by exact class OID. A DELETE whose predicate walks a path expression reads an inner query's list instead of scanning the class, so the plan carries no spec for it -- and the helper returned ER_FAILED without setting an error, which surfaced as the generic "Query execution failure" (-495). Enabling DELETE reevaluation made that dormant path reachable and broke the medium suite's path-expression deletes (del1, del2, del4, del6, del7). A plan that does not scan the class has no filters to re-evaluate with, which is a state to handle rather than an error: the helper now reports it through has_spec, and the delete phase falls back to skipping a version the statement never evaluated -- the same contract a plan carrying no reevaluation data already has. The update phase keeps treating it as a failure, as before. Verified by running the medium suite through CTP as CI does (975 cases, ha_mode on): 5 failures before, 0 after. Write-write, footprint, reevaluation (skip 0/2000) and full-mix gates stay green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
/run all |
…EY UPDATE The duplicate the unique probe returns may carry an in-progress delete by another transaction, which holds no per-row lock once it has published. Reading the visible version hides that delete, so the update overwrote a record its deleter still had to undo: rolling that deleter back then found no delete stamp on the record and failed the server (heap_rv_mvcc_undo_delete). Every other write path settles such a row by waiting on the deleter's MVCCID; this one read around the protocol. Fetch through locator_lock_and_get_object instead, which waits the deleter out and re-reads under the row lock. If the deleter committed in the meantime the key is free, so the row is no longer a duplicate and the caller inserts, exactly as it would have had the probe found nothing. Reproduced with the online-index stress from cbrd_22705 (100 sessions of update/delete/insert/replace/ODKU with rollbacks, against parallel CREATE INDEX): the server aborted within 3-5 runs before, and survived 8 consecutive runs after. develop does not abort. medium suite 975/975, write-write, footprint, reevaluation and full-mix gates green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The comments carried issue numbers as labels and narrated the change rather than what holds afterwards. Say the invariant instead, so each one still reads correctly after the next rewrite of the code under it, and inline the external fact (the prepare record persists the self-lock across 2PC) rather than pointing at an issue for it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
/run all |
locator_lock_and_get_object_with_evaluation () left mvcc_reev_data->filter_result untouched when the last version was visible to the statement snapshot: that object needs no reevaluation, the visible version being the one already evaluated. But the verdict is initialized once per statement, so the previous object's V_FALSE was still sitting there, and the callers that read the field right after the call skipped an object that had failed nothing. A DELETE of ten matching rows, one of them flipped by a concurrent transaction, deleted 0 rows instead of 9 -- the flipped row correctly failed reevaluation and the nine untouched rows inherited its verdict. On develop the two UPDDEL callers could not reach this, both taking the already-locked-in-select-phase path; a multi-object qexec_execute_selupd_list could, silently skipping an increment. The field now describes the fetch that just happened: V_TRUE on entry, and every path that decides otherwise writes its own result. This is the discipline the scan path already keeps by calling set_scan_reevaluation () before each fetch. Reported by Greptile on CUBRID#7711. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
/run all |
prepare_mvcc_reev_data () value-initializes the reevaluation struct and then returns early when the statement carries no reevaluation class, so the value descriptor was bound only on the path that has one. Bind it before that early return and drop the two assignments the change makes redundant -- the one in qexec_execute_delete (), which prepare_mvcc_reev_data () overwrote immediately, and the later duplicate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…spec qexec_upddel_mvcc_set_filters () used to return ER_FAILED when the plan holds no access spec for the class, and both update call sites went to the error exit. Reporting the case through has_spec left those call sites ignoring it, so the update phase ran on with a reevaluation entry whose filters init () never filled. Act on has_spec there and keep failing the statement; only the delete phase is meant to treat the case as "no reevaluation possible". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… counter mvcc_reev_class_cnt is the bound of the loop that consumes the OID - CLASS OID pairs of the scan's value list, so zeroing it to say "reevaluation is off" ended that loop early and left the pairs of the remaining reevaluation classes unconsumed. The trailing LOB loop then read one of them as a LOB: an assert in a debug build, and in a release build db_get_elo () reading the OID union. Record the state in its own flag and consult it only where the reevaluation data is used -- the set-filters call, the scan order setup, and the per class loop. That loop also indexes mvcc_reev_classes without an upper bound, which reads past the array once there are fewer reevaluation classes than deleted classes; bound it while here. Reproduced by multi_delete_3tables.sql and cbrd_25382_2.sql. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
num_reev_classes was counted over the delete statement's spec list, while mvcc_reev_classes[] is filled from the generated SELECT's OID column list. A class the SELECT drops -- an outer joined table none of whose columns the outer query reads -- is counted but never gets an entry, so the tail of the array stays uninitialized and the delete phase walks the value list past its end. Report the number of entries the loop wrote, and bound the loop so it cannot run past the allocation in the other direction either. Reproduced by multi_delete_using_bit.sql. Unreachable until this branch, since reevaluation was disabled for DELETE before the count was taken. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
/run all |
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
1 participant
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.
http://jira.cubrid.org/browse/CBRD-27034
Purpose
An MVCC
DELETEholds a per-row X-lock until commit: O(rows) lock-table entries, longer bucket chains, and escalation to a table X-lock pastlock_escalation_at.mvcc_del_id, and is retained until commit.ER_CANNOT_GET_LOCKrather than spin.Uncommitted write-lock footprint becomes O(transactions): 1 granted X entry while holding an uncommitted 1,900-row DELETE (baseline ~1,900).
Implementation
logtb_is_active_other_mvccid(the is-another-writer-active predicate) andlogtb_wait_for_tran_end(the wait on an owner's MVCCID self-lock: uncond S_LOCK, release; a no-op grant fails the statement instead of spinning).skip_unevaluated_version), not modified unevaluated.locator_lock_and_get_object_with_evaluationestablishes the reevaluation verdict for the fetch it performs (V_TRUEon entry). It used to leavefilter_resultuntouched when the last version was visible — that object needs no reevaluation — but the field is initialized once per statement, so the previous object'sV_FALSEwas read back as this one's: a DELETE of ten matching rows with one of them flipped by a concurrent transaction deleted 0 rows instead of 9. Reachable here because DELETE now reevaluates; a multi-objectqexec_execute_selupd_listcan reach it on develop as well. Reported by Greptile on [CBRD-27238] Extend the transient row lock to UPDATE, with reevaluation against the latest version #7711.mvcc_reev_classesfor condition-only reevaluation (GROUP BY / derived-table plans keep the select-phase lock as before).inst_oidper tuple for reevaluation and releases the row lock at publish;heap_delete_logicalensures the MVCCID self-lock is held before the DELID stamp becomes observable, so "an observable in-progress DELID implies a held self-lock" holds by construction.Remarks
SELECT ... FOR UPDATE, non-MVCC operations and DDL are out of scope and unchanged; UPDATE keeps its per-row X-lock in this PR. Follow-ups: UPDATE on the same protocol, then the lockless FK existence check (CBRD-26664).Test
Debug build, asserts active: write-write endings (second deleter waits, then 0-rows on commit / deletes on rollback), FK child-insert vs parent-delete both endings, deadlock resolves with a single victim, savepoint partial rollback, REC_BIGONE rows, multi-class DELETE; 90s full-mix gate (3 deleters + 2 inserters + 3 key-moving updaters): heap count == index count, checkdb clean; reevaluation gate: 0 skips / 2,000; the reevaluation verdict is not inherited across rows (10 matching rows, 1 flipped mid-statement → 9 deleted, 1 left). Medium suite as CI runs it: Fail 0 / Success 975 / Total 975.
Release build: hot-row del/upd mix 30s/11s vs baseline 34s/38s (a sealed prototype: 156s/179s);
Num_object_locks_waits−46% (cross-checked against self-lock accounting); data-page fetches and cold-buffer ioreads at parity; during a single 150k-row DELETE (lock_escalation_at=100k) a concurrent writer completed 293 single-row inserts vs 51 on baseline, the bulk statement itself at parity; 10-cycle soak with checkdb between cycles.