Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kvserver: unexpected non-locking read handling during FK check #111429

Closed
michae2 opened this issue Sep 28, 2023 · 3 comments
Closed

kvserver: unexpected non-locking read handling during FK check #111429

michae2 opened this issue Sep 28, 2023 · 3 comments
Assignees
Labels
A-read-committed Related to the introduction of Read Committed C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@michae2
Copy link
Collaborator

michae2 commented Sep 28, 2023

With #110945 applied, we have replicated locks wired up to SQL for the first time. It looks like we're running into a panic when performing a FK check, which tries to take a shared guaranteed-durable lock on a single row. The FK check is performing a Scan [/Table/104/1/1/0,/Table/104/1/1/1) with shared guaranteed-durable locking, which results in this panic:

* ERROR: a panic has occurred!
* unexpected non-locking read handling
* (1) attached stack trace
*   -- stack trace:
*   | runtime.gopanic
*   | 	GOROOT/src/runtime/panic.go:884
*   | github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Store).SendWithWriteBytes.func1
*   | 	github.com/cockroachdb/cockroach/pkg/kv/kvserver/pkg/kv/kvserver/store_send.go:109
*   | runtime.gopanic
*   | 	GOROOT/src/runtime/panic.go:890
*   | github.com/cockroachdb/cockroach/pkg/kv/kvserver/batcheval.DefaultDeclareIsolatedKeys
*   | 	github.com/cockroachdb/cockroach/pkg/kv/kvserver/batcheval/declare.go:98
*   | github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Replica).collectSpans
*   | 	github.com/cockroachdb/cockroach/pkg/kv/kvserver/pkg/kv/kvserver/replica_send.go:1192
*   | github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Replica).executeBatchWithConcurrencyRetries
*   | 	github.com/cockroachdb/cockroach/pkg/kv/kvserver/pkg/kv/kvserver/replica_send.go:457
*   | github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Replica).SendWithWriteBytes
*   | 	github.com/cockroachdb/cockroach/pkg/kv/kvserver/pkg/kv/kvserver/replica_send.go:189
*   | github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Store).SendWithWriteBytes
*   | 	github.com/cockroachdb/cockroach/pkg/kv/kvserver/pkg/kv/kvserver/store_send.go:193
*   | github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Stores).SendWithWriteBytes
*   | 	github.com/cockroachdb/cockroach/pkg/kv/kvserver/pkg/kv/kvserver/stores.go:202
*   | github.com/cockroachdb/cockroach/pkg/server.(*Node).batchInternal
*   | 	github.com/cockroachdb/cockroach/pkg/server/node.go:1328
*   | github.com/cockroachdb/cockroach/pkg/server.(*Node).Batch
*   | 	github.com/cockroachdb/cockroach/pkg/server/node.go:1459
*   | github.com/cockroachdb/cockroach/pkg/rpc.makeInternalClientAdapter.func1
*   | 	github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:704
*   | github.com/cockroachdb/cockroach/pkg/util/tracing/grpcinterceptor.ServerInterceptor.func1
*   | 	github.com/cockroachdb/cockroach/pkg/util/tracing/grpcinterceptor/grpc_interceptor.go:97
*   | github.com/cockroachdb/cockroach/pkg/rpc.bindUnaryServerInterceptorToHandler.func1
*   | 	github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:815
*   | github.com/cockroachdb/cockroach/pkg/rpc.NewServerEx.func3
*   | 	github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:169
*   | github.com/cockroachdb/cockroach/pkg/rpc.bindUnaryServerInterceptorToHandler.func1
*   | 	github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:815
*   | github.com/cockroachdb/cockroach/pkg/rpc.kvAuth.unaryInterceptor
*   | 	github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/auth.go:105
*   | github.com/cockroachdb/cockroach/pkg/rpc.bindUnaryServerInterceptorToHandler.func1
*   | 	github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:815
*   | github.com/cockroachdb/cockroach/pkg/rpc.NewServerEx.func1.1
*   | 	github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:136
*   | github.com/cockroachdb/cockroach/pkg/util/stop.(*Stopper).RunTaskWithErr
*   | 	github.com/cockroachdb/cockroach/pkg/util/stop/stopper.go:336
*   | github.com/cockroachdb/cockroach/pkg/rpc.NewServerEx.func1
*   | 	github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:134
*   | github.com/cockroachdb/cockroach/pkg/rpc.bindUnaryServerInterceptorToHandler.func1
*   | 	github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:815
*   | github.com/cockroachdb/cockroach/pkg/rpc.makeInternalClientAdapter.func2
*   | 	github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:714
*   | github.com/cockroachdb/cockroach/pkg/util/tracing/grpcinterceptor.ClientInterceptor.func2
*   | 	github.com/cockroachdb/cockroach/pkg/util/tracing/grpcinterceptor/grpc_interceptor.go:228
*   | github.com/cockroachdb/cockroach/pkg/rpc.getChainUnaryInvoker.func1
*   | 	github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:899
*   | github.com/cockroachdb/cockroach/pkg/rpc.makeInternalClientAdapter.func3
*   | 	github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:784
*   | github.com/cockroachdb/cockroach/pkg/rpc.internalClientAdapter.Batch
*   | 	github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:907
*   | github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord.(*grpcTransport).sendBatch
*   | 	github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord/transport.go:211
*   | github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord.(*grpcTransport).SendNext
*   | 	github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord/transport.go:189
*   | github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord.(*DistSender).sendToReplicas
*   | 	github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord/dist_sender.go:2380
*   | github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord.(*DistSender).sendPartialBatch
*   | 	github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord/dist_sender.go:1875
* Wraps: (2) assertion failure
* Wraps: (3) attached stack trace
*   -- stack trace:
*   | github.com/cockroachdb/cockroach/pkg/kv/kvserver/batcheval.DefaultDeclareIsolatedKeys
*   | 	github.com/cockroachdb/cockroach/pkg/kv/kvserver/batcheval/declare.go:98
*   | github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Replica).collectSpans
*   | 	github.com/cockroachdb/cockroach/pkg/kv/kvserver/pkg/kv/kvserver/replica_send.go:1192
*   | github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Replica).executeBatchWithConcurrencyRetries
*   | 	github.com/cockroachdb/cockroach/pkg/kv/kvserver/pkg/kv/kvserver/replica_send.go:457
*   | github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Replica).SendWithWriteBytes
*   | 	github.com/cockroachdb/cockroach/pkg/kv/kvserver/pkg/kv/kvserver/replica_send.go:189
*   | github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Store).SendWithWriteBytes
*   | 	github.com/cockroachdb/cockroach/pkg/kv/kvserver/pkg/kv/kvserver/store_send.go:193
*   | github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Stores).SendWithWriteBytes
*   | 	github.com/cockroachdb/cockroach/pkg/kv/kvserver/pkg/kv/kvserver/stores.go:202
*   | github.com/cockroachdb/cockroach/pkg/server.(*Node).batchInternal
*   | 	github.com/cockroachdb/cockroach/pkg/server/node.go:1328
*   | github.com/cockroachdb/cockroach/pkg/server.(*Node).Batch
*   | 	github.com/cockroachdb/cockroach/pkg/server/node.go:1459
*   | github.com/cockroachdb/cockroach/pkg/rpc.makeInternalClientAdapter.func1
*   | 	github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:704
*   | github.com/cockroachdb/cockroach/pkg/util/tracing/grpcinterceptor.ServerInterceptor.func1
*   | 	github.com/cockroachdb/cockroach/pkg/util/tracing/grpcinterceptor/grpc_interceptor.go:97
*   | github.com/cockroachdb/cockroach/pkg/rpc.bindUnaryServerInterceptorToHandler.func1
*   | 	github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:815
*   | github.com/cockroachdb/cockroach/pkg/rpc.NewServerEx.func3
*   | 	github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:169
*   | github.com/cockroachdb/cockroach/pkg/rpc.bindUnaryServerInterceptorToHandler.func1
*   | 	github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:815
*   | github.com/cockroachdb/cockroach/pkg/rpc.kvAuth.unaryInterceptor
*   | 	github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/auth.go:105
*   | github.com/cockroachdb/cockroach/pkg/rpc.bindUnaryServerInterceptorToHandler.func1
*   | 	github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:815
*   | github.com/cockroachdb/cockroach/pkg/rpc.NewServerEx.func1.1
*   | 	github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:136
*   | github.com/cockroachdb/cockroach/pkg/util/stop.(*Stopper).RunTaskWithErr
*   | 	github.com/cockroachdb/cockroach/pkg/util/stop/stopper.go:336
*   | github.com/cockroachdb/cockroach/pkg/rpc.NewServerEx.func1
*   | 	github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:134
*   | github.com/cockroachdb/cockroach/pkg/rpc.bindUnaryServerInterceptorToHandler.func1
*   | 	github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:815
*   | github.com/cockroachdb/cockroach/pkg/rpc.makeInternalClientAdapter.func2
*   | 	github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:714
*   | github.com/cockroachdb/cockroach/pkg/util/tracing/grpcinterceptor.ClientInterceptor.func2
*   | 	github.com/cockroachdb/cockroach/pkg/util/tracing/grpcinterceptor/grpc_interceptor.go:228
*   | github.com/cockroachdb/cockroach/pkg/rpc.getChainUnaryInvoker.func1
*   | 	github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:899
*   | github.com/cockroachdb/cockroach/pkg/rpc.makeInternalClientAdapter.func3
*   | 	github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:784
*   | github.com/cockroachdb/cockroach/pkg/rpc.internalClientAdapter.Batch
*   | 	github.com/cockroachdb/cockroach/pkg/rpc/pkg/rpc/context.go:907
*   | github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord.(*grpcTransport).sendBatch
*   | 	github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord/transport.go:211
*   | github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord.(*grpcTransport).SendNext
*   | 	github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord/transport.go:189
*   | github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord.(*DistSender).sendToReplicas
*   | 	github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord/dist_sender.go:2380
*   | github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord.(*DistSender).sendPartialBatch
*   | 	github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord/dist_sender.go:1875
*   | github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord.(*DistSender).divideAndSendBatchToRanges
*   | 	github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord/dist_sender.go:1446
*   | github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord.(*DistSender).Send
*   | 	github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord/dist_sender.go:1066
*   | github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord.(*txnLockGatekeeper).SendLocked
*   | 	github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord/txn_lock_gatekeeper.go:82
*   | github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord.(*txnMetricRecorder).SendLocked
*   | 	github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord/txn_interceptor_metric_recorder.go:47
* Wraps: (4) unexpected non-locking read handling
* Error types: (1) *withstack.withStack (2) *assert.withAssertionFailure (3) *withstack.withStack (4) *errutil.leafError

To reproduce it, use the PR branch:

git remote add michae2 [email protected]:michae2/cockroach.git
git fetch michae2
git checkout golden_spike
./dev build short
./cockroach demo

The repro steps are to execute an INSERT that has a FK check:

SET CLUSTER SETTING sql.txn.read_committed_syntax.enabled = true;
CREATE TABLE jars (j INT PRIMARY KEY);
CREATE TABLE cookies (c INT PRIMARY KEY, j INT REFERENCES jars (j));
SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL READ COMMITTED;
INSERT INTO jars VALUES (1), (2);
INSERT INTO cookies VALUES (1, 1);

(This can also be reproduced with ./dev testlogic base --config=local --files=fk_read_committed which is a logic test that does the same thing.)

Here's the explain of that insert:

[email protected]:26257/demoapp/defaultdb> EXPLAIN (OPT) INSERT INTO cookies VALUES (1, 1);
                              info
----------------------------------------------------------------
  insert cookies
   ├── values
   │    └── (1, 1)
   └── f-k-checks
        └── f-k-checks-item: cookies(j) -> jars(j)
             └── anti-join (lookup jars)
                  ├── lookup columns are key
                  ├── locking: for-share,durability-guaranteed
                  ├── with-scan &1
                  └── filters (true)
(10 rows)

Time: 5ms total (execution 4ms / network 0ms)

[email protected]:26257/demoapp/defaultdb> EXPLAIN INSERT INTO cookies VALUES (1, 1);
                    info
---------------------------------------------
  distribution: local
  vectorized: true

  • insert fast path
    into: cookies(c, j)
    auto commit
    FK check: jars@jars_pkey
    FK check locking strength: for share
    FK check locking durability: guaranteed
    size: 2 columns, 1 row
(10 rows)

Time: 2ms total (execution 2ms / network 0ms)

This happens regardless of whether we're using enable_insert_fast_path or not.

Jira issue: CRDB-31894

@michae2 michae2 added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-read-committed Related to the introduction of Read Committed labels Sep 28, 2023
@michae2 michae2 self-assigned this Sep 28, 2023
@michae2
Copy link
Collaborator Author

michae2 commented Sep 28, 2023

Ah, Nathan figured it out: #110945 (comment)

arulajmani added a commit to arulajmani/cockroach that referenced this issue Sep 28, 2023
Over in cockroachdb#111429, we saw a non-locking read request which specified
a key locking durability of lock.Replicated. This is not allowed --
however, the resulting panic message can be improved.

Epic: none

Release note: None
@michae2
Copy link
Collaborator Author

michae2 commented Sep 28, 2023

This will be fixed by #109638

arulajmani added a commit to arulajmani/cockroach that referenced this issue Sep 29, 2023
Over in cockroachdb#111429, we saw a non-locking read request which specified
a key locking durability of lock.Replicated. This is not allowed --
however, it doesn't warrant a panic on the server. We fix this and
improve the error message.

Epic: none

Release note: None
craig bot pushed a commit that referenced this issue Sep 29, 2023
111190: sql: add SHOW EXPERIMENTAL_FINGERPRINTS FROM VIRTUAL CLUSTER r=knz,msbutler a=stevendanna

This adds a user-facing interface for tenant fingerprinting that does not require crdb_internal functions.

We've moved the underlying implementation to a function that lives on the planner so that it can be shared from both call sites.

Additionally,

- we now issue the ExportRequests in parallel, and
- we allow users with MANAGETENANT to issue the fingerprint command.

Epic: none

Release note: None

111432: kv: do not panic when non-locking read requests ask for replicated locks r=nvanbenschoten a=arulajmani

Over in #111429, we saw a non-locking read request which specified
a key locking durability of lock.Replicated. This is not allowed --
however, it doesn't warrant a panic on the server. We fix this and
improve the error message.

Epic: none

Release note: None

Co-authored-by: Steven Danna <[email protected]>
Co-authored-by: Arul Ajmani <[email protected]>
THardy98 pushed a commit to THardy98/cockroach that referenced this issue Oct 6, 2023
Over in cockroachdb#111429, we saw a non-locking read request which specified
a key locking durability of lock.Replicated. This is not allowed --
however, it doesn't warrant a panic on the server. We fix this and
improve the error message.

Epic: none

Release note: None
@michae2
Copy link
Collaborator Author

michae2 commented Oct 10, 2023

Fixed.

@michae2 michae2 closed this as completed Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-read-committed Related to the introduction of Read Committed C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

No branches or pull requests

1 participant