kv/kvserver: TestSharedLocksBasic failed during spanset assertion #111409
Assignees
Labels
A-kv-transactions
Relating to MVCC and the transactional model.
branch-master
Failures and bugs on the master branch.
C-bug
Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
C-test-failure
Broken test (automatically or manually discovered).
O-robot
Originated from a bot.
release-blocker
Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked.
T-kv
KV Team
Milestone
Comments
cockroach-teamcity
added
branch-master
|
pav-kv
added
the
C-bug
|
Stressed this 10k times on master, couldn't reproduce on Mac or gceworker. Was this fixed recently? |
|
I think this has to do with how we declare latches for replicated shared lock acquisition. We're declaring a read latch at MaxTimestamp, not a write latch. This is safe because the writes from different transaction IDs are to different keys and the writes to the same transaction ID are protected by the write latch added in #111388. But the spanset assertions don't know this. So I think we'll want a fix like: diff --git a/pkg/kv/kvserver/spanset/batch.go b/pkg/kv/kvserver/spanset/batch.go
index 3f5efb08d57..3900e97d36c 100644
--- a/pkg/kv/kvserver/spanset/batch.go
+++ b/pkg/kv/kvserver/spanset/batch.go
@@ -891,6 +891,10 @@ func addLockTableSpans(spans *SpanSet) *SpanSet {
if span.EndKey != nil {
ltEndKey, _ = keys.LockTableSingleKey(span.EndKey, nil)
}
+ if sa == SpanReadOnly && span.Timestamp == hlc.MaxTimestamp {
+ // TODO: explain.
+ sa = SpanReadWrite
+ }
withLocks.AddNonMVCC(sa, roachpb.Span{Key: ltKey, EndKey: ltEndKey})
})
return withLocks |
nvanbenschoten
changed the title
|
Thanks for tracking this down @nvanbenschoten before I even got to my desk! I'll end out a patch today. |
|
kv/kvserver.TestSharedLocksBasic failed with artifacts on master @ d89e0f438dfd8fdd1e891bd222f88205458966f4: Parameters: |
craig bot
pushed a commit
that referenced
this issue
Sep 29, 2023
109638: sql: use the correct locking strength for FOR SHARE clause r=michae2 a=arulajmani Previously, FOR SHARE and FOR KEY SHARE would use non-locking KV scans. Now that the lock table supports shared locks, we can use lock.Shared as the locking strength for KV scans. This patch does that, and in doing so, wires up SHARED locks end to end. By default, we turn of this functionality for serializable transactions. Instead, it's gated behind a session setting called `enable_shared_locking_for_serializable`. Informs #91545 Release note (sql change): SELECT FOR SHARE and SELECT FOR UPDATE previously did not acquire any locks. Users issuing these statements would expect them to acquire shared locks (multiple readers allowed, no writers though). This patch switches over the behavior to acquire such read locks. For serializable transactions, we default to the old behaviour, unless the `enable_shared_locking_for_serializable` session setting is set to true. We'll probably switch this behavior in the future, but for now, given shared locks are a preview feature, we gate things behind a session setting. 111441: goschedstats: reduce underloadedRunnablePerProcThreshold r=aadityasondhi a=sumeerbhola By reducing this threshold, we more often sample the stats at 1ms intervals, which is desirable for admission control slot adjustment. Fixes #111125 Epic: none Release note: None 111466: ui: update default timescale to 1h r=maryliag a=maryliag On the Metrics page, the default value was 10min, which was too small. This commit updates it to 1h to match the most selected value on the SQL Activity. Fixes #96479 Release note: None 111521: roachprod: fix createTenantCertBundle script r=andy-kimball a=herkolategan The `createTenantCertBundle` function has a script in it that has erroneous "+" characters which results in an error when trying to start a secure cluster. This change removes those characters. Epic: None Release Note: None 111526: kv/spanset: permit writes to lock table by shared lock latching configuration r=nvanbenschoten a=nvanbenschoten Fixes #111409. Fixes #111492. This commit updates addLockTableSpans to account for the way that shared locking requests declare their latches. Even though they declare a "read" latch, they do so at max timestamp, which gives them sufficient isolation to write to the lock table without having to declare a write latch and be serialized with other shared lock acquisitions. Release note: None Co-authored-by: Arul Ajmani <[email protected]> Co-authored-by: sumeerbhola <[email protected]> Co-authored-by: maryliag <[email protected]> Co-authored-by: Herko Lategan <[email protected]> Co-authored-by: Nathan VanBenschoten <[email protected]>
THardy98
pushed a commit
to THardy98/cockroach
that referenced
this issue
Oct 6, 2023
…guration Fixes cockroachdb#111409. Fixes cockroachdb#111492. This commit updates addLockTableSpans to account for the way that shared locking requests declare their latches. Even though they declare a "read" latch, they do so at max timestamp, which gives them sufficient isolation to write to the lock table without having to declare a write latch and be serialized with other shared lock acquisitions. Release note: None
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
kv/kvserver.TestSharedLocksBasic failed with artifacts on master @ 2111b61b2d7c789bc03b1e9392062df80c779075:
Parameters:
TAGS=bazel,gss,stress=trueHelp
See also: How To Investigate a Go Test Failure (internal)
This test on roachdash | Improve this report!
Jira issue: CRDB-31888
The text was updated successfully, but these errors were encountered: