From 459fba084cd39104c0055765170007452348b0ec Mon Sep 17 00:00:00 2001 From: Arul Ajmani Date: Thu, 14 Dec 2023 20:22:53 +0530 Subject: [PATCH] kv: disable use of shared locks in conjunction with skip locked We currently don't support the use of shared locks and the skip locked wait policy together. Until this limitation is lifted, we should return unimplemented errors instead of the wrong result. Informs https://github.com/cockroachdb/cockroach/issues/110743 Release note: None --- pkg/kv/kvnemesis/BUILD.bazel | 1 + pkg/kv/kvnemesis/applier.go | 5 +++ .../TestApplier/get-for-share-skip-locked | 2 +- ...or-share-skip-locked-guaranteed-durability | 2 +- .../TestApplier/rscan-for-share-skip-locked | 2 +- ...or-share-skip-locked-guaranteed-durability | 2 +- .../TestApplier/scan-for-share-skip-locked | 2 +- ...or-share-skip-locked-guaranteed-durability | 2 +- pkg/kv/kvnemesis/validator.go | 2 + pkg/kv/kvserver/batcheval/cmd_get.go | 4 ++ pkg/kv/kvserver/batcheval/cmd_reverse_scan.go | 4 ++ pkg/kv/kvserver/batcheval/cmd_scan.go | 39 +++++++++++++++++++ .../testdata/logic_test/select_for_share | 6 +++ 13 files changed, 67 insertions(+), 6 deletions(-) diff --git a/pkg/kv/kvnemesis/BUILD.bazel b/pkg/kv/kvnemesis/BUILD.bazel index 49c219186d8b..e073333f2cd6 100644 --- a/pkg/kv/kvnemesis/BUILD.bazel +++ b/pkg/kv/kvnemesis/BUILD.bazel @@ -27,6 +27,7 @@ go_library( "//pkg/kv/kvnemesis/kvnemesisutil", "//pkg/kv/kvpb", "//pkg/kv/kvserver", + "//pkg/kv/kvserver/batcheval", "//pkg/kv/kvserver/concurrency", "//pkg/kv/kvserver/concurrency/isolation", "//pkg/kv/kvserver/concurrency/lock", diff --git a/pkg/kv/kvnemesis/applier.go b/pkg/kv/kvnemesis/applier.go index 4c6b48bca640..6e6902d67f82 100644 --- a/pkg/kv/kvnemesis/applier.go +++ b/pkg/kv/kvnemesis/applier.go @@ -18,6 +18,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv" "github.com/cockroachdb/cockroach/pkg/kv/kvnemesis/kvnemesisutil" "github.com/cockroachdb/cockroach/pkg/kv/kvpb" + "github.com/cockroachdb/cockroach/pkg/kv/kvserver/batcheval" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/lock" "github.com/cockroachdb/cockroach/pkg/roachpb" @@ -119,6 +120,10 @@ func exceptSkipLockedReplayError(err error) bool { // true if skip locked replay return errors.Is(err, &concurrency.SkipLockedReplayError{}) } +func exceptSkipLockedUnsupportedError(err error) bool { // true if unsupported use of skip locked error + return errors.Is(err, &batcheval.SkipLockedUnsupportedError{}) +} + func applyOp(ctx context.Context, env *Env, db *kv.DB, op *Operation) { switch o := op.GetValue().(type) { case *GetOperation, diff --git a/pkg/kv/kvnemesis/testdata/TestApplier/get-for-share-skip-locked b/pkg/kv/kvnemesis/testdata/TestApplier/get-for-share-skip-locked index aac314839b99..2a8dfa645bff 100644 --- a/pkg/kv/kvnemesis/testdata/TestApplier/get-for-share-skip-locked +++ b/pkg/kv/kvnemesis/testdata/TestApplier/get-for-share-skip-locked @@ -1,3 +1,3 @@ echo ---- -db0.GetForShareSkipLocked(ctx, tk(1)) // @ (v1, ) +db0.GetForShareSkipLocked(ctx, tk(1)) // usage of shared locks in conjunction with skip locked wait policy is currently unsupported diff --git a/pkg/kv/kvnemesis/testdata/TestApplier/get-for-share-skip-locked-guaranteed-durability b/pkg/kv/kvnemesis/testdata/TestApplier/get-for-share-skip-locked-guaranteed-durability index 30c8e9112b4b..c5fcf8617c9e 100644 --- a/pkg/kv/kvnemesis/testdata/TestApplier/get-for-share-skip-locked-guaranteed-durability +++ b/pkg/kv/kvnemesis/testdata/TestApplier/get-for-share-skip-locked-guaranteed-durability @@ -1,3 +1,3 @@ echo ---- -db0.GetForShareSkipLockedGuaranteedDurability(ctx, tk(1)) // @ (v1, ) +db0.GetForShareSkipLockedGuaranteedDurability(ctx, tk(1)) // usage of shared locks in conjunction with skip locked wait policy is currently unsupported diff --git a/pkg/kv/kvnemesis/testdata/TestApplier/rscan-for-share-skip-locked b/pkg/kv/kvnemesis/testdata/TestApplier/rscan-for-share-skip-locked index bcbe73914385..40e167e7e0f9 100644 --- a/pkg/kv/kvnemesis/testdata/TestApplier/rscan-for-share-skip-locked +++ b/pkg/kv/kvnemesis/testdata/TestApplier/rscan-for-share-skip-locked @@ -1,3 +1,3 @@ echo ---- -db0.ReverseScanForShareSkipLocked(ctx, tk(1), tk(2), 0) // @ (/Table/100/"0000000000000001":v21, ) +db0.ReverseScanForShareSkipLocked(ctx, tk(1), tk(2), 0) // usage of shared locks in conjunction with skip locked wait policy is currently unsupported diff --git a/pkg/kv/kvnemesis/testdata/TestApplier/rscan-for-share-skip-locked-guaranteed-durability b/pkg/kv/kvnemesis/testdata/TestApplier/rscan-for-share-skip-locked-guaranteed-durability index 2aefd1fb4d33..8e190a97647e 100644 --- a/pkg/kv/kvnemesis/testdata/TestApplier/rscan-for-share-skip-locked-guaranteed-durability +++ b/pkg/kv/kvnemesis/testdata/TestApplier/rscan-for-share-skip-locked-guaranteed-durability @@ -1,3 +1,3 @@ echo ---- -db0.ReverseScanForShareSkipLockedGuaranteedDurability(ctx, tk(1), tk(2), 0) // @ (/Table/100/"0000000000000001":v21, ) +db0.ReverseScanForShareSkipLockedGuaranteedDurability(ctx, tk(1), tk(2), 0) // usage of shared locks in conjunction with skip locked wait policy is currently unsupported diff --git a/pkg/kv/kvnemesis/testdata/TestApplier/scan-for-share-skip-locked b/pkg/kv/kvnemesis/testdata/TestApplier/scan-for-share-skip-locked index b829978f2a57..7385f941513e 100644 --- a/pkg/kv/kvnemesis/testdata/TestApplier/scan-for-share-skip-locked +++ b/pkg/kv/kvnemesis/testdata/TestApplier/scan-for-share-skip-locked @@ -1,3 +1,3 @@ echo ---- -db0.ScanForShareSkipLocked(ctx, tk(1), tk(3), 0) // @ (/Table/100/"0000000000000001":v1, ) +db0.ScanForShareSkipLocked(ctx, tk(1), tk(3), 0) // usage of shared locks in conjunction with skip locked wait policy is currently unsupported diff --git a/pkg/kv/kvnemesis/testdata/TestApplier/scan-for-share-skip-locked-guaranteed-durability b/pkg/kv/kvnemesis/testdata/TestApplier/scan-for-share-skip-locked-guaranteed-durability index 2bdc41f053da..0844aaa01ed2 100644 --- a/pkg/kv/kvnemesis/testdata/TestApplier/scan-for-share-skip-locked-guaranteed-durability +++ b/pkg/kv/kvnemesis/testdata/TestApplier/scan-for-share-skip-locked-guaranteed-durability @@ -1,3 +1,3 @@ echo ---- -db0.ScanForShareSkipLockedGuaranteedDurability(ctx, tk(1), tk(3), 0) // @ (/Table/100/"0000000000000001":v1, ) +db0.ScanForShareSkipLockedGuaranteedDurability(ctx, tk(1), tk(3), 0) // usage of shared locks in conjunction with skip locked wait policy is currently unsupported diff --git a/pkg/kv/kvnemesis/validator.go b/pkg/kv/kvnemesis/validator.go index 503875b2b415..59f1cef6e56d 100644 --- a/pkg/kv/kvnemesis/validator.go +++ b/pkg/kv/kvnemesis/validator.go @@ -770,6 +770,7 @@ func (v *validator) processOp(op Operation) { v.failIfError( op, t.Result, exceptRollback, exceptAmbiguous, exceptSharedLockPromotionError, exceptSkipLockedReplayError, + exceptSkipLockedUnsupportedError, ) ops := t.Ops @@ -1273,6 +1274,7 @@ func (v *validator) checkError( exceptDelRangeUsingTombstoneStraddlesRangeBoundary, exceptSharedLockPromotionError, exceptSkipLockedReplayError, + exceptSkipLockedUnsupportedError, } sl = append(sl, extraExceptions...) return v.failIfError(op, r, sl...) diff --git a/pkg/kv/kvserver/batcheval/cmd_get.go b/pkg/kv/kvserver/batcheval/cmd_get.go index 2fdf31bff733..219dff676dd8 100644 --- a/pkg/kv/kvserver/batcheval/cmd_get.go +++ b/pkg/kv/kvserver/batcheval/cmd_get.go @@ -33,6 +33,10 @@ func Get( h := cArgs.Header reply := resp.(*kvpb.GetResponse) + if err := maybeDisallowSkipLockedRequest(h, args.KeyLockingStrength); err != nil { + return result.Result{}, err + } + getRes, err := storage.MVCCGet(ctx, readWriter, args.Key, h.Timestamp, storage.MVCCGetOptions{ Inconsistent: h.ReadConsistency != kvpb.CONSISTENT, SkipLocked: h.WaitPolicy == lock.WaitPolicy_SkipLocked, diff --git a/pkg/kv/kvserver/batcheval/cmd_reverse_scan.go b/pkg/kv/kvserver/batcheval/cmd_reverse_scan.go index 3f2ab1a16c81..6ba58ce1534f 100644 --- a/pkg/kv/kvserver/batcheval/cmd_reverse_scan.go +++ b/pkg/kv/kvserver/batcheval/cmd_reverse_scan.go @@ -35,6 +35,10 @@ func ReverseScan( h := cArgs.Header reply := resp.(*kvpb.ReverseScanResponse) + if err := maybeDisallowSkipLockedRequest(h, args.KeyLockingStrength); err != nil { + return result.Result{}, err + } + var res result.Result var scanRes storage.MVCCScanResult var err error diff --git a/pkg/kv/kvserver/batcheval/cmd_scan.go b/pkg/kv/kvserver/batcheval/cmd_scan.go index 13397574979a..a713e71be506 100644 --- a/pkg/kv/kvserver/batcheval/cmd_scan.go +++ b/pkg/kv/kvserver/batcheval/cmd_scan.go @@ -14,11 +14,13 @@ import ( "context" "fmt" + "github.com/cockroachdb/cockroach/pkg/build" "github.com/cockroachdb/cockroach/pkg/kv/kvpb" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/batcheval/result" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/lock" "github.com/cockroachdb/cockroach/pkg/storage" "github.com/cockroachdb/cockroach/pkg/util/admission/admissionpb" + "github.com/cockroachdb/errors" ) func init() { @@ -36,6 +38,10 @@ func Scan( h := cArgs.Header reply := resp.(*kvpb.ScanResponse) + if err := maybeDisallowSkipLockedRequest(h, args.KeyLockingStrength); err != nil { + return result.Result{}, err + } + var res result.Result var scanRes storage.MVCCScanResult var err error @@ -133,3 +139,36 @@ func ScanReadCategory(ah kvpb.AdmissionHeader) storage.ReadCategory { } return readCategory } + +// maybeDisallowSkipLockedRequest returns an error if the skip locked wait +// policy is used in conjunction with shared locks. +// +// TODO(arul): this won't be needed once +// https://github.com/cockroachdb/cockroach/issues/110743 is addressed. Until +// then, we return unimplemented errors. +func maybeDisallowSkipLockedRequest(h kvpb.Header, str lock.Strength) error { + if h.WaitPolicy == lock.WaitPolicy_SkipLocked && str == lock.Shared { + return MarkSkipLockedUnsupportedError(errors.UnimplementedError( + errors.IssueLink{IssueURL: build.MakeIssueURL(110743)}, + "usage of shared locks in conjunction with skip locked wait policy is currently unsupported", + )) + } + return nil +} + +// SkipLockedUnsupportedError is used to mark errors resulting from unsupported +// (currently unimplemented) uses of the skip locked wait policy. +type SkipLockedUnsupportedError struct{} + +func (e *SkipLockedUnsupportedError) Error() string { + return "unsupported skip locked use error" +} + +// MarkSkipLockedUnsupportedError wraps the given error, if not nil, as a skip +// locked unsupported error. +func MarkSkipLockedUnsupportedError(cause error) error { + if cause == nil { + return nil + } + return errors.Mark(cause, &SkipLockedUnsupportedError{}) +} diff --git a/pkg/sql/logictest/testdata/logic_test/select_for_share b/pkg/sql/logictest/testdata/logic_test/select_for_share index ed596a00740a..db835a848a1c 100644 --- a/pkg/sql/logictest/testdata/logic_test/select_for_share +++ b/pkg/sql/logictest/testdata/logic_test/select_for_share @@ -104,6 +104,12 @@ user testuser statement ok COMMIT +statement ok +SET enable_shared_locking_for_serializable = true + +statement error usage of shared locks in conjunction with skip locked wait policy is currently unsupported +SELECT * FROM t FOR SHARE SKIP LOCKED + # TODO(arul): Add a test to show that the session setting doesn't apply to read # committed transactions. We currently can't issue SELECT FOR SHARE statements # in read committed transactions because durable locking hasn't been fully