From 9fa4b04697881426ab22a9dc4c656a6b3aefb855 Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Sat, 6 Aug 2022 17:30:38 -0400 Subject: [PATCH 1/4] opt: add more tests for user-defined type dependency tracking This commit updates the expression formatter to format user-defined type dependencies, which were previously omitted. It also adds some additional tests to ensure that user-defined type dependency tracking is working as intended for `CREATE VIEW` and `CREATE FUNCTION`. Release note: None --- pkg/sql/opt/memo/BUILD.bazel | 1 + pkg/sql/opt/memo/expr_format.go | 20 +++++++-- .../opt/optbuilder/testdata/create_function | 42 ++++++++++++++++++- pkg/sql/opt/optbuilder/testdata/create_view | 26 ++++++++++++ 4 files changed, 83 insertions(+), 6 deletions(-) diff --git a/pkg/sql/opt/memo/BUILD.bazel b/pkg/sql/opt/memo/BUILD.bazel index 5e6a8106195e..dc373e51de2e 100644 --- a/pkg/sql/opt/memo/BUILD.bazel +++ b/pkg/sql/opt/memo/BUILD.bazel @@ -38,6 +38,7 @@ go_library( "//pkg/sql/rowenc/keyside", "//pkg/sql/rowenc/valueside", "//pkg/sql/sem/cast", + "//pkg/sql/sem/catid", "//pkg/sql/sem/eval", "//pkg/sql/sem/tree", "//pkg/sql/sem/tree/treewindow", diff --git a/pkg/sql/opt/memo/expr_format.go b/pkg/sql/opt/memo/expr_format.go index f2cc433a2e49..9ecb901a82ab 100644 --- a/pkg/sql/opt/memo/expr_format.go +++ b/pkg/sql/opt/memo/expr_format.go @@ -22,6 +22,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/opt/cat" "github.com/cockroachdb/cockroach/pkg/sql/opt/props" "github.com/cockroachdb/cockroach/pkg/sql/opt/props/physical" + "github.com/cockroachdb/cockroach/pkg/sql/sem/catid" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree/treewindow" "github.com/cockroachdb/cockroach/pkg/sql/types" @@ -700,11 +701,11 @@ func (f *ExprFmtCtx) formatRelational(e RelExpr, tp treeprinter.Node) { f.formatCol(col.Alias, col.ID, opt.ColSet{} /* notNullCols */) } tp.Child(f.Buffer.String()) - f.formatDependencies(tp, t.Deps) + f.formatDependencies(tp, t.Deps, t.TypeDeps) case *CreateFunctionExpr: tp.Child(t.Syntax.String()) - f.formatDependencies(tp, t.Deps) + f.formatDependencies(tp, t.Deps, t.TypeDeps) case *CreateStatisticsExpr: tp.Child(t.Syntax.String()) @@ -1510,8 +1511,10 @@ func (f *ExprFmtCtx) formatLockingWithPrefix( } // formatDependencies adds a new treeprinter child for schema dependencies. -func (f *ExprFmtCtx) formatDependencies(tp treeprinter.Node, deps opt.SchemaDeps) { - if len(deps) == 0 { +func (f *ExprFmtCtx) formatDependencies( + tp treeprinter.Node, deps opt.SchemaDeps, typeDeps opt.SchemaTypeDeps, +) { + if len(deps) == 0 && typeDeps.Empty() { tp.Child("no dependencies") return } @@ -1535,6 +1538,15 @@ func (f *ExprFmtCtx) formatDependencies(tp treeprinter.Node, deps opt.SchemaDeps } n.Child(f.Buffer.String()) } + for _, typ := range f.Memo.Metadata().AllUserDefinedTypes() { + typeID, err := catid.UserDefinedOIDToID(typ.Oid()) + if err != nil { + panic(err) + } + if typeDeps.Contains(int(typeID)) { + n.Child(typ.Name()) + } + } } // ScanIsReverseFn is a callback that is used to figure out if a scan needs to diff --git a/pkg/sql/opt/optbuilder/testdata/create_function b/pkg/sql/opt/optbuilder/testdata/create_function index ee37ac75476b..370ea14ad61f 100644 --- a/pkg/sql/opt/optbuilder/testdata/create_function +++ b/pkg/sql/opt/optbuilder/testdata/create_function @@ -1,4 +1,3 @@ -# This table has ID 53. exec-ddl CREATE TABLE ab (a INT PRIMARY KEY, b INT, INDEX idx(b)) ---- @@ -11,6 +10,10 @@ exec-ddl CREATE TYPE workday AS ENUM ('MON', 'TUE') ---- +exec-ddl +CREATE TABLE workdays (w workday) +---- + build CREATE FUNCTION f() RETURNS INT LANGUAGE SQL AS $$ SELECT 1 $$ ---- @@ -29,7 +32,42 @@ create-function │ RETURNS INT8 │ LANGUAGE SQL │ AS $$SELECT 1;$$ - └── no dependencies + └── dependencies + └── workday + +build +CREATE FUNCTION f() RETURNS workday LANGUAGE SQL AS $$SELECT w FROM workdays$$ +---- +create-function + ├── CREATE FUNCTION f() + │ RETURNS workday + │ LANGUAGE SQL + │ AS $$SELECT w FROM t.public.workdays;$$ + └── dependencies + ├── workdays [columns: w] + └── workday + +build +CREATE FUNCTION f() RETURNS STRING LANGUAGE SQL AS $$ SELECT 'MON'::workday::STRING $$ +---- +create-function + ├── CREATE FUNCTION f() + │ RETURNS STRING + │ LANGUAGE SQL + │ AS $$SELECT 'MON'::STRING;$$ + └── dependencies + └── workday + +build +CREATE FUNCTION f() RETURNS STRING LANGUAGE SQL AS $$SELECT w::STRING FROM workdays$$ +---- +create-function + ├── CREATE FUNCTION f() + │ RETURNS STRING + │ LANGUAGE SQL + │ AS $$SELECT w::STRING FROM t.public.workdays;$$ + └── dependencies + └── workdays [columns: w] build CREATE FUNCTION f(a INT) RETURNS INT LANGUAGE SQL AS $$ SELECT a FROM ab $$ diff --git a/pkg/sql/opt/optbuilder/testdata/create_view b/pkg/sql/opt/optbuilder/testdata/create_view index dc42c1409f4c..981e0bbce715 100644 --- a/pkg/sql/opt/optbuilder/testdata/create_view +++ b/pkg/sql/opt/optbuilder/testdata/create_view @@ -11,6 +11,14 @@ exec-ddl CREATE SEQUENCE s ---- +exec-ddl +CREATE TYPE foobar AS ENUM ('foo', 'bar') +---- + +exec-ddl +CREATE TABLE foobars (fb foobar) +---- + build CREATE VIEW v1 AS VALUES (1) ---- @@ -352,3 +360,21 @@ create-view t.public.v24 ├── columns: setval:1 └── dependencies └── s + +build +CREATE VIEW v25 AS VALUES ('foo'::foobar) +---- +create-view t.public.v25 + ├── VALUES ('foo'::foobar) + ├── columns: column1:1 + └── dependencies + └── foobar + +build +CREATE VIEW v26 AS SELECT fb FROM foobars +---- +create-view t.public.v26 + ├── SELECT fb FROM t.public.foobars + ├── columns: fb:1 + └── dependencies + └── foobars [columns: fb] From c8c14a65c14b204d0a5fb6f9c3761942114b76d0 Mon Sep 17 00:00:00 2001 From: e-mbrown Date: Tue, 9 Aug 2022 14:18:54 -0400 Subject: [PATCH 2/4] sql: Support DISCARD SEQUENCES Release note (sql change): We now support DISCARD SEQUENCES, which discards all sequence-related state such as currval/lastval information. DISCARD ALL now also discards sequence-related state. Release justification: Bug Fix --- docs/generated/sql/bnf/discard_stmt.bnf | 1 + docs/generated/sql/bnf/stmt_block.bnf | 1 + pkg/sql/discard.go | 12 ++++ pkg/sql/logictest/testdata/logic_test/discard | 60 +++++++++++++++++++ pkg/sql/parser/parse_test.go | 1 - pkg/sql/parser/sql.y | 5 +- pkg/sql/sem/tree/discard.go | 5 ++ 7 files changed, 83 insertions(+), 2 deletions(-) diff --git a/docs/generated/sql/bnf/discard_stmt.bnf b/docs/generated/sql/bnf/discard_stmt.bnf index 73f00c6615c1..9f84f9881f71 100644 --- a/docs/generated/sql/bnf/discard_stmt.bnf +++ b/docs/generated/sql/bnf/discard_stmt.bnf @@ -1,2 +1,3 @@ discard_stmt ::= 'DISCARD' 'ALL' + | 'DISCARD' 'SEQUENCES' diff --git a/docs/generated/sql/bnf/stmt_block.bnf b/docs/generated/sql/bnf/stmt_block.bnf index 8e896b6c0df3..9ee2f35bf423 100644 --- a/docs/generated/sql/bnf/stmt_block.bnf +++ b/docs/generated/sql/bnf/stmt_block.bnf @@ -83,6 +83,7 @@ deallocate_stmt ::= discard_stmt ::= 'DISCARD' 'ALL' + | 'DISCARD' 'SEQUENCES' grant_stmt ::= 'GRANT' privileges 'ON' grant_targets 'TO' role_spec_list opt_with_grant_option diff --git a/pkg/sql/discard.go b/pkg/sql/discard.go index b1ddf6504330..c4abcef719ad 100644 --- a/pkg/sql/discard.go +++ b/pkg/sql/discard.go @@ -16,6 +16,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/errors" ) @@ -52,6 +53,17 @@ func (n *discardNode) startExec(params runParams) error { // DEALLOCATE ALL params.p.preparedStatements.DeleteAll(params.ctx) + + // DISCARD SEQUENCES + params.p.sessionDataMutatorIterator.applyOnEachMutator(func(m sessionDataMutator) { + m.data.SequenceState = sessiondata.NewSequenceState() + m.initSequenceCache() + }) + case tree.DiscardModeSequences: + params.p.sessionDataMutatorIterator.applyOnEachMutator(func(m sessionDataMutator) { + m.data.SequenceState = sessiondata.NewSequenceState() + m.initSequenceCache() + }) default: return errors.AssertionFailedf("unknown mode for DISCARD: %d", n.mode) } diff --git a/pkg/sql/logictest/testdata/logic_test/discard b/pkg/sql/logictest/testdata/logic_test/discard index 7ce33dd80e40..8160f4eadb92 100644 --- a/pkg/sql/logictest/testdata/logic_test/discard +++ b/pkg/sql/logictest/testdata/logic_test/discard @@ -54,3 +54,63 @@ BEGIN statement error DISCARD ALL cannot run inside a transaction block DISCARD ALL + +statement ok +ROLLBACK + +statement ok +CREATE SEQUENCE discard_seq_test START WITH 10 + +query I +SELECT nextval('discard_seq_test') +---- +10 + +query I +SELECT lastval() +---- +10 + +query I +SELECT currval('discard_seq_test') +---- +10 + +statement ok +DISCARD SEQUENCES + +statement error pgcode 55000 pq: lastval\(\): lastval is not yet defined in this session +SELECT lastval() + +statement error pgcode 55000 pq: currval\(\): currval of sequence "test.public.discard_seq_test" is not yet defined in this session +SELECT currval('discard_seq_test') + +statement ok +CREATE SEQUENCE discard_seq_test_2 START WITH 10 + +query I +SELECT nextval('discard_seq_test_2') +---- +10 + +statement ok +DISCARD ALL + +statement error pgcode 55000 pq: lastval\(\): lastval is not yet defined in this session +SELECT lastval() + +statement ok +CREATE SEQUENCE S2 CACHE 10 + +query I +SELECT nextval('s2') +---- +1 + +statement ok +DISCARD SEQUENCES + +query I +SELECT nextval('s2') +---- +11 diff --git a/pkg/sql/parser/parse_test.go b/pkg/sql/parser/parse_test.go index 199df1908bdd..855e4b116623 100644 --- a/pkg/sql/parser/parse_test.go +++ b/pkg/sql/parser/parse_test.go @@ -450,7 +450,6 @@ func TestUnimplementedSyntax(t *testing.T) { {`DROP TRIGGER a`, 28296, `drop`, ``}, {`DISCARD PLANS`, 0, `discard plans`, ``}, - {`DISCARD SEQUENCES`, 0, `discard sequences`, ``}, {`DISCARD TEMP`, 0, `discard temp`, ``}, {`DISCARD TEMPORARY`, 0, `discard temp`, ``}, diff --git a/pkg/sql/parser/sql.y b/pkg/sql/parser/sql.y index 65e097002a80..0ffb76464e29 100644 --- a/pkg/sql/parser/sql.y +++ b/pkg/sql/parser/sql.y @@ -4805,7 +4805,10 @@ discard_stmt: $$.val = &tree.Discard{Mode: tree.DiscardModeAll} } | DISCARD PLANS { return unimplemented(sqllex, "discard plans") } -| DISCARD SEQUENCES { return unimplemented(sqllex, "discard sequences") } +| DISCARD SEQUENCES + { + $$.val = &tree.Discard{Mode: tree.DiscardModeSequences} + } | DISCARD TEMP { return unimplemented(sqllex, "discard temp") } | DISCARD TEMPORARY { return unimplemented(sqllex, "discard temp") } | DISCARD error // SHOW HELP: DISCARD diff --git a/pkg/sql/sem/tree/discard.go b/pkg/sql/sem/tree/discard.go index 857693c19ffd..e7407f5caddc 100644 --- a/pkg/sql/sem/tree/discard.go +++ b/pkg/sql/sem/tree/discard.go @@ -23,6 +23,9 @@ type DiscardMode int const ( // DiscardModeAll represents a DISCARD ALL statement. DiscardModeAll DiscardMode = iota + + // DiscardModeSequences represents a DISCARD SEQUENCES statement + DiscardModeSequences ) // Format implements the NodeFormatter interface. @@ -30,6 +33,8 @@ func (node *Discard) Format(ctx *FmtCtx) { switch node.Mode { case DiscardModeAll: ctx.WriteString("DISCARD ALL") + case DiscardModeSequences: + ctx.WriteString("DISCARD SEQUENCES") } } From 0402f4764e1fb91283095513a64fae761645d7c1 Mon Sep 17 00:00:00 2001 From: Arul Ajmani Date: Tue, 16 Aug 2022 14:13:59 -0400 Subject: [PATCH 3/4] roachpb: cleanup speculative leases returned by NotLeaseHolderErrors Speculative leases are returned as part of NLHEs if the committed lease is not known, but there is a guess as to who the leaseholder may be. Previoulsy, there were two ways to return these -- either by populating just the LeaseHolder field on the NLHE or by populating the Lease field with an unset sequence number. This patch unifies this behavior, and going forward, we expect the latter to be the canonical form to represent speculative leases. To that effect, the LeaseHolder field in the NLHE proto is prefixed as "deprecated". We should be able to remove the thing entirely in v23.1. Release note: None Release justification: low risk change. --- pkg/kv/kvclient/kvcoord/dist_sender.go | 8 +++-- pkg/kv/kvclient/kvcoord/dist_sender_test.go | 37 ++++++++++++--------- pkg/kv/kvserver/client_raft_test.go | 5 +-- pkg/kv/kvserver/client_replica_test.go | 8 ++--- pkg/kv/kvserver/replica_gossip.go | 4 +-- pkg/kv/kvserver/replica_proposal_buf.go | 12 ++++--- pkg/kv/kvserver/replica_range_lease.go | 18 +++++++++- pkg/kv/kvserver/replica_test.go | 2 +- pkg/kv/kvserver/testing_knobs.go | 15 ++++----- pkg/roachpb/errors.go | 4 +-- pkg/roachpb/errors.proto | 21 +++++++----- 11 files changed, 83 insertions(+), 51 deletions(-) diff --git a/pkg/kv/kvclient/kvcoord/dist_sender.go b/pkg/kv/kvclient/kvcoord/dist_sender.go index fbd80fc812a8..1e1f220af59f 100644 --- a/pkg/kv/kvclient/kvcoord/dist_sender.go +++ b/pkg/kv/kvclient/kvcoord/dist_sender.go @@ -2188,7 +2188,7 @@ func (ds *DistSender) sendToReplicas( ds.metrics.NotLeaseHolderErrCount.Inc(1) // If we got some lease information, we use it. If not, we loop around // and try the next replica. - if tErr.Lease != nil || tErr.LeaseHolder != nil { + if tErr.Lease != nil || tErr.DeprecatedLeaseHolder != nil { // Update the leaseholder in the range cache. Naively this would also // happen when the next RPC comes back, but we don't want to wait out // the additional RPC latency. @@ -2196,8 +2196,10 @@ func (ds *DistSender) sendToReplicas( var updatedLeaseholder bool if tErr.Lease != nil { updatedLeaseholder = routing.SyncTokenAndMaybeUpdateCache(ctx, tErr.Lease, &tErr.RangeDesc) - } else if tErr.LeaseHolder != nil { - updatedLeaseholder = routing.SyncTokenAndMaybeUpdateCacheWithSpeculativeLease(ctx, *tErr.LeaseHolder, &tErr.RangeDesc) + } else if tErr.DeprecatedLeaseHolder != nil { + updatedLeaseholder = routing.SyncTokenAndMaybeUpdateCacheWithSpeculativeLease( + ctx, *tErr.DeprecatedLeaseHolder, &tErr.RangeDesc, + ) } // Move the new leaseholder to the head of the queue for the next // retry. Note that the leaseholder might not be the one indicated by diff --git a/pkg/kv/kvclient/kvcoord/dist_sender_test.go b/pkg/kv/kvclient/kvcoord/dist_sender_test.go index 089f9bc9111c..f8795ba63cce 100644 --- a/pkg/kv/kvclient/kvcoord/dist_sender_test.go +++ b/pkg/kv/kvclient/kvcoord/dist_sender_test.go @@ -601,11 +601,13 @@ func TestRetryOnNotLeaseHolderError(t *testing.T) { expLease: true, }, { + // TODO(arul): This is only possible in 22.{1,2} mixed version clusters; + // remove once we get rid of the LeaseHolder field in 23.1. name: "leaseholder in desc, no lease", nlhe: roachpb.NotLeaseHolderError{ - RangeID: testUserRangeDescriptor3Replicas.RangeID, - LeaseHolder: &recognizedLeaseHolder, - RangeDesc: testUserRangeDescriptor3Replicas, + RangeID: testUserRangeDescriptor3Replicas.RangeID, + DeprecatedLeaseHolder: &recognizedLeaseHolder, + RangeDesc: testUserRangeDescriptor3Replicas, }, expLeaseholder: &recognizedLeaseHolder, expLease: false, @@ -757,9 +759,8 @@ func TestBackoffOnNotLeaseHolderErrorDuringTransfer(t *testing.T) { } reply.Error = roachpb.NewError( &roachpb.NotLeaseHolderError{ - Replica: repls[int(seq)%2], - LeaseHolder: &repls[(int(seq)+1)%2], - Lease: lease, + Replica: repls[int(seq)%2], + Lease: lease, }) return reply, nil } @@ -840,9 +841,8 @@ func TestNoBackoffOnNotLeaseHolderErrorFromFollowerRead(t *testing.T) { br := ba.CreateReply() if ba.Replica != lease.Replica { br.Error = roachpb.NewError(&roachpb.NotLeaseHolderError{ - Replica: ba.Replica, - LeaseHolder: &lease.Replica, - Lease: &lease, + Replica: ba.Replica, + Lease: &lease, }) } return br, nil @@ -1123,8 +1123,11 @@ func TestDistSenderIgnoresNLHEBasedOnOldRangeGeneration(t *testing.T) { Replica: roachpb.ReplicaDescriptor{NodeID: 4, StoreID: 4, ReplicaID: 4}, } } else { - // Speculative lease -- the NLHE only carries LeaseHolder information. - nlhe.LeaseHolder = &roachpb.ReplicaDescriptor{NodeID: 4, StoreID: 4, ReplicaID: 4} + // Speculative lease -- the NLHE only carries LeaseHolder information + // and the sequence number is unset. + nlhe.Lease = &roachpb.Lease{ + Replica: roachpb.ReplicaDescriptor{NodeID: 4, StoreID: 4, ReplicaID: 4}, + } } cachedLease := roachpb.Lease{ @@ -1703,7 +1706,10 @@ func TestEvictCacheOnUnknownLeaseHolder(t *testing.T) { var err error switch count { case 0, 1: - err = &roachpb.NotLeaseHolderError{LeaseHolder: &roachpb.ReplicaDescriptor{NodeID: 99, StoreID: 999}} + err = &roachpb.NotLeaseHolderError{ + Lease: &roachpb.Lease{ + Replica: roachpb.ReplicaDescriptor{NodeID: 99, StoreID: 999}}, + } case 2: err = roachpb.NewRangeNotFoundError(0, 0) default: @@ -5173,8 +5179,9 @@ func TestDistSenderNLHEFromUninitializedReplicaDoesNotCauseUnboundedBackoff(t *t // will include a speculative lease that points to a replica that isn't // part of the client's range descriptor. This is only possible in // versions <= 22.1 as NLHE errors from uninitialized replicas no longer - // return speculative leases. Effectively, this acts as a mixed - // (22.1, 22.2) version test. + // return speculative leases by populating the (Deprecated)LeaseHolder + // field. Effectively, this acts as a mixed (22.1, 22.2) version test. + // TODO(arul): remove the speculative lease version of this test in 23.1. clock := hlc.NewClockWithSystemTimeSource(time.Nanosecond /* maxOffset */) rpcContext := rpc.NewInsecureTestingContext(ctx, clock, stopper) @@ -5217,7 +5224,7 @@ func TestDistSenderNLHEFromUninitializedReplicaDoesNotCauseUnboundedBackoff(t *t RangeDesc: roachpb.RangeDescriptor{}, } if returnSpeculativeLease { - nlhe.LeaseHolder = &roachpb.ReplicaDescriptor{NodeID: 5, StoreID: 5, ReplicaID: 5} + nlhe.DeprecatedLeaseHolder = &roachpb.ReplicaDescriptor{NodeID: 5, StoreID: 5, ReplicaID: 5} } br.Error = roachpb.NewError(nlhe) case 1: diff --git a/pkg/kv/kvserver/client_raft_test.go b/pkg/kv/kvserver/client_raft_test.go index 431115c9a183..4a77ab618cdf 100644 --- a/pkg/kv/kvserver/client_raft_test.go +++ b/pkg/kv/kvserver/client_raft_test.go @@ -1282,8 +1282,9 @@ func TestRequestsOnLaggingReplica(t *testing.T) { require.NotNil(t, pErr, "unexpected success") nlhe := pErr.GetDetail().(*roachpb.NotLeaseHolderError) require.NotNil(t, nlhe, "expected NotLeaseholderError, got: %s", pErr) - require.NotNil(t, nlhe.LeaseHolder, "expected NotLeaseholderError with a known leaseholder, got: %s", pErr) - require.Equal(t, leaderReplicaID, nlhe.LeaseHolder.ReplicaID) + require.False(t, nlhe.Lease.Empty()) + require.NotNil(t, nlhe.Lease.Replica, "expected NotLeaseholderError with a known leaseholder, got: %s", pErr) + require.Equal(t, leaderReplicaID, nlhe.Lease.Replica.ReplicaID) } type fakeSnapshotStream struct { diff --git a/pkg/kv/kvserver/client_replica_test.go b/pkg/kv/kvserver/client_replica_test.go index e17bd407638f..49bb8d260482 100644 --- a/pkg/kv/kvserver/client_replica_test.go +++ b/pkg/kv/kvserver/client_replica_test.go @@ -1617,9 +1617,9 @@ func TestLeaseExpirationBasedRangeTransfer(t *testing.T) { if !ok { t.Fatalf("expected %T, got %s", &roachpb.NotLeaseHolderError{}, pErr) } - if !nlhe.LeaseHolder.Equal(&l.replica1Desc) { + if !nlhe.Lease.Replica.Equal(&l.replica1Desc) { t.Fatalf("expected lease holder %+v, got %+v", - l.replica1Desc, nlhe.LeaseHolder) + l.replica1Desc, nlhe.Lease.Replica) } // Check that replica1 now has the lease. @@ -1717,9 +1717,9 @@ func TestLeaseExpirationBasedDrainTransfer(t *testing.T) { if !ok { t.Fatalf("expected %T, got %s", &roachpb.NotLeaseHolderError{}, pErr) } - if nlhe.LeaseHolder == nil || !nlhe.LeaseHolder.Equal(&l.replica1Desc) { + if nlhe.Lease.Empty() || !nlhe.Lease.Replica.Equal(&l.replica1Desc) { t.Fatalf("expected lease holder %+v, got %+v", - l.replica1Desc, nlhe.LeaseHolder) + l.replica1Desc, nlhe.Lease.Replica) } // Check that replica1 now has the lease. diff --git a/pkg/kv/kvserver/replica_gossip.go b/pkg/kv/kvserver/replica_gossip.go index 96c40a425b30..b42af3cde957 100644 --- a/pkg/kv/kvserver/replica_gossip.go +++ b/pkg/kv/kvserver/replica_gossip.go @@ -144,8 +144,8 @@ func (r *Replica) getLeaseForGossip(ctx context.Context) (bool, *roachpb.Error) switch e := pErr.GetDetail().(type) { case *roachpb.NotLeaseHolderError: // NotLeaseHolderError means there is an active lease, but only if - // the lease holder is set; otherwise, it's likely a timeout. - if e.LeaseHolder != nil { + // the lease is non-empty; otherwise, it's likely a timeout. + if !e.Lease.Empty() { pErr = nil } default: diff --git a/pkg/kv/kvserver/replica_proposal_buf.go b/pkg/kv/kvserver/replica_proposal_buf.go index b68a30f2a287..fa42407e13e8 100644 --- a/pkg/kv/kvserver/replica_proposal_buf.go +++ b/pkg/kv/kvserver/replica_proposal_buf.go @@ -1235,12 +1235,14 @@ func (rp *replicaProposer) rejectProposalWithRedirectLocked( storeID := r.store.StoreID() r.store.metrics.LeaseRequestErrorCount.Inc(1) redirectRep, _ /* ok */ := rangeDesc.GetReplicaDescriptorByID(redirectTo) - speculativeLease := roachpb.Lease{ - Replica: redirectRep, - } log.VEventf(ctx, 2, "redirecting proposal to node %s; request: %s", redirectRep.NodeID, prop.Request) - rp.rejectProposalWithErrLocked(ctx, prop, roachpb.NewError(newNotLeaseHolderError( - speculativeLease, storeID, rangeDesc, "refusing to acquire lease on follower"))) + rp.rejectProposalWithErrLocked(ctx, prop, roachpb.NewError( + newNotLeaseHolderErrorWithSpeculativeLease( + redirectRep, + storeID, + rangeDesc, + "refusing to acquire lease on follower"), + )) } func (rp *replicaProposer) rejectProposalWithLeaseTransferRejectedLocked( diff --git a/pkg/kv/kvserver/replica_range_lease.go b/pkg/kv/kvserver/replica_range_lease.go index 6fef0e76b1b0..69cd6c256b94 100644 --- a/pkg/kv/kvserver/replica_range_lease.go +++ b/pkg/kv/kvserver/replica_range_lease.go @@ -1023,12 +1023,28 @@ func newNotLeaseHolderError( if stillMember { err.Lease = new(roachpb.Lease) *err.Lease = l - err.LeaseHolder = &err.Lease.Replica } } return err } +// newNotLeaseHolderErrorWithSpeculativeLease returns a NotLeaseHolderError +// initialized with a speculative lease pointing to the supplied replica. +// A NotLeaseHolderError may be constructed with a speculative lease if the +// current lease is not known, but the error is being created by guessing who +// the leaseholder may be. +func newNotLeaseHolderErrorWithSpeculativeLease( + leaseHolder roachpb.ReplicaDescriptor, + proposerStoreID roachpb.StoreID, + rangeDesc *roachpb.RangeDescriptor, + msg string, +) *roachpb.NotLeaseHolderError { + speculativeLease := roachpb.Lease{ + Replica: leaseHolder, + } + return newNotLeaseHolderError(speculativeLease, proposerStoreID, rangeDesc, msg) +} + // newLeaseTransferRejectedBecauseTargetMayNeedSnapshotError return an error // indicating that a lease transfer failed because the current leaseholder could // not prove that the lease transfer target did not need a Raft snapshot. diff --git a/pkg/kv/kvserver/replica_test.go b/pkg/kv/kvserver/replica_test.go index 574db8d309fb..0b6cdfb47201 100644 --- a/pkg/kv/kvserver/replica_test.go +++ b/pkg/kv/kvserver/replica_test.go @@ -739,7 +739,7 @@ func TestBehaviorDuringLeaseTransfer(t *testing.T) { require.Error(t, err) var lErr *roachpb.NotLeaseHolderError require.True(t, errors.As(err, &lErr)) - require.Equal(t, secondReplica.StoreID, lErr.LeaseHolder.StoreID) + require.Equal(t, secondReplica.StoreID, lErr.Lease.Replica.StoreID) } else { // Check that the replica doesn't use its lease, even though there's // no longer a transfer in progress. This is because, even though diff --git a/pkg/kv/kvserver/testing_knobs.go b/pkg/kv/kvserver/testing_knobs.go index 2ab292632729..ce473fc30341 100644 --- a/pkg/kv/kvserver/testing_knobs.go +++ b/pkg/kv/kvserver/testing_knobs.go @@ -491,14 +491,13 @@ func (p *PinnedLeasesKnob) rejectLeaseIfPinnedElsewhere(r *Replica) *roachpb.Err if err != nil { return roachpb.NewError(err) } - var pinned *roachpb.ReplicaDescriptor - if pinnedRep, ok := r.descRLocked().GetReplicaDescriptor(pinnedStore); ok { - pinned = &pinnedRep - } + pinned, _ := r.descRLocked().GetReplicaDescriptor(pinnedStore) return roachpb.NewError(&roachpb.NotLeaseHolderError{ - Replica: repDesc, - LeaseHolder: pinned, - RangeID: r.RangeID, - CustomMsg: "injected: lease pinned to another store", + Replica: repDesc, + Lease: &roachpb.Lease{ + Replica: pinned, + }, + RangeID: r.RangeID, + CustomMsg: "injected: lease pinned to another store", }) } diff --git a/pkg/roachpb/errors.go b/pkg/roachpb/errors.go index 640e91221fd4..544b3a0a1bb4 100644 --- a/pkg/roachpb/errors.go +++ b/pkg/roachpb/errors.go @@ -488,12 +488,12 @@ func (e *NotLeaseHolderError) message(_ *Error) string { } else { fmt.Fprint(&buf, "replica not lease holder; ") } - if e.LeaseHolder == nil { + if e.DeprecatedLeaseHolder == nil { fmt.Fprint(&buf, "lease holder unknown") } else if e.Lease != nil { fmt.Fprintf(&buf, "current lease is %s", e.Lease) } else { - fmt.Fprintf(&buf, "replica %s is", *e.LeaseHolder) + fmt.Fprintf(&buf, "replica %s is", *e.DeprecatedLeaseHolder) } return buf.String() } diff --git a/pkg/roachpb/errors.proto b/pkg/roachpb/errors.proto index 617c1b7ea4b2..55f9fee61eb2 100644 --- a/pkg/roachpb/errors.proto +++ b/pkg/roachpb/errors.proto @@ -45,10 +45,15 @@ message NotLeaseHolderError { // representation, if known. optional roachpb.ReplicaDescriptor replica = 1 [(gogoproto.nullable) = false]; // The lease holder, if known. - optional roachpb.ReplicaDescriptor lease_holder = 2; - // The current lease, if known. This might be nil even when lease_holder is - // set, as sometimes one can create this error without actually knowing the - // current lease, but having a guess about who the leader is. + // + // This field was only ever meaningful if the full lease was not known, but + // when constructing this error there was a guess about who the leaseholder + // may be. The same idea applied to speculative leases (which have unset + // sequence numbers). In a bid to unify these two cases, from v22.2, we stop + // making use of this field. + // TODO(arul): remove this field in 23.1. + optional roachpb.ReplicaDescriptor deprecated_lease_holder = 2; + // The current lease, if known. // // It's possible for leases returned here to represent speculative leases, not // actual committed leases. In this case, the lease will not have its Sequence @@ -196,7 +201,7 @@ enum TransactionAbortedReason { // TODO(andrei): We should be able to identify the range merge case by saving // a bit more info in the timestamp cache. ABORT_REASON_TIMESTAMP_CACHE_REJECTED = 7; - + reserved 2; } @@ -720,15 +725,15 @@ message InsufficientSpaceError { optional int64 store_id = 1 [(gogoproto.nullable) = false, (gogoproto.customname) = "StoreID", (gogoproto.casttype) = "StoreID"]; - // Op is the operaton that was unable to be performed. + // Op is the operaton that was unable to be performed. optional string op = 2 [(gogoproto.nullable) = false]; // Available is remaining capacity. optional int64 available = 3 [(gogoproto.nullable) = false]; - // Capacity is total capacity. + // Capacity is total capacity. optional int64 capacity = 4 [(gogoproto.nullable) = false]; // RequiredFraction is the required remaining capacity fraction. optional double required = 5 [(gogoproto.nullable) = false]; -} \ No newline at end of file +} From 43e787bf9ccded23c465847e145a7bb482a5ef7e Mon Sep 17 00:00:00 2001 From: Arul Ajmani Date: Tue, 16 Aug 2022 15:32:16 -0400 Subject: [PATCH 4/4] kvserver: misc cleanup Fix a comment that I broke in a previous patch; remove a TODO that's since been addressed. Release justification: non production code change. Release note: None --- pkg/kv/kvserver/store_send.go | 14 +++++++------- pkg/roachpb/errors.proto | 3 --- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/pkg/kv/kvserver/store_send.go b/pkg/kv/kvserver/store_send.go index c823e23b55b9..cf2048740f16 100644 --- a/pkg/kv/kvserver/store_send.go +++ b/pkg/kv/kvserver/store_send.go @@ -182,13 +182,13 @@ func (s *Store) SendWithWriteBytes( return nil, nil, roachpb.NewError(err) } if !repl.IsInitialized() { - // If we have an uninitialized copy of the range, then we are - // probably a valid member of the range, we're just in the - // RangeNotFoundError, the client would invalidate its cache. Instead, - // we return a NLHE error to indicate to the client that it should move - // on and try the next replica. Very likely, the next replica the client - // tries will be initialized and will have useful leaseholder information - // for the client. + // If we have an uninitialized copy of the range, then we are probably a + // valid member of the range, we're just in the process of getting our + // snapshot. If we returned RangeNotFoundError, the client would + // invalidate its cache. Instead, we return a NLHE error to indicate to + // the client that it should move on and try the next replica. Very + // likely, the next replica the client tries will be initialized and will + // have useful leaseholder information for the client. return nil, nil, roachpb.NewError(&roachpb.NotLeaseHolderError{ RangeID: ba.RangeID, // The replica doesn't have a range descriptor yet, so we have to build diff --git a/pkg/roachpb/errors.proto b/pkg/roachpb/errors.proto index 55f9fee61eb2..d22ba4cacff0 100644 --- a/pkg/roachpb/errors.proto +++ b/pkg/roachpb/errors.proto @@ -65,9 +65,6 @@ message NotLeaseHolderError { // The generation of the descriptor is used by the DistSender's RangeCache to // determine whether the error was returned because the replica had a stale // understanding of who the leaseholder is. - // TOOD(arul): In the future we may want to update the RangeCache if the - // returned range descriptor is newer than what is in the RangeCache. This - // would help us avoid a cache eviction/descriptor lookup. optional roachpb.RangeDescriptor range_desc = 6 [(gogoproto.nullable)=false]; // If set, the Error() method will return this instead of composing its // regular spiel. Useful because we reuse this error when rejecting a command