Merge #86363 #87067 #87103 #87212 #87229

86363: bazel: make `go test` process timeout before bazel kills it r=rickystewart a=healthy-pod

Previously, bazel will kill the test process if it goes
beyond the timeout duration set for its size. This prevented
us from knowing which tests timed out and also prevented us
from getting their stack traces.

This patch causes the `go test` process to timeout before
bazel kills it to allow us to know which test timed out and
get its stack trace.

Closes #78185

Release justification: Non-production code changes
Release note: None

87067: descs,catkv: rewrite Collection backend r=postamar a=postamar

This rewrite was motivated by the upcoming need to validate descriptors
up to different levels depending on why they were being read. Doing so
requires tracking the validation level in a more granular fashion within
the Collection's caches. Trying to make this work within the existing
scheme where the uncommitted and stored descriptors are all in the same
layer turned out to be painful and hard to reason about.

This commit does several things:
1. The uncommitted descriptors are spun off into their own distinct layer
again.
2. The stored descriptors now mirror what's in storage and so that whole
layer could effectively be moved to catkv.
3. Execution flow through the layers in the Collection is now
straightforwardly implemented and easy to reason about. Validation and
hydration now take place in unique, well-defined steps at the very end
of a lookup, be it by name or by ID.
3. Instances of this new stored descriptors layer are now also used by
the lease manager, the Direct() interface provided by the Collection,
etc.
4. As a result the logic in catkv could be significantly cleaned up.
In particular, lookups for descriptor validation are significantly
less convoluted now.
5. The SystemNamespaceCache object was also moved to catkv and now
supports caching descriptors, and so was renamed to
SystemCatalogCache.

Despite significant rewriting of the inner workings of the descriptors
collection, this commit should not functionally change anything.

Informs #85263.

Release justification: low-risk, high-benefit change.
Release note: None


87103: storage: fix mvcc stats on gc of range tombstone over del r=erikgrinaker a=aliher1911

Previously if range tombstone was placed over delete, GC would
not correctly update GC bytes age incorrectly using range tombstone ts
as age of underlying tombstone.

Release justification: Bugfix
Release note: None

Fixing: #87042

87212: ci: set appropriate test tmpdir for compose test r=srosenberg a=rickystewart

Otherwise the tmpdir defaults to `/artifacts` which is non-existent
outside of a Docker container.

Release justification: Non-production code changes
Release note: None

87229: storage: fix leaked iterators in unit tests r=erikgrinaker a=jbowens

Fix a couple instances of leaked iterators in unit tests in the storage
package.

This should also resolve the goroutine leak observed in #86256.

Informs #71481.

Release justification: Non-production code changes
Release note: None

Co-authored-by: healthy-pod <[email protected]>
Co-authored-by: Marius Posta <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: Oleg Afanasyev <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
Co-authored-by: Jackson Owens <[email protected]>

.github/CODEOWNERS

@@ -245,7 +245,7 @@
 /pkg/cmd/generate-logictest/       @cockroachdb/dev-inf
 /pkg/cmd/generate-metadata-tables/ @cockroachdb/sql-experience
 /pkg/cmd/generate-spatial-ref-sys/ @cockroachdb/geospatial
-/pkg/cmd/generate-test-suites/ @cockroachdb/dev-inf
+/pkg/cmd/generate-bazel-extra/ @cockroachdb/dev-inf
 /pkg/cmd/generate-staticcheck/ @cockroachdb/dev-inf
 /pkg/cmd/geoviz/             @cockroachdb/geospatial
 /pkg/cmd/github-post/        @cockroachdb/test-eng

build/bazelutil/bazel-generate.sh

@@ -76,10 +76,13 @@ else
   bazel run pkg/gen/genbzl --run_under="cd $PWD && " -- --out-dir pkg/gen
 fi
 
-if files_unchanged_from_upstream $(find_relevant ./pkg/cmd/generate-test-suites -name BUILD.bazel -or -name '*.go') $(find_relevant ./pkg -name BUILD.bazel) $(find_relevant ./pkg -name '*.bzl'); then
-  echo "Skipping //pkg/cmd/generate-test-suites (relevant files are unchanged from upstream)."
+if ! (files_unchanged_from_upstream $(find_relevant ./pkg -name BUILD.bazel) $(find_relevant ./pkg/cmd/generate-bazel-extra -name BUILD.bazel -or -name '*.go')); then
+  bazel build @com_github_bazelbuild_buildtools//buildozer:buildozer
+  bazel run //pkg/cmd/generate-bazel-extra --run_under="cd $PWD && " -- -gen_test_suites -gen_tests_timeouts
+elif files_unchanged_from_upstream $(find_relevant ./pkg -name '*.bzl'); then
+  echo "Skipping //pkg/cmd/generate-bazel-extra (relevant files are unchanged from upstream)."
 else
+  echo "Skipping `generate tests timeouts` from //pkg/cmd/generate-bazel-extra (relevant files are unchanged from upstream)."
   bazel build @com_github_bazelbuild_buildtools//buildozer:buildozer
-  CONTENTS=$(bazel run //pkg/cmd/generate-test-suites --run_under="cd $PWD && ")
-  echo "$CONTENTS" > pkg/BUILD.bazel
+  bazel run //pkg/cmd/generate-bazel-extra --run_under="cd $PWD && " -- -gen_test_suites
 fi

build/bazelutil/check.sh

@@ -95,7 +95,7 @@ $GIT_GREP '//go:generate' 'pkg/**/*.go' | grep -v stringer | grep -v 'add-leakte
     exit 1
 done
 
-$GIT_GREP 'broken_in_bazel' pkg | grep BUILD.bazel: | grep -v pkg/BUILD.bazel | grep -v pkg/cli/BUILD.bazel | grep -v generate-test-suites | cut -d: -f1 | while read LINE; do
+$GIT_GREP 'broken_in_bazel' pkg | grep BUILD.bazel: | grep -v pkg/BUILD.bazel | grep -v pkg/cli/BUILD.bazel | grep -v generate-bazel-extra | cut -d: -f1 | while read LINE; do
     if [[ "$EXISTING_BROKEN_TESTS_IN_BAZEL" == *"$LINE"* ]]; then
 	# Grandfathered.
 	continue

build/teamcity/cockroach/nightlies/compose.sh

@@ -23,6 +23,8 @@ GO_TEST_JSON_OUTPUT_FILE=$ARTIFACTS_DIR/test.json.txt
 exit_status=0
 $BAZCI --artifacts_dir=$ARTIFACTS_DIR -- \
        test --config=ci //pkg/compose:compose_test \
+       "--sandbox_writable_path=$ARTIFACTSDIR" \
+       "--test_tmpdir=$ARTIFACTSDIR" \
        --test_env=GO_TEST_WRAP_TESTV=1 \
        --test_env=GO_TEST_JSON_OUTPUT_FILE=$GO_TEST_JSON_OUTPUT_FILE \
        --test_arg -cockroach --test_arg $COCKROACH \

pkg/BUILD.bazel

@@ -1,4 +1,4 @@
-# Code generated by generate-test-suites, DO NOT EDIT.
+# Code generated by generate-bazel-extra, DO NOT EDIT.
 # gazelle:proto_strip_import_prefix /pkg
 
 load("//build/bazelutil/unused_checker:unused.bzl", "unused_checker")
@@ -868,6 +868,8 @@ GO_TARGETS = [
     "//pkg/cmd/docs-issue-generation:docs-issue-generation_test",
     "//pkg/cmd/fuzz:fuzz",
     "//pkg/cmd/fuzz:fuzz_lib",
+    "//pkg/cmd/generate-bazel-extra:generate-bazel-extra",
+    "//pkg/cmd/generate-bazel-extra:generate-bazel-extra_lib",
     "//pkg/cmd/generate-binary:generate-binary",
     "//pkg/cmd/generate-binary:generate-binary_lib",
     "//pkg/cmd/generate-distdir:generate-distdir",
@@ -881,8 +883,6 @@ GO_TARGETS = [
     "//pkg/cmd/generate-spatial-ref-sys:generate-spatial-ref-sys_lib",
     "//pkg/cmd/generate-staticcheck:generate-staticcheck",
     "//pkg/cmd/generate-staticcheck:generate-staticcheck_lib",
-    "//pkg/cmd/generate-test-suites:generate-test-suites",
-    "//pkg/cmd/generate-test-suites:generate-test-suites_lib",
     "//pkg/cmd/geoviz:geoviz",
     "//pkg/cmd/geoviz:geoviz_lib",
     "//pkg/cmd/github-post:github-post",
@@ -2253,14 +2253,14 @@ GET_X_DATA_TARGETS = [
     "//pkg/cmd/docgen/extract:get_x_data",
     "//pkg/cmd/docs-issue-generation:get_x_data",
     "//pkg/cmd/fuzz:get_x_data",
+    "//pkg/cmd/generate-bazel-extra:get_x_data",
     "//pkg/cmd/generate-binary:get_x_data",
     "//pkg/cmd/generate-distdir:get_x_data",
     "//pkg/cmd/generate-logictest:get_x_data",
     "//pkg/cmd/generate-metadata-tables:get_x_data",
     "//pkg/cmd/generate-metadata-tables/rdbms:get_x_data",
     "//pkg/cmd/generate-spatial-ref-sys:get_x_data",
     "//pkg/cmd/generate-staticcheck:get_x_data",
-    "//pkg/cmd/generate-test-suites:get_x_data",
     "//pkg/cmd/geoviz:get_x_data",
     "//pkg/cmd/github-post:get_x_data",
     "//pkg/cmd/github-pull-request-make:get_x_data",

pkg/base/BUILD.bazel

@@ -56,6 +56,7 @@ go_test(
         "node_id_test.go",
         "store_spec_test.go",
     ],
+    args = ["-test.timeout=55s"],
     deps = [
         ":base",
         "//pkg/roachpb",

pkg/bench/BUILD.bazel

@@ -29,6 +29,7 @@ go_test(
         "main_test.go",
         "pgbench_test.go",
     ],
+    args = ["-test.timeout=55s"],
     embed = [":bench"],
     deps = [
         "//pkg/base",

pkg/bench/rttanalysis/BUILD.bazel

@@ -48,6 +48,7 @@ go_test(
         "validate_benchmark_data_test.go",
         "virtual_table_bench_test.go",
     ],
+    args = ["-test.timeout=895s"],
     data = glob(["testdata/**"]),
     embed = [":rttanalysis"],
     shard_count = 16,

pkg/bench/tpcc/BUILD.bazel

@@ -18,6 +18,7 @@ go_test(
         "tpcc_bench_options_test.go",
         "tpcc_bench_test.go",
     ],
+    args = ["-test.timeout=295s"],
     embed = [":tpcc"],
     deps = [
         "//pkg/base",

pkg/blobs/BUILD.bazel

@@ -37,6 +37,7 @@ go_test(
         "local_storage_test.go",
         "service_test.go",
     ],
+    args = ["-test.timeout=55s"],
     embed = [":blobs"],
     deps = [
         "//pkg/base",

pkg/build/starlarkutil/BUILD.bazel

@@ -12,6 +12,7 @@ go_library(
 go_test(
     name = "starlarkutil_test",
     srcs = ["starlarkutil_test.go"],
+    args = ["-test.timeout=295s"],
     embed = [":starlarkutil"],
     deps = ["@com_github_stretchr_testify//require"],
 )

pkg/build/util/BUILD.bazel

@@ -11,6 +11,7 @@ go_library(
 go_test(
     name = "util_test",
     srcs = ["util_test.go"],
+    args = ["-test.timeout=295s"],
     embed = [":util"],
     deps = ["@com_github_stretchr_testify//require"],
 )

pkg/ccl/backupccl/BUILD.bazel

@@ -177,6 +177,7 @@ go_test(
         "system_schema_test.go",
         "utils_test.go",
     ],
+    args = ["-test.timeout=3595s"],
     data = glob(["testdata/**"]) + ["//c-deps:libgeos"],
     embed = [":backupccl"],
     shard_count = 16,

pkg/ccl/backupccl/backupdest/BUILD.bazel

@@ -41,6 +41,7 @@ go_test(
         "incrementals_test.go",
         "main_test.go",
     ],
+    args = ["-test.timeout=295s"],
     embed = [":backupdest"],
     deps = [
         "//pkg/ccl/backupccl/backupbase",

pkg/ccl/backupccl/backupinfo/BUILD.bazel

@@ -52,6 +52,7 @@ go_library(
 go_test(
     name = "backupinfo_test",
     srcs = ["main_test.go"],
+    args = ["-test.timeout=295s"],
     embed = [":backupinfo"],
     deps = [
         "//pkg/ccl/utilccl",

pkg/ccl/backupccl/backupinfo/backup_metadata.go

@@ -304,8 +304,18 @@ func (a namespace) Swap(i, j int) { a[i], a[j] = a[j], a[i] }
 func (a namespace) Less(i, j int) bool {
 	if a[i].parent == a[j].parent {
 		if a[i].parentSchema == a[j].parentSchema {
-			cmp := strings.Compare(a[i].name, a[j].name)
-			return cmp < 0 || (cmp == 0 && (a[i].ts.IsEmpty() || a[j].ts.Less(a[i].ts)))
+			cmpName := strings.Compare(a[i].name, a[j].name)
+			if cmpName == 0 {
+				cmpTimestamp := a[j].ts.Compare(a[i].ts)
+				if cmpTimestamp == 0 {
+					return a[i].id > a[j].id
+				}
+				if a[i].ts.IsEmpty() {
+					return true
+				}
+				return cmpTimestamp < 0
+			}
+			return cmpName < 0
 		}
 		return a[i].parentSchema < a[j].parentSchema
 	}
@@ -333,27 +343,39 @@ func writeNamesToMetadata(
 		tb, db, typ, sc, fn := descpb.FromDescriptor(rev.Desc)
 		if db != nil {
 			names[i].name = db.Name
+			if db.State == descpb.DescriptorState_DROP {
+				names[i].id = 0
+			}
 		} else if sc != nil {
 			names[i].name = sc.Name
 			names[i].parent = sc.ParentID
+			if sc.State == descpb.DescriptorState_DROP {
+				names[i].id = 0
+			}
 		} else if tb != nil {
 			names[i].name = tb.Name
 			names[i].parent = tb.ParentID
 			names[i].parentSchema = keys.PublicSchemaID
 			if s := tb.UnexposedParentSchemaID; s != descpb.InvalidID {
 				names[i].parentSchema = s
 			}
-			if tb.Dropped() {
+			if tb.State == descpb.DescriptorState_DROP {
 				names[i].id = 0
 			}
 		} else if typ != nil {
 			names[i].name = typ.Name
 			names[i].parent = typ.ParentID
 			names[i].parentSchema = typ.ParentSchemaID
+			if typ.State == descpb.DescriptorState_DROP {
+				names[i].id = 0
+			}
 		} else if fn != nil {
 			names[i].name = fn.Name
 			names[i].parent = fn.ParentID
 			names[i].parentSchema = fn.ParentSchemaID
+			if fn.State == descpb.DescriptorState_DROP {
+				names[i].id = 0
+			}
 		}
 	}
 	sort.Sort(names)
@@ -362,11 +384,28 @@ func writeNamesToMetadata(
 		if rev.name == "" {
 			continue
 		}
-
 		if i > 0 {
 			prev := names[i-1]
+			prev.id = rev.id
+			if prev == rev {
+				// Name has multiple ID mappings at the same timestamp.
+				// At most one of these can be non-zero. Zero IDs correspond to name
+				// entry deletions following a DROP. If there is a non-zero ID, it means
+				// that the DROP was followed by a RENAME or a CREATE using the same
+				// name.
+				// The sort ordering of the names guarantees that the zero IDs are last,
+				// we therefore keep only the first mapping.
+				if rev.id != 0 {
+					return errors.AssertionFailedf(
+						"attempting to write duplicate name mappings for key (%d, %d, %q) at %s: IDs %d and %d",
+						rev.parent, rev.parentSchema, rev.name, rev.ts, prev.id, rev.id)
+				}
+				continue
+			}
+			prev = names[i-1]
 			prev.ts = rev.ts
 			if prev == rev {
+				// Ignore identical mappings at subsequent timestamps.
 				continue
 			}
 		}

pkg/ccl/backupccl/backuprand/BUILD.bazel

@@ -7,6 +7,7 @@ go_test(
         "backup_rand_test.go",
         "main_test.go",
     ],
+    args = ["-test.timeout=295s"],
     data = ["//c-deps:libgeos"],
     deps = [
         "//pkg/base",

pkg/ccl/backupccl/backupresolver/BUILD.bazel

@@ -29,6 +29,7 @@ go_test(
         "main_test.go",
         "targets_test.go",
     ],
+    args = ["-test.timeout=295s"],
     embed = [":backupresolver"],
     deps = [
         "//pkg/ccl/storageccl",

pkg/ccl/backupccl/restore_job.go

@@ -2271,7 +2271,7 @@ func (r *restoreResumer) dropDescriptors(
 		// and so we don't need to preserve MVCC semantics.
 		tableToDrop.DropTime = dropTime
 		b.Del(catalogkeys.EncodeNameKey(codec, tableToDrop))
-		descsCol.AddDeletedDescriptor(tableToDrop.GetID())
+		descsCol.NotifyOfDeletedDescriptor(tableToDrop.GetID())
 	}
 
 	// Drop the type descriptors that this restore created.
@@ -2293,7 +2293,7 @@ func (r *restoreResumer) dropDescriptors(
 		mutType.SetDropped()
 		// Remove the system.descriptor entry.
 		b.Del(catalogkeys.MakeDescMetadataKey(codec, typDesc.ID))
-		descsCol.AddDeletedDescriptor(mutType.GetID())
+		descsCol.NotifyOfDeletedDescriptor(mutType.GetID())
 	}
 
 	// Queue a GC job.
@@ -2365,7 +2365,7 @@ func (r *restoreResumer) dropDescriptors(
 
 		b.Del(catalogkeys.EncodeNameKey(codec, mutSchema))
 		b.Del(catalogkeys.MakeDescMetadataKey(codec, mutSchema.GetID()))
-		descsCol.AddDeletedDescriptor(mutSchema.GetID())
+		descsCol.NotifyOfDeletedDescriptor(mutSchema.GetID())
 		dbsWithDeletedSchemas[mutSchema.GetParentID()] = append(dbsWithDeletedSchemas[mutSchema.GetParentID()], mutSchema)
 	}
 
@@ -2444,7 +2444,7 @@ func (r *restoreResumer) dropDescriptors(
 
 		nameKey := catalogkeys.MakeDatabaseNameKey(codec, db.GetName())
 		b.Del(nameKey)
-		descsCol.AddDeletedDescriptor(db.GetID())
+		descsCol.NotifyOfDeletedDescriptor(db.GetID())
 		deletedDBs[db.GetID()] = struct{}{}
 	}
 

pkg/ccl/baseccl/BUILD.bazel

@@ -22,6 +22,7 @@ go_test(
     name = "baseccl_test",
     size = "small",
     srcs = ["encryption_spec_test.go"],
+    args = ["-test.timeout=55s"],
     embed = [":baseccl"],
     deps = ["//pkg/util/leaktest"],
 )

pkg/ccl/benchccl/rttanalysisccl/BUILD.bazel

@@ -8,6 +8,7 @@ go_test(
         "bench_test.go",
         "multi_region_bench_test.go",
     ],
+    args = ["-test.timeout=3595s"],
     data = glob(["testdata/**"]),
     shard_count = 16,
     deps = [

pkg/ccl/changefeedccl/BUILD.bazel

@@ -160,6 +160,7 @@ go_test(
         "testfeed_test.go",
         "validations_test.go",
     ],
+    args = ["-test.timeout=3595s"],
     embed = [":changefeedccl"],
     shard_count = 16,
     deps = [

pkg/ccl/changefeedccl/cdceval/BUILD.bazel

@@ -53,6 +53,7 @@ go_test(
         "main_test.go",
         "validation_test.go",
     ],
+    args = ["-test.timeout=295s"],
     embed = [":cdceval"],
     deps = [
         "//pkg/base",

pkg/ccl/changefeedccl/cdcevent/BUILD.bazel

@@ -42,6 +42,7 @@ go_test(
         "main_test.go",
         "projection_test.go",
     ],
+    args = ["-test.timeout=295s"],
     embed = [":cdcevent"],
     deps = [
         "//pkg/base",

pkg/ccl/changefeedccl/cdctest/BUILD.bazel

@@ -47,6 +47,7 @@ go_test(
         "main_test.go",
         "validator_test.go",
     ],
+    args = ["-test.timeout=55s"],
     embed = [":cdctest"],
     deps = [
         "//pkg/base",

pkg/ccl/changefeedccl/cdcutils/BUILD.bazel

@@ -20,6 +20,7 @@ go_library(
 go_test(
     name = "cdcutils_test",
     srcs = ["throttle_test.go"],
+    args = ["-test.timeout=295s"],
     embed = [":cdcutils"],
     deps = [
         "//pkg/ccl/changefeedccl/changefeedbase",

pkg/ccl/changefeedccl/changefeedbase/BUILD.bazel

@@ -27,6 +27,7 @@ go_library(
 go_test(
     name = "changefeedbase_test",
     srcs = ["options_test.go"],
+    args = ["-test.timeout=295s"],
     embed = [":changefeedbase"],
     deps = [
         "//pkg/util/leaktest",

pkg/ccl/changefeedccl/kvevent/BUILD.bazel

@@ -41,6 +41,7 @@ go_test(
         "blocking_buffer_test.go",
         "chunked_event_queue_test.go",
     ],
+    args = ["-test.timeout=295s"],
     embed = [":kvevent"],
     deps = [
         "//pkg/jobs/jobspb",

pkg/ccl/changefeedccl/kvfeed/BUILD.bazel

@@ -44,6 +44,7 @@ go_test(
         "main_test.go",
         "scanner_test.go",
     ],
+    args = ["-test.timeout=55s"],
     embed = [":kvfeed"],
     deps = [
         "//pkg/base",

pkg/ccl/changefeedccl/schemafeed/BUILD.bazel

@@ -52,6 +52,7 @@ go_test(
         "table_event_filter_datadriven_test.go",
         "table_event_filter_test.go",
     ],
+    args = ["-test.timeout=295s"],
     data = glob(["testdata/**"]),
     embed = [":schemafeed"],
     deps = [