Merge cockroachdb#108739 cockroachdb#108744 cockroachdb#108748 cockro…

…achdb#108752 cockroachdb#108764 cockroachdb#108765

108739: roachtest/costfuzz: create failure.log file on demand r=yuzefovich a=yuzefovich

This commit makes it so that `failure.log` files are created lazily on demand in `costfuzz` and `unoptimized_query_oracle` roachtests. It seems nicer this way since for successful runs previously we'd see empty failure log files which were a bit confusing.

Epic: None

Release note: None

108744: build: update aws-cli r=pavelkalinnikov,srosenberg,herkolategan,rail a=renatolabs

The version being previously used was quite old, and didn't support setting the `Throughput` parameter on `gp3` volumes.

See: cockroachdb#108629 (comment).

Epic: none

Release note: None

108748: CODEOWNERS: split sql-queries-prs from sql-queries r=knz,rytaft a=michae2

This allows us to add or remove people from the PR rotation while keeping them on the team.

Epic: None
Release note: None

108752: ui: fix errors thrown by 0.toNumber() r=sjbarag a=sjbarag

When generating JS protobuf bindings, protobufjs's `pbjs` CLI accepts a --strict-long (or the more modern --force-long) flag with the following description:

  > Enfores the use of 'Long' for s-/u-/int64 and s-/fixed64 fields.

This uses the `long` package on NPM[^1], which it expects to be present when one of those clients is used at runtime. If `long` isn't available when `pbjs` is executed, it falls back to generating number-based fields where possible. Long is available in all consumers of `crdb-protobuf-client`, and `protobufjs` declares `long` as one of its few build-time and run-time dependencies, so nothing looked alarming.

For each field with a type that might be represented by a Long (a heuristic influenced by --strict-long), that field is marked as "long" if and only if the protobufjs-internal util.Long is non-null[^2]. That util.Long value is set via the use of the ``@protobufjs/inquire`` package instead of a standard `require`[^3] though, since the former silently returns `null` instead of throwing an error[^4]. This, again, is not alarming on its own because `protobufjs` depends directly on `long`.

The complication comes in with rules_js' use of the pnpm visibility rules, which are very strict when it comes to JS package dependencies. Put simply: packages only have access to the dependencies they directly declare, not transitive dependencies ("phantom dependencies")[^5]. Since ``@protobufjs/inquire`` is a separate package with no direct dependencies of its own[^6], Bazel lays out a node_modules tree where ``@protobufjs/inquire`` has no access to the `long` package. Since inaccessible packages are indistinguishable from nonexistent packages and `inquire` is intended to be silent, it returns `null`. The `pbjs` CLI then assumes Long isn't installed and falls back to number-based fields where a Long isn't strictly required.

When combined with protobuf's zero-value, this caused several cases of '0.toNumber is not a function' JS errors at runtime[^7]. We requested a Long, we support Longs, but a client was silently generated without respecting --strict-long.

Like with most other phantom dependencies, use pnpm.packageOverrides to declare that `@protobufjs/inquire` does in fact depend on Long.

[^1]: https://www.npmjs.com/package/long
[^2]: https://github.com/protobufjs/protobuf.js/blob/918ff014efe19f3eb43195ae3d71f7aeb3fcdd73/src/field.js#L153-L157
[^3]: https://github.com/protobufjs/protobuf.js/blob/918ff014efe19f3eb43195ae3d71f7aeb3fcdd73/src/util/minimal.js#L163-L167
[^4]: https://github.com/protobufjs/protobuf.js/blob/918ff014efe19f3eb43195ae3d71f7aeb3fcdd73/lib/inquire/index.js#L4-L17
[^5]: https://docs.aspect.build/rules/aspect_rules_js/docs/pnpm/#hoisting
[^6]: https://github.com/protobufjs/protobuf.js/blob/918ff014efe19f3eb43195ae3d71f7aeb3fcdd73/lib/inquire/package.json
[^7]: cockroachdb#106318

Fixes: cockroachdb#106318
Release note: Observability pages no longer crash when they encounter zeros (e.g. a session with no memory allocated).

108764: sql/physicalplan: make more tests compatible with secondary tenants  r=maryliag,yuzefovich a=knz

Informs cockroachdb#76378.
Links to cockroachdb#108763.
Epic: CRDB-18499

108765: sql: make TestScatterResponse work with secondary tenants r=yuzefovich a=knz

First two commits from cockroachdb#108764
Informs cockroachdb#76378.
Epic: CRDB-18499

Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Renato Costa <[email protected]>
Co-authored-by: Michael Erickson <[email protected]>
Co-authored-by: Sean Barag <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>

.github/CODEOWNERS

@@ -38,30 +38,30 @@
 
 #!/pkg/sql/                    @cockroachdb/sql-queries-noreview
 
-/pkg/sql/inverted/           @cockroachdb/sql-queries
-/pkg/sql/opt/                @cockroachdb/sql-queries
-/pkg/sql/opt_*.go            @cockroachdb/sql-queries
+/pkg/sql/inverted/           @cockroachdb/sql-queries-prs
+/pkg/sql/opt/                @cockroachdb/sql-queries-prs
+/pkg/sql/opt_*.go            @cockroachdb/sql-queries-prs
 #!/pkg/sql/opt/exec/execbuilder/testdata/ @cockroachdb/sql-queries-noreview
-/pkg/sql/plan_opt*.go        @cockroachdb/sql-queries
-/pkg/sql/querycache/         @cockroachdb/sql-queries
-/pkg/sql/span/               @cockroachdb/sql-queries
-/pkg/sql/stats/              @cockroachdb/sql-queries
-
-/pkg/sql/col*                @cockroachdb/sql-queries
-/pkg/sql/create_stats*       @cockroachdb/sql-queries
-/pkg/sql/distsql*.go         @cockroachdb/sql-queries
-/pkg/sql/exec*               @cockroachdb/sql-queries
+/pkg/sql/plan_opt*.go        @cockroachdb/sql-queries-prs
+/pkg/sql/querycache/         @cockroachdb/sql-queries-prs
+/pkg/sql/span/               @cockroachdb/sql-queries-prs
+/pkg/sql/stats/              @cockroachdb/sql-queries-prs
+
+/pkg/sql/col*                @cockroachdb/sql-queries-prs
+/pkg/sql/create_stats*       @cockroachdb/sql-queries-prs
+/pkg/sql/distsql*.go         @cockroachdb/sql-queries-prs
+/pkg/sql/exec*               @cockroachdb/sql-queries-prs
 #!/pkg/sql/exec_log*.go        @cockroachdb/sql-queries-noreview
 #!/pkg/sql/exec_util*.go       @cockroachdb/sql-queries-noreview
-/pkg/sql/flowinfra/          @cockroachdb/sql-queries
-/pkg/sql/physicalplan/       @cockroachdb/sql-queries
-/pkg/sql/row*                @cockroachdb/sql-queries
-/pkg/sql/control_job*        @cockroachdb/sql-queries @cockroachdb/jobs-prs
-/pkg/sql/job_exec_context*   @cockroachdb/sql-queries @cockroachdb/jobs-prs
+/pkg/sql/flowinfra/          @cockroachdb/sql-queries-prs
+/pkg/sql/physicalplan/       @cockroachdb/sql-queries-prs
+/pkg/sql/row*                @cockroachdb/sql-queries-prs
+/pkg/sql/control_job*        @cockroachdb/sql-queries-prs @cockroachdb/jobs-prs
+/pkg/sql/job_exec_context*   @cockroachdb/sql-queries-prs @cockroachdb/jobs-prs
 /pkg/sql/delegate/*job*.go   @cockroachdb/jobs-prs @cockroachdb/disaster-recovery
 
-/pkg/sql/importer/           @cockroachdb/sql-queries
-/pkg/ccl/importerccl/        @cockroachdb/sql-queries
+/pkg/sql/importer/           @cockroachdb/sql-queries-prs
+/pkg/ccl/importerccl/        @cockroachdb/sql-queries-prs
 
 /pkg/sql/appstatspb          @cockroachdb/cluster-observability
 /pkg/sql/execstats/          @cockroachdb/cluster-observability
@@ -176,7 +176,7 @@
 /pkg/server/decommission*.go             @cockroachdb/kv-prs      @cockroachdb/server-prs
 /pkg/server/drain*.go                    @cockroachdb/kv-prs      @cockroachdb/server-prs
 /pkg/server/dumpstore/                   @cockroachdb/obs-inf-prs @cockroachdb/server-prs
-/pkg/server/external_storage*.go         @cockroachdb/sql-queries @cockroachdb/server-prs
+/pkg/server/external_storage*.go         @cockroachdb/sql-queries-prs @cockroachdb/server-prs
 /pkg/server/fanout*.go                   @cockroachdb/obs-inf-prs @cockroachdb/server-prs
 /pkg/server/diagnostics/                 @cockroachdb/obs-inf-prs
 /pkg/server/dumpstore/                   @cockroachdb/obs-inf-prs @cockroachdb/server-prs
@@ -257,7 +257,7 @@
 /pkg/kv/kvbase/                         @cockroachdb/kv-prs
 /pkg/kv/kvclient/                       @cockroachdb/kv-prs
 /pkg/kv/kvclient/kvcoord/*rangefeed*    @cockroachdb/repl-prs
-/pkg/kv/kvclient/kvstreamer             @cockroachdb/sql-queries
+/pkg/kv/kvclient/kvstreamer             @cockroachdb/sql-queries-prs
 /pkg/kv/kvclient/rangefeed/             @cockroachdb/repl-prs
 /pkg/kv/kvnemesis/                      @cockroachdb/kv-prs
 /pkg/kv/kvpb/                           @cockroachdb/kv-prs
@@ -387,7 +387,7 @@
 /pkg/ccl/telemetryccl/       @cockroachdb/obs-inf-prs
 
 /pkg/ccl/testccl/authccl/    @cockroachdb/cloud-identity @cockroachdb/prodsec
-/pkg/ccl/testccl/sqlccl/     @cockroachdb/sql-queries
+/pkg/ccl/testccl/sqlccl/     @cockroachdb/sql-queries-prs
 /pkg/ccl/testccl/workload/schemachange/ @cockroachdb/sql-foundations
 #!/pkg/ccl/testutilsccl/     @cockroachdb/test-eng-noreview
 /pkg/ccl/testutilsccl/alter_* @cockroachdb/sql-foundations
@@ -432,7 +432,7 @@
 /pkg/cmd/protoc-gen-gogoroach/ @cockroachdb/dev-inf
 /pkg/cmd/publish-artifacts/  @cockroachdb/dev-inf
 /pkg/cmd/publish-provisional-artifacts/ @cockroachdb/dev-inf
-/pkg/cmd/reduce/             @cockroachdb/sql-queries
+/pkg/cmd/reduce/             @cockroachdb/sql-queries-prs
 /pkg/cmd/release/            @cockroachdb/dev-inf
 /pkg/cmd/returncheck/        @cockroachdb/dev-inf
 /pkg/cmd/roachprod/          @cockroachdb/test-eng
@@ -451,9 +451,9 @@
 /pkg/cmd/skip-test/          @cockroachdb/test-eng
 /pkg/cmd/skiperrs/           @cockroachdb/sql-foundations
 /pkg/cmd/skipped-tests/      @cockroachdb/test-eng
-/pkg/cmd/smith/              @cockroachdb/sql-queries
-/pkg/cmd/smithcmp/           @cockroachdb/sql-queries
-/pkg/cmd/smithtest/          @cockroachdb/sql-queries
+/pkg/cmd/smith/              @cockroachdb/sql-queries-prs
+/pkg/cmd/smithcmp/           @cockroachdb/sql-queries-prs
+/pkg/cmd/smithtest/          @cockroachdb/sql-queries-prs
 /pkg/cmd/teamcity-trigger/   @cockroachdb/dev-inf
 /pkg/cmd/testfilter/         @cockroachdb/test-eng
 /pkg/cmd/uptodate/           @cockroachdb/dev-inf
@@ -462,7 +462,7 @@
 /pkg/cmd/workload/           @cockroachdb/test-eng #! @cockroachdb/sql-foundations-noreview
 #!/pkg/cmd/wraprules/          @cockroachdb/obs-inf-prs-noreview
 #!/pkg/cmd/zerosum/            @cockroachdb/kv-noreview
-/pkg/col/                    @cockroachdb/sql-queries
+/pkg/col/                    @cockroachdb/sql-queries-prs
 /pkg/compose/                @cockroachdb/sql-foundations
 /pkg/config/                 @cockroachdb/kv-prs @cockroachdb/server-prs
 # TODO(nickvigilante): add the cockroach repo to the docs-infra-prs team so that
@@ -474,8 +474,8 @@
 /pkg/internal/client/requestbatcher/ @cockroachdb/kv-prs
 /pkg/internal/codeowners/    @cockroachdb/test-eng
 /pkg/internal/reporoot       @cockroachdb/dev-inf
-/pkg/internal/rsg/           @cockroachdb/sql-queries
-/pkg/internal/sqlsmith/      @cockroachdb/sql-queries
+/pkg/internal/rsg/           @cockroachdb/sql-queries-prs
+/pkg/internal/sqlsmith/      @cockroachdb/sql-queries-prs
 /pkg/internal/team/          @cockroachdb/test-eng
 /pkg/internal/workloadreplay/          @cockroachdb/test-eng
 /pkg/jobs/                   @cockroachdb/jobs-prs @cockroachdb/disaster-recovery
@@ -512,8 +512,8 @@
 /pkg/spanconfig/             @cockroachdb/kv-prs
 /pkg/repstream/              @cockroachdb/disaster-recovery
 #!/pkg/testutils/              @cockroachdb/test-eng-noreview
-/pkg/testutils/reduce/       @cockroachdb/sql-queries
-/pkg/testutils/sqlutils/     @cockroachdb/sql-queries
+/pkg/testutils/reduce/       @cockroachdb/sql-queries-prs
+/pkg/testutils/sqlutils/     @cockroachdb/sql-queries-prs
 /pkg/testutils/jobutils/     @cockroachdb/jobs-prs @cockroachdb/disaster-recovery
 /pkg/ts/                     @cockroachdb/kv-prs
 /pkg/ts/catalog/             @cockroachdb/obs-inf-prs

TEAMS.yaml

@@ -30,6 +30,7 @@ cockroachdb/sql-foundations:
   label: T-sql-foundations
 cockroachdb/sql-queries:
   aliases:
+    cockroachdb/sql-queries-prs: other
     cockroachdb/sql-optimizer: other
     cockroachdb/sql-opt-prs: other
   # SQL Queries team uses GH projects v2, which doesn't have a REST API, so

build/.bazelbuilderversion

@@ -1 +1 @@
-cockroachdb/bazel:20230803-060342
+cockroachdb/bazel:20230815-141035

build/.bazelbuilderversion-fips

@@ -1 +1 @@
-cockroachdb/bazel-fips:20230629-030909
+cockroachdb/bazel-fips:20230815-141035

build/bazelbuilder/Dockerfile

@@ -93,10 +93,10 @@ RUN apt-get purge -y \
 # NB: we don't use apt-get because we need an up to date version of awscli
 # If you update the below, you should probably also update build/builder/Dockerfile.
 RUN case ${TARGETPLATFORM} in \
-    "linux/amd64") ARCH=x86_64; SHASUM=7ee475f22c1b35cc9e53affbf96a9ffce91706e154a9441d0d39cbf8366b718e ;; \
-    "linux/arm64") ARCH=aarch64; SHASUM=624ebb04705d4909eb0d56d467fe6b8b5c53a8c59375ed520e70236120125077 ;; \
+    "linux/amd64") ARCH=x86_64; SHASUM=e679933eec90b0e5a75d485be6c2fae0f89a3f9ccdcb1748be69f8f456e9a85f ;; \
+    "linux/arm64") ARCH=aarch64; SHASUM=7d6460f795712ebdac7e3c60d4800dde682d136d909810402aac164f2789b860 ;; \
   esac && \
- curl -fsSL "https://awscli.amazonaws.com/awscli-exe-linux-$ARCH-2.0.30.zip" -o "awscliv2.zip" && \
+ curl -fsSL "https://awscli.amazonaws.com/awscli-exe-linux-$ARCH-2.13.9.zip" -o "awscliv2.zip" && \
  echo "$SHASUM awscliv2.zip" | sha256sum -c - && \
  unzip awscliv2.zip && \
  ./aws/install && \

build/builder.sh

@@ -3,7 +3,7 @@
 set -euo pipefail
 
 image=cockroachdb/builder
-version=20230629-033334
+version=20230815-142459
 
 function init() {
   docker build --tag="${image}" "$(dirname "${0}")/builder"

build/builder/Dockerfile

@@ -174,8 +174,8 @@ 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
 # If you update the below, you should probably also update build/bazelbuilder/Dockerfile.
-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 - && \
+RUN curl -fsSL "https://awscli.amazonaws.com/awscli-exe-linux-x86_64-2.13.9.zip" -o "awscliv2.zip" && \
+ echo "e679933eec90b0e5a75d485be6c2fae0f89a3f9ccdcb1748be69f8f456e9a85f  awscliv2.zip" | sha256sum -c - && \
  unzip awscliv2.zip && \
  ./aws/install && \
  rm -rf aws awscliv2.zip

pkg/ccl/multiregionccl/multiregion_system_table_test.go

@@ -55,7 +55,7 @@ func TestMrSystemDatabase(t *testing.T) {
 	tenantArgs := base.TestTenantArgs{
 		Settings: cs,
 		TenantID: id,
-		Locality: *cluster.Servers[0].Locality(),
+		Locality: cluster.Servers[0].Locality(),
 	}
 	_, tenantSQL := serverutils.StartTenant(t, cluster.Servers[0], tenantArgs)
 

pkg/ccl/testccl/sqlstatsccl/sql_stats_test.go

@@ -120,7 +120,7 @@ func TestSQLStatsRegions(t *testing.T) {
 		_, tenantDb := serverutils.StartTenant(t, server, base.TestTenantArgs{
 			Settings: st,
 			TenantID: roachpb.MustMakeTenantID(11),
-			Locality: *server.Locality(),
+			Locality: server.Locality(),
 		})
 		tenantDbs = append(tenantDbs, tenantDb)
 	}

pkg/cmd/roachtest/tests/query_comparison_util.go

@@ -158,15 +158,23 @@ func runOneRoundQueryComparison(
 		t.L().Printf("\n\n")
 	}
 
-	failureLogPath := filepath.Join(
-		t.ArtifactsDir(), fmt.Sprintf("%s%03d.failure.log", qct.name, iter),
-	)
-	failureLog, err := os.Create(failureLogPath)
-	if err != nil {
-		t.Fatalf("could not create %s%03d.failure.log: %v", qct.name, iter, err)
-	}
-	defer failureLog.Close()
+	// We will create the failure log file on demand.
+	var failureLog *os.File
+	defer func() {
+		if failureLog != nil {
+			_ = failureLog.Close()
+		}
+	}()
 	logFailure := func(stmt string, rows [][]string) {
+		if failureLog == nil {
+			failureLogName := fmt.Sprintf("%s%03d.failure.log", qct.name, iter)
+			failureLogPath := filepath.Join(t.ArtifactsDir(), failureLogName)
+			var err error
+			failureLog, err = os.Create(failureLogPath)
+			if err != nil {
+				t.Fatalf("could not create %s: %v", failureLogName, err)
+			}
+		}
 		fmt.Fprint(failureLog, stmt)
 		fmt.Fprint(failureLog, "\n----\n")
 		fmt.Fprint(failureLog, sqlutils.MatrixToStr(rows))

pkg/kv/kvserver/client_lease_test.go

@@ -1054,15 +1054,17 @@ func TestLeasePreferencesDuringOutage(t *testing.T) {
 
 	srv, err := tc.FindMemberServer(newLeaseHolder.StoreID)
 	require.NoError(t, err)
-	region, ok := srv.Locality().Find("region")
+	loc := srv.Locality()
+	region, ok := loc.Find("region")
 	require.True(t, ok)
 	require.Equal(t, "us", region)
 	require.Equal(t, 3, len(repl.Desc().Replicas().Voters().VoterDescriptors()))
 	// Validate that we upreplicated outside of SF.
 	for _, replDesc := range repl.Desc().Replicas().Voters().VoterDescriptors() {
 		serv, err := tc.FindMemberServer(replDesc.StoreID)
 		require.NoError(t, err)
-		dc, ok := serv.Locality().Find("dc")
+		memberLoc := serv.Locality()
+		dc, ok := memberLoc.Find("dc")
 		require.True(t, ok)
 		require.NotEqual(t, "sf", dc)
 	}

pkg/server/testserver.go

@@ -901,11 +901,27 @@ func (t *testTenant) DistSenderI() interface{} {
 	return t.sql.execCfg.DistSender
 }
 
+// NodeDescStoreI is part of the serverutils.ApplicationLayerInterface.
+func (t *testTenant) NodeDescStoreI() interface{} {
+	return t.sql.execCfg.DistSQLPlanner.NodeDescStore()
+}
+
 // InternalDB is part of the serverutils.ApplicationLayerInterface.
 func (t *testTenant) InternalDB() interface{} {
 	return t.sql.internalDB
 }
 
+// Locality is part of the serverutils.ApplicationLayerInterface.
+func (t *testTenant) Locality() roachpb.Locality {
+	return t.Cfg.Locality
+}
+
+// DistSQLPlanningNodeID is part of the serverutils.ApplicationLayerInterface.
+func (t *testTenant) DistSQLPlanningNodeID() roachpb.NodeID {
+	// See comments on replicaoracle.Config.
+	return 0
+}
+
 // LeaseManager is part of the serverutils.ApplicationLayerInterface.
 func (t *testTenant) LeaseManager() interface{} {
 	return t.sql.leaseMgr
@@ -1704,9 +1720,14 @@ func (ts *testServer) MustGetSQLNetworkCounter(name string) int64 {
 	return mustGetSQLCounterForRegistry(reg, name)
 }
 
-// Locality is part of the serverutils.StorageLayerInterface.
-func (ts *testServer) Locality() *roachpb.Locality {
-	return &ts.cfg.Locality
+// Locality is part of the serverutils.ApplicationLayerInterface.
+func (ts *testServer) Locality() roachpb.Locality {
+	return ts.cfg.Locality
+}
+
+// DistSQLPlanningNodeID is part of the serverutils.ApplicationLayerInterface.
+func (ts *testServer) DistSQLPlanningNodeID() roachpb.NodeID {
+	return ts.NodeID()
 }
 
 // LeaseManager is part of the serverutils.ApplicationLayerInterface.
@@ -1734,6 +1755,11 @@ func (ts *testServer) DistSenderI() interface{} {
 	return ts.distSender
 }
 
+// NodeDescStoreI is part of the serverutils.ApplicationLayerInterface.
+func (ts *testServer) NodeDescStoreI() interface{} {
+	return ts.sqlServer.execCfg.DistSQLPlanner.NodeDescStore()
+}
+
 // MigrationServer is part of the serverutils.ApplicationLayerInterface.
 func (ts *testServer) MigrationServer() interface{} {
 	return ts.topLevelServer.migrationServer

pkg/sql/distsql_physical_planner.go

@@ -4768,3 +4768,7 @@ func (dsp *DistSQLPlanner) createPlanForInsert(
 		execinfrapb.Ordering{})
 	return plan, nil
 }
+
+func (dsp *DistSQLPlanner) NodeDescStore() kvcoord.NodeDescStore {
+	return dsp.nodeDescs
+}

pkg/sql/physicalplan/BUILD.bazel

@@ -54,7 +54,6 @@ go_test(
     shard_count = 16,
     deps = [
         "//pkg/base",
-        "//pkg/gossip",
         "//pkg/keys",
         "//pkg/kv",
         "//pkg/kv/kvclient/kvcoord",