Merge #85703 #86230 #86247

85703: opt: add more tests for user-defined type dependency tracking r=mgartner a=mgartner

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

Release justification: This is a test-only change.


86230: sql: Support DISCARD SEQUENCES r=rafiss a=e-mbrown

Refs #83061

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: Low risk, high benefit changes to existing functionality(DISCARD ALL)

86247: roachpb: cleanup speculative leases returned by NotLeaseHolderErrors r=nvanbenschoten a=arulajmani

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.
Previously, 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.

Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: e-mbrown <[email protected]>
Co-authored-by: Arul Ajmani <[email protected]>

docs/generated/sql/bnf/discard_stmt.bnf

@@ -1,2 +1,3 @@
 discard_stmt ::=
 	'DISCARD' 'ALL'
+	| 'DISCARD' 'SEQUENCES'

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

pkg/kv/kvclient/kvcoord/dist_sender.go

@@ -2188,16 +2188,18 @@ 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.
 
 					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

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:

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 {

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.

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:

pkg/kv/kvserver/replica_proposal_buf.go

@@ -1232,12 +1232,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(

pkg/kv/kvserver/replica_range_lease.go

@@ -1038,12 +1038,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.

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

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

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",
 	})
 }

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()
 }

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
@@ -60,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
@@ -196,7 +198,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 +722,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];
-}
+}

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)
 	}