Merge #85346 #88844 #88859 #88875 #88966 #89081 #89131 #89169 #89195 #…

…89200 #89211

85346: builtins: recategorize some builtins for docs r=rafiss a=jordanlewis

Updates cockroachdb/docs#11842

This commit moves the json generator functions into the JSON
documentation category rather than the more generically named
"set-returning functions" category.

Also, move the "levenshtein" function into the fuzzy text search
category.

Also, move another couple of internal functions into "system info".

Release note: None

88844: *: Use CPut when writing to `system.descriptor` table r=Xiang-Gu a=Xiang-Gu

Each individual commit has a rather thorough message, so I encourage
reviewers to read them as well.

Commit 1: code pattern, non-functional changes
Commit 2: Added a `[]byte` field in descriptor builder
and immutable struct. We also modified existing functions
to carry over this field between descriptor and Mutable/immutable
descriptor, and vice versa.

Commit 3: Upgrade `WriteDescToBatch` to use `CPut` rather than
`Put` to prevent clobbering of the `system.descriptor` table.
We get the expected value for the `CPut` when first reading the
descriptor into collection from storage, as well as keep booking
the descriptor's latest change inside the `uncommitted` descriptor
set, in case more we write to the same descriptor more than once
in the same transaction.

I didn't add any tests in this PR since this changed function 
`WriteDescToBatch` is heavily invoked for many sql stmts,
so I think we should be pretty confident about its correctness
if it passes all existing tests (unit, logic, and roachtest).

Release note: None

88859: cluster-ui: add helper to determine empty sql results and export sql api wrapper r=xinhaoz a=xinhaoz

### Commit 1
This commit adds a helper function, `sqlResultsAreEmpty` to
the sql api that determines whether there are execution
results in the request response.

Release note: None

### Commit 2

Release note: None

88875: sql/sqlutil, descs: move `TxnWithExecutor()` and methods to `sql.InternalExecutorFactory` r=ajwerner a=ZhouXing19

Having `TxnWithExecutor()` in `descs.CollectionFactory` is unnatural, and will bring dependency loop headaches. This commit is to move the same logic under `sql.InternalExecutorFactory`.

Release note: None

88966: build: support running extra checks on ARM r=rail a=healthy-pod

This code change changes CI scripts for acceptance, bench,
and roachtest to be runnable on ARM machines.

Release note: None

89081: builtins: fix doc formatting r=rafiss a=aliher1911

Some builtins had docs incorrectly quoted resulting in wrong formatting applied on generated web page docs.

Release note: None

89131: stmtdiagnostics: remove conditional request from registry after completion r=yuzefovich a=yuzefovich

Previously, we had a minor bug in how we handle the conditional
diagnostics requests when we got a bundle that satisfied the condition - we
correctly updated the corresponding system table, but we forgot to remove
the request from the local registry. As a result, we would continue
collecting conditional bundles until the local node polls the system
table and updates its registry (every 10 seconds by default). This
commit fixes that issue. Additionally, this commit updates the tests to
enforce that the in-memory registry doesn't contain completed requests.

Release note: None

89169: changefeedccl: verify changefeed failure for reverted import r=samiskin a=samiskin

Resolves #82943

A new test verifies that even if an import was reverted the changefeed still fails on a table offline error.  This is now needed to ensure we don't have undesired behavior during a case where  rangefeeds would emit range tombstones.

Release note: None

89195: storage: correctly check that a value is a tombstone r=erikgrinaker a=sumeerbhola

We can't rely on the byte slice being of length 0. This was not a correctness bug, but limits a wider MVCC range tombstone.

Release note: None

89200: roachtest: disable decimal columns in costfuzz and unoptimized tests r=rytaft a=rytaft

The use of decimal columns was making costfuzz and unoptimized-query-oracle tests flaky. This commit disables generation of decimal columns as a temporary mitigation for these flakes.

Fixes #88547

Release note: None

89211: roachtest: skip cdc/bank on ARM64 r=miretskiy a=healthy-pod

This code change skips `cdc/bank` on ARM64 because Confluent CLI is not available on `linux/arm64`.

Release note: None

Co-authored-by: Jordan Lewis <[email protected]>
Co-authored-by: Xiang Gu <[email protected]>
Co-authored-by: Xin Hao Zhang <[email protected]>
Co-authored-by: Jane Xing <[email protected]>
Co-authored-by: healthy-pod <[email protected]>
Co-authored-by: Oleg Afanasyev <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Shiranka Miskin <[email protected]>
Co-authored-by: sumeerbhola <[email protected]>
Co-authored-by: Rebecca Taft <[email protected]>

build/teamcity/cockroach/ci/tests/acceptance.sh

@@ -6,8 +6,15 @@ dir="$(dirname $(dirname $(dirname $(dirname $(dirname "${0}")))))"
 source "$dir/teamcity-support.sh"
 source "$dir/teamcity-bazel-support.sh"  # For run_bazel
 
+if [[ "$(uname -m)" =~ (arm64|aarch64)$ ]]; then
+  export CROSSLINUX_CONFIG="crosslinuxarm"
+else
+  export CROSSLINUX_CONFIG="crosslinux"
+fi
+
 tc_start_block "Build cockroach"
-run_bazel /usr/bin/bash -c 'bazel build --config crosslinux --config ci //pkg/cmd/cockroach-short && cp $(bazel info bazel-bin --config crosslinux --config ci)/pkg/cmd/cockroach-short/cockroach-short_/cockroach-short /artifacts/cockroach && chmod a+w /artifacts/cockroach'
+build_script='bazel build --config $1 --config ci //pkg/cmd/cockroach-short && cp $(bazel info bazel-bin --config $1 --config ci)/pkg/cmd/cockroach-short/cockroach-short_/cockroach-short artifacts/cockroach && chmod a+w artifacts/cockroach'
+run_bazel /usr/bin/bash -c "$build_script" -- "$CROSSLINUX_CONFIG"
 tc_end_block "Build cockroach"
 
 export ARTIFACTSDIR=$PWD/artifacts/acceptance
@@ -21,7 +28,7 @@ BAZCI=$(bazel info bazel-bin --config=ci)/pkg/cmd/bazci/bazci_/bazci
 
 $BAZCI --artifacts_dir=$PWD/artifacts -- \
   test //pkg/acceptance:acceptance_test \
-  --config=crosslinux --config=ci \
+  --config=$CROSSLINUX_CONFIG --config=ci \
   "--sandbox_writable_path=$ARTIFACTSDIR" \
   "--test_tmpdir=$ARTIFACTSDIR" \
   --test_arg=-l="$ARTIFACTSDIR" \

build/teamcity/cockroach/ci/tests/bench_impl.sh

@@ -5,6 +5,12 @@ set -euo pipefail
 dir="$(dirname $(dirname $(dirname $(dirname $(dirname "${0}")))))"
 source "$dir/teamcity/util.sh"
 
+if [[ "$(uname -m)" =~ (arm64|aarch64)$ ]]; then
+  export CROSSLINUX_CONFIG="crosslinuxarm"
+else
+  export CROSSLINUX_CONFIG="crosslinux"
+fi
+
 # Enumerate test targets that have benchmarks.
 all_tests=$(bazel query 'kind(go_test, //pkg/...)' --output=label)
 pkgs=$(git grep -l '^func Benchmark' -- 'pkg/*_test.go' | rev | cut -d/ -f2- | rev | sort | uniq)
@@ -21,7 +27,7 @@ do
     tc_start_block "Bench $target"
     # We need the `test_sharding_strategy` flag or else the benchmarks will
     # fail to run sharded tests like //pkg/sql/importer:importer_test.
-    bazel run --config=test --config=crosslinux --config=ci --test_sharding_strategy=disabled $target -- \
+    bazel run --config=test --config=$CROSSLINUX_CONFIG --config=ci --test_sharding_strategy=disabled $target -- \
           -test.bench=. -test.benchtime=1ns -test.short -test.run=-
     tc_end_block "Bench $target"
 done

build/teamcity/cockroach/ci/tests/local_roachtest_impl.sh

@@ -2,11 +2,17 @@
 
 set -euo pipefail
 
-bazel build --config=crosslinux --config=ci //pkg/cmd/cockroach-short \
+if [[ "$(uname -m)" =~ (arm64|aarch64)$ ]]; then
+  export CROSSLINUX_CONFIG="crosslinuxarm"
+else
+  export CROSSLINUX_CONFIG="crosslinux"
+fi
+
+bazel build --config=$CROSSLINUX_CONFIG --config=ci //pkg/cmd/cockroach-short \
       //pkg/cmd/roachtest \
       //pkg/cmd/workload
 
-BAZEL_BIN=$(bazel info bazel-bin --config=crosslinux --config=ci)
+BAZEL_BIN=$(bazel info bazel-bin --config=$CROSSLINUX_CONFIG --config=ci)
 $BAZEL_BIN/pkg/cmd/roachtest/roachtest_/roachtest run acceptance kv/splits cdc/bank \
   --local \
   --parallelism=1 \

pkg/ccl/backupccl/backup_metadata_test.go

@@ -25,7 +25,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
-	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
+	"github.com/cockroachdb/cockroach/pkg/sql/sqlutil"
 	"github.com/cockroachdb/cockroach/pkg/sql/stats"
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
@@ -88,7 +88,7 @@ func checkMetadata(
 		blobs.TestEmptyBlobClientFactory,
 		username.RootUserName(),
 		tc.Servers[0].InternalExecutor().(*sql.InternalExecutor),
-		tc.Servers[0].CollectionFactory().(*descs.CollectionFactory),
+		tc.Servers[0].InternalExecutorFactory().(sqlutil.InternalExecutorFactory),
 		tc.Servers[0].DB(),
 		nil, /* limiters */
 	)

pkg/ccl/backupccl/backup_test.go

@@ -573,7 +573,7 @@ func TestBackupRestoreAppend(t *testing.T) {
 				blobs.TestEmptyBlobClientFactory,
 				username.RootUserName(),
 				tc.Servers[0].InternalExecutor().(*sql.InternalExecutor),
-				tc.Servers[0].CollectionFactory().(*descs.CollectionFactory),
+				tc.Servers[0].InternalExecutorFactory().(sqlutil.InternalExecutorFactory),
 				tc.Servers[0].DB(),
 				nil, /* limiters */
 			)
@@ -8027,7 +8027,7 @@ func TestReadBackupManifestMemoryMonitoring(t *testing.T) {
 		blobs.TestBlobServiceClient(dir),
 		username.RootUserName(),
 		nil, /* ie */
-		nil, /* cf */
+		nil, /* ief */
 		nil, /* kvDB */
 		nil, /* limiters */
 	)

pkg/ccl/backupccl/restore_data_processor_test.go

@@ -255,7 +255,7 @@ func runTestIngest(t *testing.T, init func(*cluster.Settings)) {
 				return cloud.MakeExternalStorage(ctx, dest, base.ExternalIODirConfig{},
 					s.ClusterSettings(), blobs.TestBlobServiceClient(s.ClusterSettings().ExternalIODir),
 					nil, /* ie */
-					nil, /* cf */
+					nil, /* ief */
 					nil, /* kvDB */
 					nil, /* limiters */
 					opts...)

pkg/ccl/backupccl/restore_job.go

@@ -2223,7 +2223,7 @@ func (r *restoreResumer) OnFailOrCancel(
 	logJobCompletion(ctx, restoreJobEventType, r.job.ID(), false, jobErr)
 
 	execCfg := execCtx.(sql.JobExecContext).ExecCfg()
-	if err := execCfg.CollectionFactory.TxnWithExecutor(ctx, execCfg.DB, p.SessionData(), func(
+	if err := execCfg.InternalExecutorFactory.DescsTxnWithExecutor(ctx, execCfg.DB, p.SessionData(), func(
 		ctx context.Context, txn *kv.Txn, descsCol *descs.Collection, ie sqlutil.InternalExecutor,
 	) error {
 		for _, tenant := range details.Tenants {

pkg/ccl/backupccl/restore_planning.go

@@ -706,7 +706,7 @@ func getDatabaseIDAndDesc(
 // as regular databases, we drop them before restoring them again in the
 // restore.
 func dropDefaultUserDBs(ctx context.Context, execCfg *sql.ExecutorConfig) error {
-	return execCfg.CollectionFactory.TxnWithExecutor(ctx, execCfg.DB, nil /* session data */, func(
+	return execCfg.InternalExecutorFactory.DescsTxnWithExecutor(ctx, execCfg.DB, nil /* session data */, func(
 		ctx context.Context, txn *kv.Txn, _ *descs.Collection, ie sqlutil.InternalExecutor,
 	) error {
 		_, err := ie.Exec(ctx, "drop-defaultdb", txn, "DROP DATABASE IF EXISTS defaultdb")

pkg/ccl/changefeedccl/changefeed_test.go

@@ -2648,25 +2648,62 @@ func TestChangefeedFailOnTableOffline(t *testing.T) {
 	}))
 	defer dataSrv.Close()
 
-	testFn := func(t *testing.T, s TestServer, f cdctest.TestFeedFactory) {
+	cdcTestNamed(t, "import fails changefeed", func(t *testing.T, s TestServer, f cdctest.TestFeedFactory) {
 		sqlDB := sqlutils.MakeSQLRunner(s.DB)
 		sqlDB.Exec(t, "SET CLUSTER SETTING kv.closed_timestamp.target_duration = '50ms'")
-		t.Run("import fails changefeed", func(t *testing.T) {
-			sqlDB.Exec(t, `CREATE TABLE for_import (a INT PRIMARY KEY, b INT)`)
-			defer sqlDB.Exec(t, `DROP TABLE for_import`)
-			sqlDB.Exec(t, `INSERT INTO for_import VALUES (0, NULL)`)
-			forImport := feed(t, f, `CREATE CHANGEFEED FOR for_import `)
-			defer closeFeed(t, forImport)
-			assertPayloads(t, forImport, []string{
-				`for_import: [0]->{"after": {"a": 0, "b": null}}`,
-			})
-			sqlDB.Exec(t, `IMPORT INTO for_import CSV DATA ($1)`, dataSrv.URL)
-			requireErrorSoon(context.Background(), t, forImport,
-				regexp.MustCompile(`CHANGEFEED cannot target offline table: for_import \(offline reason: "importing"\)`))
-		})
-	}
+		sqlDB.Exec(t, `CREATE TABLE for_import (a INT PRIMARY KEY, b INT)`)
+		defer sqlDB.Exec(t, `DROP TABLE for_import`)
+		sqlDB.Exec(t, `INSERT INTO for_import VALUES (0, NULL)`)
+		forImport := feed(t, f, `CREATE CHANGEFEED FOR for_import `)
+		defer closeFeed(t, forImport)
+		assertPayloads(t, forImport, []string{
+			`for_import: [0]->{"after": {"a": 0, "b": null}}`,
+		})
+		sqlDB.Exec(t, `IMPORT INTO for_import CSV DATA ($1)`, dataSrv.URL)
+		requireErrorSoon(context.Background(), t, forImport,
+			regexp.MustCompile(`CHANGEFEED cannot target offline table: for_import \(offline reason: "importing"\)`))
+	})
 
-	cdcTest(t, testFn)
+	cdcTestNamedWithSystem(t, "reverted import fails changefeed with earlier cursor", func(t *testing.T, s TestServerWithSystem, f cdctest.TestFeedFactory) {
+		sysSQLDB := sqlutils.MakeSQLRunner(s.SystemDB)
+		sysSQLDB.Exec(t, "SET CLUSTER SETTING kv.bulk_io_write.small_write_size = '1'")
+		sysSQLDB.Exec(t, "SET CLUSTER SETTING storage.mvcc.range_tombstones.enabled = true")
+
+		sqlDB := sqlutils.MakeSQLRunner(s.DB)
+		sqlDB.Exec(t, `CREATE TABLE for_import (a INT PRIMARY KEY, b INT)`)
+
+		var start string
+		sqlDB.QueryRow(t, `SELECT cluster_logical_timestamp()`).Scan(&start)
+		sqlDB.Exec(t, "INSERT INTO for_import VALUES (0, 10);")
+
+		// Start an import job which will immediately pause after ingestion
+		sqlDB.Exec(t, "SET CLUSTER SETTING jobs.debug.pausepoints = 'import.after_ingest';")
+		go func() {
+			sqlDB.ExpectErr(t, `pause point`, `IMPORT INTO for_import CSV DATA ($1);`, dataSrv.URL)
+		}()
+		sqlDB.CheckQueryResultsRetry(
+			t,
+			fmt.Sprintf(`SELECT count(*) FROM [SHOW JOBS] WHERE job_type='IMPORT' AND status='%s'`, jobs.StatusPaused),
+			[][]string{{"1"}},
+		)
+
+		// Cancel to trigger a revert and verify revert completion
+		var jobID string
+		sqlDB.QueryRow(t, `SELECT job_id FROM [SHOW JOBS] where job_type='IMPORT'`).Scan(&jobID)
+		sqlDB.Exec(t, `CANCEL JOB $1`, jobID)
+		sqlDB.CheckQueryResultsRetry(
+			t,
+			fmt.Sprintf(`SELECT count(*) FROM [SHOW JOBS] WHERE job_type='IMPORT' AND status='%s'`, jobs.StatusCanceled),
+			[][]string{{"1"}},
+		)
+		sqlDB.CheckQueryResultsRetry(t, "SELECT count(*) FROM for_import", [][]string{{"1"}})
+
+		// Changefeed should fail regardless
+		forImport := feed(t, f, `CREATE CHANGEFEED FOR for_import WITH cursor=$1`, start)
+		defer closeFeed(t, forImport)
+		requireErrorSoon(context.Background(), t, forImport,
+			regexp.MustCompile(`CHANGEFEED cannot target offline table: for_import \(offline reason: "importing"\)`))
+	})
 }
 
 func TestChangefeedRestartMultiNode(t *testing.T) {

pkg/ccl/changefeedccl/schemafeed/schema_feed.go

@@ -84,15 +84,16 @@ func New(
 	tolerances changefeedbase.CanHandle,
 ) SchemaFeed {
 	m := &schemaFeed{
-		filter:            schemaChangeEventFilters[events],
-		db:                cfg.DB,
-		clock:             cfg.DB.Clock(),
-		settings:          cfg.Settings,
-		targets:           targets,
-		leaseMgr:          cfg.LeaseManager.(*lease.Manager),
-		collectionFactory: cfg.CollectionFactory,
-		metrics:           metrics,
-		tolerances:        tolerances,
+		filter:                  schemaChangeEventFilters[events],
+		db:                      cfg.DB,
+		clock:                   cfg.DB.Clock(),
+		settings:                cfg.Settings,
+		targets:                 targets,
+		leaseMgr:                cfg.LeaseManager.(*lease.Manager),
+		collectionFactory:       cfg.CollectionFactory,
+		internalExecutorFactory: cfg.InternalExecutorFactory,
+		metrics:                 metrics,
+		tolerances:              tolerances,
 	}
 	m.mu.previousTableVersion = make(map[descpb.ID]catalog.TableDescriptor)
 	m.mu.highWater = initialHighwater
@@ -122,8 +123,9 @@ type schemaFeed struct {
 	// TODO(ajwerner): Should this live underneath the FilterFunc?
 	// Should there be another function to decide whether to update the
 	// lease manager?
-	leaseMgr          *lease.Manager
-	collectionFactory *descs.CollectionFactory
+	leaseMgr                *lease.Manager
+	collectionFactory       *descs.CollectionFactory
+	internalExecutorFactory descs.TxnManager
 
 	mu struct {
 		syncutil.Mutex
@@ -291,7 +293,7 @@ func (tf *schemaFeed) primeInitialTableDescs(ctx context.Context) error {
 		})
 	}
 
-	if err := tf.collectionFactory.Txn(ctx, tf.db, initialTableDescsFn); err != nil {
+	if err := tf.internalExecutorFactory.DescsTxn(ctx, tf.db, initialTableDescsFn); err != nil {
 		return err
 	}
 

pkg/ccl/changefeedccl/sink_cloudstorage_test.go

@@ -176,7 +176,7 @@ func TestCloudStorageSink(t *testing.T) {
 			clientFactory,
 			user,
 			nil, /* ie */
-			nil, /* cf */
+			nil, /* ief */
 			nil, /* kvDB */
 			nil, /* limiters */
 			opts...)

pkg/ccl/storageccl/BUILD.bazel

@@ -46,7 +46,7 @@ go_test(
         "//pkg/security/username",
         "//pkg/server",
         "//pkg/sql",
-        "//pkg/sql/catalog/descs",
+        "//pkg/sql/sqlutil",
         "//pkg/storage",
         "//pkg/testutils",
         "//pkg/testutils/serverutils",

pkg/ccl/storageccl/external_sst_reader_test.go

@@ -20,7 +20,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/sql"
-	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
+	"github.com/cockroachdb/cockroach/pkg/sql/sqlutil"
 	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/storageutils"
@@ -124,7 +124,7 @@ func TestNewExternalSSTReader(t *testing.T) {
 			blobs.TestBlobServiceClient(tempDir),
 			username.RootUserName(),
 			tc.Servers[0].InternalExecutor().(*sql.InternalExecutor),
-			tc.Servers[0].CollectionFactory().(*descs.CollectionFactory),
+			tc.Servers[0].InternalExecutorFactory().(sqlutil.InternalExecutorFactory),
 			tc.Servers[0].DB(),
 			nil, /* limiters */
 		)

pkg/ccl/workloadccl/storage.go

@@ -43,7 +43,7 @@ func GetStorage(ctx context.Context, cfg FixtureConfig) (cloud.ExternalStorage,
 		nil, /* blobClientFactory */
 		username.SQLUsername{},
 		nil, /* ie */
-		nil, /* cf */
+		nil, /* ief */
 		nil, /* kvDB */
 		nil, /* limiters */
 	)

pkg/cloud/BUILD.bazel

@@ -23,7 +23,6 @@ go_library(
         "//pkg/security/username",
         "//pkg/settings",
         "//pkg/settings/cluster",
-        "//pkg/sql/catalog/descs",
         "//pkg/sql/sqlutil",
         "//pkg/util/ctxgroup",
         "//pkg/util/ioctx",