Merge #72653 #72825 #72871 #72887 #72894 #72895 #72902 #72907

72653: sql: implement ConstructZigzagJoin in distsql_spec_exec_factory r=cucaroach a=cucaroach

Informs #47473

Release note: None


72825: sql: handle no cluster_inflight_traces table on tenants r=adityamaru a=aliher1911

Previously query for traces under tenant will fail with NPE.
This was happening because trace collector is not available
and virtual table was not aware of the case.
This patch adds handling of the table on tenant sql pods.

Release note: None

Fixes #72564

72871: backupccl: don't include tenants in non-cluster backups r=dt a=dt

Release note (bug fix): System tenant backups of individual tables and databases no longer include tenants as well.

72887: roachprod: remove ClusterImpl interface r=RaduBerinde a=RaduBerinde

The ClusterImpl interface was useful when roachprod had some (very
limited) support for Cassandra, but that has been removed. We're left
with an unnecessary indirection that lacks documentation. The code
doesn't follow any layering, with `Cockroach` working directly with
`SyncedCluster` internals and plenty of cockroach-specific code in
`SyncedCluster`.

This commit removes this interface and improves the documentation of
the methods that are simplified.

Release note: None

72894: kv: use version-less key in rditer.KeyRange r=nvanbenschoten a=nvanbenschoten

This commit replaces the use of `storage.MVCCKey` bounds with `roachpb.Key`
bounds in `rditer.KeyRange`. It addresses an existing TODO and helps clarify
things in #72121 slightly, as that PR is going to add a new field to `MVCCKey`.

72895: storage: remove decodeMVCCKey r=nvanbenschoten a=nvanbenschoten

The function was identical to `DecodeMVCCKey`, which is defined a few lines up in the file.

72902: storage: delete deprecated PutProto function r=nvanbenschoten a=nvanbenschoten

This function has been deprecated since 5e5eaa5. It was only used in one test, so this commit removes it.

72907: build/builder: install awscli v2  r=rail a=erikgrinaker

`roachprod` requires awscli v2, but the `builder` image included v1.

Release note: None

Co-authored-by: Tommy Reilly <treilly@cockroachlabs.com>
Co-authored-by: Oleg Afanasyev <oleg@cockroachlabs.com>
Co-authored-by: David Taylor <tinystatemachine@gmail.com>
Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Erik Grinaker <grinaker@cockroachlabs.com>

build/builder.sh

@@ -3,7 +3,7 @@
 set -euo pipefail
 
 image=cockroachdb/builder
-version=20211117-113545
+version=20211118-190337
 
 function init() {
   docker build --tag="${image}" "$(dirname "${0}")/builder"

build/builder/Dockerfile

@@ -167,10 +167,11 @@ RUN curl -fsSL https://deb.nodesource.com/gpgkey/nodesource.gpg.key | apt-key ad
 
 # awscli - roachtests
 # NB: we don't use apt-get because we need an up to date version of awscli
-RUN curl -fsSL "https://s3.amazonaws.com/aws-cli/awscli-bundle.zip" -o "awscli-bundle.zip" && \
-  unzip awscli-bundle.zip && \
-  ./awscli-bundle/install -i /usr/local/aws -b /usr/local/bin/aws && \
-  rm -rf awscli-bundle.zip awscli-bundle
+RUN curl -fsSL "https://awscli.amazonaws.com/awscli-exe-linux-x86_64-2.0.30.zip" -o "awscliv2.zip" && \
+ echo "7ee475f22c1b35cc9e53affbf96a9ffce91706e154a9441d0d39cbf8366b718e  awscliv2.zip" | sha256sum -c - && \
+ unzip awscliv2.zip && \
+ ./aws/install && \
+ rm -rf aws awscliv2.zip
 
 # git - Upgrade to a more modern version
 RUN DEBIAN_FRONTEND=noninteractive apt-get install dh-autoreconf libcurl4-gnutls-dev libexpat1-dev gettext libz-dev libssl-dev -y && \

pkg/ccl/backupccl/backup_planning.go

@@ -839,7 +839,7 @@ func backupPlanHook(
 			}
 			spans = append(spans, tableSpans...)
 
-			if p.ExecCfg().Codec.ForSystemTenant() {
+			if p.ExecCfg().Codec.ForSystemTenant() && backupStmt.Coverage() == tree.AllDescriptors {
 				// Include all tenants.
 				tenants, err = retrieveAllTenantsMetadata(
 					ctx, p.ExecCfg().InternalExecutor, p.ExtendedEvalContext().Txn,

pkg/ccl/backupccl/backup_test.go

@@ -7011,6 +7011,9 @@ func TestBackupRestoreTenant(t *testing.T) {
 		return nil
 	})
 
+	systemDB.Exec(t, `BACKUP system.users TO 'nodelocal://1/users'`)
+	systemDB.CheckQueryResults(t, `SELECT manifest->>'tenants' FROM [SHOW BACKUP 'nodelocal://1/users' WITH as_json]`, [][]string{{"[]"}})
+
 	// Prevent a logging assertion that the server ID is initialized multiple times.
 	log.TestingClearServerIdentifiers()
 

pkg/ccl/logictestccl/testdata/logic_test/tenant_unsupported

@@ -65,3 +65,8 @@ ALTER TABLE kv EXPERIMENTAL_RELOCATE LEASE VALUES (1, 'k')
 
 statement error operation is unsupported in multi-tenancy mode
 SELECT crdb_internal.check_consistency(true, '', '')
+
+# Can not query inflight traces on sql pods
+
+statement error table crdb_internal.cluster_inflight_traces is not implemented on tenants
+SELECT * from crdb_internal.cluster_inflight_traces WHERE trace_id = 42

pkg/ccl/serverccl/BUILD.bazel

@@ -30,6 +30,7 @@ go_test(
         "//pkg/server",
         "//pkg/server/serverpb",
         "//pkg/sql",
+        "//pkg/sql/distsql",
         "//pkg/sql/pgwire/pgcode",
         "//pkg/sql/tests",
         "//pkg/testutils/serverutils",

pkg/ccl/serverccl/server_sql_test.go

@@ -22,9 +22,11 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/cockroach/pkg/sql"
+	"github.com/cockroachdb/cockroach/pkg/sql/distsql"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
 	"github.com/cockroachdb/cockroach/pkg/util/envutil"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -225,3 +227,24 @@ func TestTenantRowIDs(t *testing.T) {
 	}
 	require.Equal(t, numRows, rowCount)
 }
+
+// TestNoInflightTracesVirtualTableOnTenant verifies that internal inflight traces table
+// is correctly handled by tenants (which don't provide this functionality as of now).
+func TestNoInflightTracesVirtualTableOnTenant(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	args := base.TestClusterArgs{}
+	tc := testcluster.StartTestCluster(t, 2 /* nodes */, args)
+	defer tc.Stopper().Stop(ctx)
+
+	tenn, err := tc.Server(0).StartTenant(ctx, base.TestTenantArgs{TenantID: serverutils.TestTenantID()})
+	require.NoError(t, err, "Failed to start tenant node")
+	ex := tenn.DistSQLServer().(*distsql.ServerImpl).ServerConfig.Executor
+	_, err = ex.Exec(ctx, "get table", nil, /* txn */
+		"select * from crdb_internal.cluster_inflight_traces WHERE trace_id = 4;")
+	require.Error(t, err, "cluster_inflight_traces should be unsupported")
+	require.Contains(t, err.Error(), "table crdb_internal.cluster_inflight_traces is not implemented on tenants")
+}

pkg/cmd/roachprod/flags.go

@@ -43,18 +43,18 @@ var (
 		"COCKROACH_ENABLE_RPC_COMPRESSION=false",
 		"COCKROACH_UI_RELEASE_NOTES_SIGNUP_DISMISSED=true",
 	}
-	tag          string
-	external     = false
-	certsDir     string
-	adminurlOpen = false
-	adminurlPath = ""
-	adminurlIPs  = false
-	useTreeDist  = true
-	quiet        = false
-	sig          = 9
-	waitFlag     = false
-	createVMOpts = vm.DefaultCreateOpts()
-	startOpts    = install.StartOpts{
+	tag           string
+	external      = false
+	pgurlCertsDir string
+	adminurlOpen  = false
+	adminurlPath  = ""
+	adminurlIPs   = false
+	useTreeDist   = true
+	quiet         = false
+	sig           = 9
+	waitFlag      = false
+	createVMOpts  = vm.DefaultCreateOpts()
+	startOpts     = install.StartOpts{
 		Encrypt:    false,
 		Sequential: true,
 		SkipInit:   false,
@@ -152,7 +152,7 @@ func initFlags() {
 
 	pgurlCmd.Flags().BoolVar(&external,
 		"external", false, "return pgurls for external connections")
-	pgurlCmd.Flags().StringVar(&certsDir,
+	pgurlCmd.Flags().StringVar(&pgurlCertsDir,
 		"certs-dir", "./certs", "cert dir to use for secure connections")
 
 	pprofCmd.Flags().DurationVar(&pprofOptions.duration,

pkg/cmd/roachprod/main.go

@@ -85,7 +85,7 @@ func wrap(f func(cmd *cobra.Command, args []string) error) func(cmd *cobra.Comma
 func clusterOpts() install.ClusterSettings {
 	return install.ClusterSettings{
 		Tag:            tag,
-		CertsDir:       certsDir,
+		PGUrlCertsDir:  pgurlCertsDir,
 		Secure:         secure,
 		Quiet:          quiet || !term.IsTerminal(int(os.Stdout.Fd())),
 		UseTreeDist:    useTreeDist,

pkg/kv/kvserver/client_merge_test.go

@@ -3789,7 +3789,7 @@ func TestStoreRangeMergeRaftSnapshot(t *testing.T) {
 		for _, r := range keyRanges {
 			sstFile := &storage.MemFile{}
 			sst := storage.MakeIngestionSSTWriter(sstFile)
-			if err := sst.ClearRawRange(r.Start.Key, r.End.Key); err != nil {
+			if err := sst.ClearRawRange(r.Start, r.End); err != nil {
 				return err
 			}
 
@@ -3800,7 +3800,7 @@ func TestStoreRangeMergeRaftSnapshot(t *testing.T) {
 				if err != nil {
 					return err
 				}
-				if !valid || r.End.Key.Compare(it.UnsafeKey().Key) <= 0 {
+				if !valid || r.End.Compare(it.UnsafeKey().Key) <= 0 {
 					if err := sst.Finish(); err != nil {
 						return err
 					}
@@ -3828,7 +3828,7 @@ func TestStoreRangeMergeRaftSnapshot(t *testing.T) {
 			sst := storage.MakeIngestionSSTWriter(sstFile)
 			defer sst.Close()
 			r := rditer.MakeRangeIDLocalKeyRange(rangeID, false /* replicatedOnly */)
-			if err := sst.ClearRawRange(r.Start.Key, r.End.Key); err != nil {
+			if err := sst.ClearRawRange(r.Start, r.End); err != nil {
 				return err
 			}
 			tombstoneKey := keys.RangeTombstoneKey(rangeID)
@@ -3852,7 +3852,7 @@ func TestStoreRangeMergeRaftSnapshot(t *testing.T) {
 			EndKey:   roachpb.RKey(keyEnd),
 		}
 		r := rditer.MakeUserKeyRange(&desc)
-		if err := storage.ClearRangeWithHeuristic(receivingEng, &sst, r.Start.Key, r.End.Key); err != nil {
+		if err := storage.ClearRangeWithHeuristic(receivingEng, &sst, r.Start, r.End); err != nil {
 			return err
 		}
 		err := sst.Finish()

pkg/kv/kvserver/rditer/replica_data_iter.go

@@ -18,10 +18,8 @@ import (
 
 // KeyRange is a helper struct for the ReplicaMVCCDataIterator and
 // ReplicaEngineDataIterator.
-// TODO(sumeer): change these to roachpb.Key since the timestamp is
-// always empty and the code below assumes that fact.
 type KeyRange struct {
-	Start, End storage.MVCCKey
+	Start, End roachpb.Key
 }
 
 // ReplicaMVCCDataIterator provides a complete iteration over MVCC or unversioned
@@ -143,8 +141,8 @@ func MakeRangeIDLocalKeyRange(rangeID roachpb.RangeID, replicatedOnly bool) KeyR
 	}
 	sysRangeIDKey := prefixFn(rangeID)
 	return KeyRange{
-		Start: storage.MakeMVCCMetadataKey(sysRangeIDKey),
-		End:   storage.MakeMVCCMetadataKey(sysRangeIDKey.PrefixEnd()),
+		Start: sysRangeIDKey,
+		End:   sysRangeIDKey.PrefixEnd(),
 	}
 }
 
@@ -154,8 +152,8 @@ func MakeRangeIDLocalKeyRange(rangeID roachpb.RangeID, replicatedOnly bool) KeyR
 // /System), but it actually belongs to [/Table/1, /Table/2).
 func makeRangeLocalKeyRange(d *roachpb.RangeDescriptor) KeyRange {
 	return KeyRange{
-		Start: storage.MakeMVCCMetadataKey(keys.MakeRangeKeyPrefix(d.StartKey)),
-		End:   storage.MakeMVCCMetadataKey(keys.MakeRangeKeyPrefix(d.EndKey)),
+		Start: keys.MakeRangeKeyPrefix(d.StartKey),
+		End:   keys.MakeRangeKeyPrefix(d.EndKey),
 	}
 }
 
@@ -177,12 +175,12 @@ func makeRangeLockTableKeyRanges(d *roachpb.RangeDescriptor) [2]KeyRange {
 	endGlobal, _ := keys.LockTableSingleKey(roachpb.Key(d.EndKey), nil)
 	return [2]KeyRange{
 		{
-			Start: storage.MakeMVCCMetadataKey(startRangeLocal),
-			End:   storage.MakeMVCCMetadataKey(endRangeLocal),
+			Start: startRangeLocal,
+			End:   endRangeLocal,
 		},
 		{
-			Start: storage.MakeMVCCMetadataKey(startGlobal),
-			End:   storage.MakeMVCCMetadataKey(endGlobal),
+			Start: startGlobal,
+			End:   endGlobal,
 		},
 	}
 }
@@ -191,8 +189,8 @@ func makeRangeLockTableKeyRanges(d *roachpb.RangeDescriptor) [2]KeyRange {
 func MakeUserKeyRange(d *roachpb.RangeDescriptor) KeyRange {
 	userKeys := d.KeySpan()
 	return KeyRange{
-		Start: storage.MakeMVCCMetadataKey(userKeys.Key.AsRawKey()),
-		End:   storage.MakeMVCCMetadataKey(userKeys.EndKey.AsRawKey()),
+		Start: userKeys.Key.AsRawKey(),
+		End:   userKeys.EndKey.AsRawKey(),
 	}
 }
 
@@ -240,13 +238,13 @@ func (ri *ReplicaMVCCDataIterator) tryCloseAndCreateIter() {
 		ri.it = ri.reader.NewMVCCIterator(
 			storage.MVCCKeyAndIntentsIterKind,
 			storage.IterOptions{
-				LowerBound: ri.ranges[ri.curIndex].Start.Key,
-				UpperBound: ri.ranges[ri.curIndex].End.Key,
+				LowerBound: ri.ranges[ri.curIndex].Start,
+				UpperBound: ri.ranges[ri.curIndex].End,
 			})
 		if ri.reverse {
-			ri.it.SeekLT(ri.ranges[ri.curIndex].End)
+			ri.it.SeekLT(storage.MakeMVCCMetadataKey(ri.ranges[ri.curIndex].End))
 		} else {
-			ri.it.SeekGE(ri.ranges[ri.curIndex].Start)
+			ri.it.SeekGE(storage.MakeMVCCMetadataKey(ri.ranges[ri.curIndex].Start))
 		}
 		if valid, err := ri.it.Valid(); valid || err != nil {
 			ri.err = err
@@ -356,7 +354,7 @@ func NewReplicaEngineDataIterator(
 // seekStart seeks the iterator to the start of its data range.
 func (ri *ReplicaEngineDataIterator) seekStart() {
 	ri.curIndex = 0
-	ri.valid, ri.err = ri.it.SeekEngineKeyGE(storage.EngineKey{Key: ri.ranges[ri.curIndex].Start.Key})
+	ri.valid, ri.err = ri.it.SeekEngineKeyGE(storage.EngineKey{Key: ri.ranges[ri.curIndex].Start})
 	ri.advance()
 }
 
@@ -383,13 +381,13 @@ func (ri *ReplicaEngineDataIterator) advance() {
 			ri.valid = false
 			return
 		}
-		if k.Key.Compare(ri.ranges[ri.curIndex].End.Key) < 0 {
+		if k.Key.Compare(ri.ranges[ri.curIndex].End) < 0 {
 			return
 		}
 		ri.curIndex++
 		if ri.curIndex < len(ri.ranges) {
 			ri.valid, ri.err = ri.it.SeekEngineKeyGE(
-				storage.EngineKey{Key: ri.ranges[ri.curIndex].Start.Key})
+				storage.EngineKey{Key: ri.ranges[ri.curIndex].Start})
 		} else {
 			ri.valid = false
 			return

pkg/kv/kvserver/rditer/replica_data_iter_test.go

@@ -314,7 +314,7 @@ func TestReplicaDataIterator(t *testing.T) {
 
 func checkOrdering(t *testing.T, ranges []KeyRange) {
 	for i := 1; i < len(ranges); i++ {
-		if ranges[i].Start.Less(ranges[i-1].End) {
+		if ranges[i].Start.Compare(ranges[i-1].End) < 0 {
 			t.Fatalf("ranges need to be ordered and non-overlapping, but %s > %s",
 				ranges[i-1].End, ranges[i].Start)
 		}

pkg/kv/kvserver/rditer/stats.go

@@ -27,11 +27,11 @@ func ComputeStatsForRange(
 	for _, keyRange := range MakeReplicatedKeyRangesExceptLockTable(d) {
 		func() {
 			iter := reader.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind,
-				storage.IterOptions{UpperBound: keyRange.End.Key})
+				storage.IterOptions{UpperBound: keyRange.End})
 			defer iter.Close()
 
 			var msDelta enginepb.MVCCStats
-			if msDelta, err = iter.ComputeStats(keyRange.Start.Key, keyRange.End.Key, nowNanos); err != nil {
+			if msDelta, err = iter.ComputeStats(keyRange.Start, keyRange.End, nowNanos); err != nil {
 				return
 			}
 			ms.Add(msDelta)

pkg/kv/kvserver/replica_consistency.go

@@ -632,9 +632,9 @@ func (r *Replica) sha512(
 		// using MVCCKeyAndIntentsIterKind and consider all locks here.
 		for _, span := range rditer.MakeReplicatedKeyRangesExceptLockTable(&desc) {
 			iter := snap.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind,
-				storage.IterOptions{UpperBound: span.End.Key})
+				storage.IterOptions{UpperBound: span.End})
 			spanMS, err := storage.ComputeStatsForRange(
-				iter, span.Start.Key, span.End.Key, 0 /* nowNanos */, visitor,
+				iter, span.Start, span.End, 0 /* nowNanos */, visitor,
 			)
 			iter.Close()
 			if err != nil {

pkg/kv/kvserver/replica_raftstorage.go

@@ -733,7 +733,7 @@ func clearRangeData(
 	}
 
 	for _, keyRange := range keyRanges {
-		if err := clearRangeFn(reader, writer, keyRange.Start.Key, keyRange.End.Key); err != nil {
+		if err := clearRangeFn(reader, writer, keyRange.Start, keyRange.End); err != nil {
 			return err
 		}
 	}
@@ -1067,10 +1067,10 @@ func (r *Replica) clearSubsumedReplicaDiskData(
 		// Compute the total key space covered by the current replica and all
 		// subsumed replicas.
 		for i := range srKeyRanges {
-			if srKeyRanges[i].Start.Key.Compare(totalKeyRanges[i].Start.Key) < 0 {
+			if srKeyRanges[i].Start.Compare(totalKeyRanges[i].Start) < 0 {
 				totalKeyRanges[i].Start = srKeyRanges[i].Start
 			}
-			if srKeyRanges[i].End.Key.Compare(totalKeyRanges[i].End.Key) > 0 {
+			if srKeyRanges[i].End.Compare(totalKeyRanges[i].End) > 0 {
 				totalKeyRanges[i].End = srKeyRanges[i].End
 			}
 		}
@@ -1090,15 +1090,15 @@ func (r *Replica) clearSubsumedReplicaDiskData(
 	// before it completes. It is reasonable for a snapshot for r1 from S3 to
 	// subsume both r1 and r2 in S1.
 	for i := range keyRanges {
-		if totalKeyRanges[i].End.Key.Compare(keyRanges[i].End.Key) > 0 {
+		if totalKeyRanges[i].End.Compare(keyRanges[i].End) > 0 {
 			subsumedReplSSTFile := &storage.MemFile{}
 			subsumedReplSST := storage.MakeIngestionSSTWriter(subsumedReplSSTFile)
 			defer subsumedReplSST.Close()
 			if err := storage.ClearRangeWithHeuristic(
 				r.store.Engine(),
 				&subsumedReplSST,
-				keyRanges[i].End.Key,
-				totalKeyRanges[i].End.Key,
+				keyRanges[i].End,
+				totalKeyRanges[i].End,
 			); err != nil {
 				subsumedReplSST.Close()
 				return err
@@ -1120,7 +1120,7 @@ func (r *Replica) clearSubsumedReplicaDiskData(
 		// Extending to the left implies that either we merged "to the left" (we
 		// don't), or that we're applying a snapshot for another range (we don't do
 		// that either). Something is severely wrong for this to happen.
-		if totalKeyRanges[i].Start.Key.Compare(keyRanges[i].Start.Key) < 0 {
+		if totalKeyRanges[i].Start.Compare(keyRanges[i].Start) < 0 {
 			log.Fatalf(ctx, "subsuming replica to our left; key range: %v; total key range %v",
 				keyRanges[i], totalKeyRanges[i])
 		}

pkg/kv/kvserver/replica_sst_snapshot_storage_test.go

@@ -150,7 +150,7 @@ func TestMultiSSTWriterInitSST(t *testing.T) {
 			sstFile := &storage.MemFile{}
 			sst := storage.MakeIngestionSSTWriter(sstFile)
 			defer sst.Close()
-			err := sst.ClearRawRange(r.Start.Key, r.End.Key)
+			err := sst.ClearRawRange(r.Start, r.End)
 			require.NoError(t, err)
 			err = sst.Finish()
 			require.NoError(t, err)