Merge #61214 #61300 #61349 #61356 #61376 #61410 #61427

61214: importccl: avoid random() collisions between KV batches r=pbardea a=pbardea

An import may parallelize the work to convert SQL rows into KVs. During
this phase, default expressions are evaluated. Previously, IMPORT's
implementations of random number generates that are evaluated in default
expressions assumed that all of the given row IDs were contiguous. This
is not the case since 1 row converter may be responsible for converting
several non-contiguous batches of rows. This resulted in random_values
colliding between different batches of KV space.

This commit fixes this bug by feeding in the current position and
resetting the random source backing these methods. This ensures that
import treats a new contiguous batch of rows separately.

Fixes #61203.

Release justification: bug fix
Release note (bug fix): Fix a bug where random numbers generated as
default expressions during IMPORT would collide a few hundred rows apart
from eachother.

61300: sql: set zone configs before backfill on ALTER TABLE LOCALITY/PRIMARY KEY r=arulajmani,ajstorm a=otan

Ensure that the zone configurations are correctly set before the
backfill runs, so data is backfilled to the correct location upfront for
REGIONAL BY ROW tables. Note this bug also exists on existing ALTER
PRIMARY KEY statements.

Release justification: bug fixes and low-risk updates to new
functionality

Release note (bug fix): Fix bug where zone configurations on indexes
were not copied before the backfill of an ALTER PRIMARY KEY. They used
to be copied afterwards instead.



61349: sql: add is_temporary and is_virtual columns to crdb_internal.create_statements r=rafiss a=RichardJCai

These columns are useful for filtering for generating output for
SHOW CREATE ALL TABLES.

Release justification: None, change to internal table.
Release note: None

61356: server: fix node decommissioning itself r=knz,tbg a=erikgrinaker

Previously, if a node was asked to decommission itself, the
decommissioning process would sometimes hang or fail since the node
would become decommissioned and lose RPC access to the rest of the
cluster while it was building the response.

This patch returns an empty response from the
`adminServer.Decommission()` call when setting the final
`DECOMMISSIONED` status, thereby avoiding further use of cluster RPC
after decommissioning itself. It also defers self-decommissioning until
the end if multiple nodes are being decommissioned.

This change is backwards-compatible with old CLI versions, which will
simply output the now-empty status result set before completing with
"No more data reported on target nodes". The CLI has been updated to
simply omit the empty response.

Resolves #56718.

A separate commit ensures errors with `codes.PermissionDenied` abort
RPC retries and return immediately, to prevent operations hanging with
indefinite internal retries when RPC access is lost.

Release justification: bug fixes and low-risk updates to new functionality

Release note (bug fix): Fixed a bug from 21.1-alpha where a node
decommissioning process could sometimes hang or fail when the
decommission request was submitted via the node being decommissioned.

61376: opt: prune unnecessary columns in uniqueness checks r=mgartner a=mgartner

Projects now wrap semi-joins of unique checks which pass-through the
columns of the unique constraint. This allows normalization rules to
prune unnecessary columns from the expression.

Release justification: This is a low-risk change to new functionality,
implicitly partitioned unique indexes.

Release note (performance improvement): The columns fetched for
uniqueness checks of implicitly partitioned unique indexes are now
pruned to only include columns necessary for determining uniqueness.


61410: sql: disallow GRANT/REVOKE on system tables r=RichardJCai a=RichardJCai

Disallow GRANT/REVOKE operations on system objects to avoid potential
deadlocks related to version bumps.

Release justification: bug fix
Release note (sql change): Disallow `GRANT/REVOKE` operations on system
tables.
Release note (bug fix): Fix a bug where `GRANT/REVOKE` on the
`system.lease` table would result in a deadlock.

Fixes #43842

Picking up PR from here:
#53683

It seems like the `system.comments` migration that used the GRANT/REVOKE is gone now (comment here: #53683 (comment))

61427: geomfn: prevent smaller densifyFrac for ST_{Frechet,Hausdorff}Distance r=rafiss a=otan

Release justification: low-risk change to existing functionality

Resolves #61367.

Release note (sql change): Prevent densifyFracs < 1e-6 for
ST_FrechetDistance and ST_HausdorffDistance to protect panics and out of
memory errors.

Co-authored-by: Paul Bardea <pbardea@gmail.com>
Co-authored-by: Oliver Tan <otan@cockroachlabs.com>
Co-authored-by: richardjcai <caioftherichard@gmail.com>
Co-authored-by: Erik Grinaker <grinaker@cockroachlabs.com>
Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>

pkg/ccl/importccl/import_stmt_test.go

@@ -3968,7 +3968,7 @@ func TestImportDefault(t *testing.T) {
 
 		}
 	})
-	t.Run("random-related", func(t *testing.T) {
+	t.Run("random-functions", func(t *testing.T) {
 		testCases := []struct {
 			name       string
 			create     string
@@ -3978,19 +3978,19 @@ func TestImportDefault(t *testing.T) {
 		}{
 			{
 				name:       "random-multiple",
-				create:     "a INT, b FLOAT DEFAULT random(), c STRING, d FLOAT DEFAULT random()",
+				create:     "a INT, b FLOAT PRIMARY KEY DEFAULT random(), c STRING, d FLOAT DEFAULT random()",
 				targetCols: []string{"a", "c"},
 				randomCols: []string{selectNotNull("b"), selectNotNull("d")},
 			},
 			{
 				name:       "gen_random_uuid",
-				create:     "a INT, b STRING, c UUID DEFAULT gen_random_uuid()",
+				create:     "a INT, b STRING, c UUID PRIMARY KEY DEFAULT gen_random_uuid(), d UUID DEFAULT gen_random_uuid()",
 				targetCols: []string{"a", "b"},
-				randomCols: []string{selectNotNull("c")},
+				randomCols: []string{selectNotNull("c"), selectNotNull("d")},
 			},
 			{
 				name:       "mixed_random_uuid",
-				create:     "a INT, b STRING, c UUID DEFAULT gen_random_uuid(), d FLOAT DEFAULT random()",
+				create:     "a INT, b STRING, c UUID PRIMARY KEY DEFAULT gen_random_uuid(), d FLOAT DEFAULT random()",
 				targetCols: []string{"a", "b"},
 				randomCols: []string{selectNotNull("c")},
 			},
@@ -4033,6 +4033,50 @@ func TestImportDefault(t *testing.T) {
 	})
 }
 
+// This is a regression test for #61203. We test that the random() keys are
+// unique on a larger data set. This would previously fail with a primary key
+// collision error since we would generate duplicate UUIDs.
+//
+// Note: that although there is no guarantee that UUIDs do not collide, the
+// probability of such a collision is vanishingly low.
+func TestUniqueUUID(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	// This test is slow under race since it explicitly tried to import a large
+	// amount of data.
+	skip.UnderRace(t, "slow under race")
+
+	const (
+		nodes     = 3
+		dataDir   = "userfile://defaultdb.my_files/export"
+		dataFiles = dataDir + "/*"
+	)
+	ctx := context.Background()
+	args := base.TestServerArgs{}
+	tc := testcluster.StartTestCluster(t, nodes, base.TestClusterArgs{ServerArgs: args})
+	defer tc.Stopper().Stop(ctx)
+	connDB := tc.Conns[0]
+	sqlDB := sqlutils.MakeSQLRunner(connDB)
+
+	dataSize := parallelImporterReaderBatchSize * 100
+
+	sqlDB.Exec(t, fmt.Sprintf(`CREATE TABLE data AS SELECT * FROM generate_series(1, %d);`, dataSize))
+	sqlDB.Exec(t, `EXPORT INTO CSV $1 FROM TABLE data;`, dataDir)
+
+	// Ensure that UUIDs do not collide when importing 20000 rows.
+	sqlDB.Exec(t, `CREATE TABLE r1 (a UUID PRIMARY KEY DEFAULT gen_random_uuid(), b INT);`)
+	sqlDB.Exec(t, `IMPORT INTO r1 (b) CSV DATA ($1);`, dataFiles)
+
+	// Ensure that UUIDs do not collide when importing into a table with several UUID calls.
+	sqlDB.Exec(t, `CREATE TABLE r2 (a UUID PRIMARY KEY DEFAULT gen_random_uuid(), b INT, c UUID DEFAULT gen_random_uuid());`)
+	sqlDB.Exec(t, `IMPORT INTO r2 (b) CSV DATA ($1);`, dataFiles)
+
+	// Ensure that random keys do not collide.
+	sqlDB.Exec(t, `CREATE TABLE r3 (a FLOAT PRIMARY KEY DEFAULT random(), b INT);`)
+	sqlDB.Exec(t, `IMPORT INTO r3 (b) CSV DATA ($1);`, dataFiles)
+}
+
 func TestImportDefaultNextVal(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)

pkg/ccl/kvccl/kvtenantccl/connector.go

@@ -329,8 +329,8 @@ func (c *Connector) RangeLookup(
 		})
 		if err != nil {
 			log.Warningf(ctx, "error issuing RangeLookup RPC: %v", err)
-			if grpcutil.IsAuthenticationError(err) {
-				// Authentication error. Propagate.
+			if grpcutil.IsAuthError(err) {
+				// Authentication or authorization error. Propagate.
 				return nil, nil, err
 			}
 			// Soft RPC error. Drop client and retry.

pkg/ccl/kvccl/kvtenantccl/connector_test.go

@@ -330,7 +330,7 @@ func TestConnectorRangeLookup(t *testing.T) {
 	desc, err := c.FirstRange()
 	require.Nil(t, desc)
 	require.Regexp(t, "does not have access to FirstRange", err)
-	require.True(t, grpcutil.IsAuthenticationError(err))
+	require.True(t, grpcutil.IsAuthError(err))
 }
 
 // TestConnectorRetriesUnreachable tests that Connector iterates over each of

pkg/ccl/logictestccl/testdata/logic_test/partitioning_implicit

@@ -656,7 +656,6 @@ vectorized: true
 │           │     spans: FULL SCAN
 │           │
 │           └── • scan buffer
-│                 estimated row count: 1
 │                 label: buffer 1
 │
 └── • constraint-check
@@ -674,7 +673,6 @@ vectorized: true
             │     spans: FULL SCAN
             │
             └── • scan buffer
-                  estimated row count: 1
                   label: buffer 1
 
 statement ok

pkg/ccl/multiregionccl/BUILD.bazel

@@ -32,6 +32,7 @@ go_test(
         "//pkg/base",
         "//pkg/ccl/multiregionccl/multiregionccltestutils",
         "//pkg/ccl/partitionccl",
+        "//pkg/ccl/testutilsccl",
         "//pkg/ccl/utilccl",
         "//pkg/jobs",
         "//pkg/keys",

pkg/ccl/multiregionccl/regional_by_row_test.go

@@ -16,6 +16,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/ccl/multiregionccl/multiregionccltestutils"
+	"github.com/cockroachdb/cockroach/pkg/ccl/testutilsccl"
 	"github.com/cockroachdb/cockroach/pkg/jobs"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
@@ -34,6 +35,129 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+// TestAlterTableLocalityRegionalByRowCorrectZoneConfigBeforeBackfill tests that
+// the zone configurations are properly set up before the LOCALITY REGIONAL BY ROW
+// backfill begins.
+func TestAlterTableLocalityRegionalByRowCorrectZoneConfigBeforeBackfill(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	testCases := []testutilsccl.AlterPrimaryKeyCorrectZoneConfigTestCase{
+		{
+			Desc:       "REGIONAL BY TABLE to REGIONAL BY ROW",
+			SetupQuery: `CREATE TABLE t.test (k INT NOT NULL, v INT) LOCALITY REGIONAL BY TABLE`,
+			AlterQuery: `ALTER TABLE t.test SET LOCALITY REGIONAL BY ROW`,
+			ExpectedIntermediateZoneConfigs: []testutilsccl.AlterPrimaryKeyCorrectZoneConfigIntermediateZoneConfig{
+				{
+					ShowConfigStatement: `SHOW ZONE CONFIGURATION FOR TABLE t.test`,
+					ExpectedTarget:      `DATABASE t`,
+					ExpectedSQL: `ALTER DATABASE t CONFIGURE ZONE USING
+	range_min_bytes = 134217728,
+	range_max_bytes = 536870912,
+	gc.ttlseconds = 90000,
+	num_replicas = 3,
+	num_voters = 3,
+	constraints = '{+region=ajstorm-1: 1}',
+	voter_constraints = '[+region=ajstorm-1]',
+	lease_preferences = '[[+region=ajstorm-1]]'`,
+				},
+				{
+					ShowConfigStatement: `SHOW ZONE CONFIGURATION FOR PARTITION "ajstorm-1" OF INDEX t.test@new_primary_key`,
+					ExpectedTarget:      `PARTITION "ajstorm-1" OF INDEX t.public.test@new_primary_key`,
+					ExpectedSQL: `ALTER PARTITION "ajstorm-1" OF INDEX t.public.test@new_primary_key CONFIGURE ZONE USING
+	range_min_bytes = 134217728,
+	range_max_bytes = 536870912,
+	gc.ttlseconds = 90000,
+	num_replicas = 3,
+	num_voters = 3,
+	constraints = '{+region=ajstorm-1: 1}',
+	voter_constraints = '[+region=ajstorm-1]',
+	lease_preferences = '[[+region=ajstorm-1]]'`,
+				},
+			},
+		},
+		{
+			Desc:       "GLOBAL to REGIONAL BY ROW",
+			SetupQuery: `CREATE TABLE t.test (k INT NOT NULL, v INT) LOCALITY GLOBAL`,
+			AlterQuery: `ALTER TABLE t.test SET LOCALITY REGIONAL BY ROW`,
+			ExpectedIntermediateZoneConfigs: []testutilsccl.AlterPrimaryKeyCorrectZoneConfigIntermediateZoneConfig{
+				{
+					ShowConfigStatement: `SHOW ZONE CONFIGURATION FOR TABLE t.test`,
+					ExpectedTarget:      `TABLE t.public.test`,
+					ExpectedSQL: `ALTER TABLE t.public.test CONFIGURE ZONE USING
+	range_min_bytes = 134217728,
+	range_max_bytes = 536870912,
+	gc.ttlseconds = 90000,
+	global_reads = true,
+	num_replicas = 3,
+	num_voters = 3,
+	constraints = '{+region=ajstorm-1: 1}',
+	voter_constraints = '[+region=ajstorm-1]',
+	lease_preferences = '[[+region=ajstorm-1]]'`,
+				},
+				{
+					ShowConfigStatement: `SHOW ZONE CONFIGURATION FOR PARTITION "ajstorm-1" OF INDEX t.test@new_primary_key`,
+					ExpectedTarget:      `PARTITION "ajstorm-1" OF INDEX t.public.test@new_primary_key`,
+					ExpectedSQL: `ALTER PARTITION "ajstorm-1" OF INDEX t.public.test@new_primary_key CONFIGURE ZONE USING
+	range_min_bytes = 134217728,
+	range_max_bytes = 536870912,
+	gc.ttlseconds = 90000,
+	num_replicas = 3,
+	num_voters = 3,
+	constraints = '{+region=ajstorm-1: 1}',
+	voter_constraints = '[+region=ajstorm-1]',
+	lease_preferences = '[[+region=ajstorm-1]]'`,
+				},
+			},
+		},
+		{
+			Desc:       "REGIONAL BY ROW to REGIONAL BY TABLE",
+			SetupQuery: `CREATE TABLE t.test (k INT NOT NULL, v INT) LOCALITY REGIONAL BY ROW`,
+			AlterQuery: `ALTER TABLE t.test SET LOCALITY REGIONAL BY TABLE`,
+			ExpectedIntermediateZoneConfigs: []testutilsccl.AlterPrimaryKeyCorrectZoneConfigIntermediateZoneConfig{
+				{
+					ShowConfigStatement: `SHOW ZONE CONFIGURATION FOR TABLE t.test`,
+					ExpectedTarget:      `DATABASE t`,
+					ExpectedSQL: `ALTER DATABASE t CONFIGURE ZONE USING
+	range_min_bytes = 134217728,
+	range_max_bytes = 536870912,
+	gc.ttlseconds = 90000,
+	num_replicas = 3,
+	num_voters = 3,
+	constraints = '{+region=ajstorm-1: 1}',
+	voter_constraints = '[+region=ajstorm-1]',
+	lease_preferences = '[[+region=ajstorm-1]]'`,
+				},
+			},
+		},
+		{
+			Desc:       "REGIONAL BY ROW to GLOBAL",
+			SetupQuery: `CREATE TABLE t.test (k INT NOT NULL, v INT) LOCALITY REGIONAL BY ROW`,
+			AlterQuery: `ALTER TABLE t.test SET LOCALITY GLOBAL`,
+			ExpectedIntermediateZoneConfigs: []testutilsccl.AlterPrimaryKeyCorrectZoneConfigIntermediateZoneConfig{
+				{
+					ShowConfigStatement: `SHOW ZONE CONFIGURATION FOR TABLE t.test`,
+					ExpectedTarget:      `DATABASE t`,
+					ExpectedSQL: `ALTER DATABASE t CONFIGURE ZONE USING
+	range_min_bytes = 134217728,
+	range_max_bytes = 536870912,
+	gc.ttlseconds = 90000,
+	num_replicas = 3,
+	num_voters = 3,
+	constraints = '{+region=ajstorm-1: 1}',
+	voter_constraints = '[+region=ajstorm-1]',
+	lease_preferences = '[[+region=ajstorm-1]]'`,
+				},
+			},
+		},
+	}
+	testutilsccl.AlterPrimaryKeyCorrectZoneConfigTest(
+		t,
+		`CREATE DATABASE t PRIMARY REGION "ajstorm-1"`,
+		testCases,
+	)
+}
+
 // TestAlterTableLocalityRegionalByRowError tests an alteration involving
 // REGIONAL BY ROW which gets its async job interrupted by some sort of
 // error or cancellation. After this, we expect the table to retain
@@ -60,7 +184,11 @@ func TestAlterTableLocalityRegionalByRowError(t *testing.T) {
 	ctx := context.Background()
 
 	const showCreateTableStringSQL = `SELECT create_statement FROM [SHOW CREATE TABLE t.test]`
-	const showZoneConfigurationSQL = `SHOW ZONE CONFIGURATION FROM TABLE t.test`
+	const zoneConfigureSQLStatements = `
+		SELECT coalesce(string_agg(raw_config_sql, ';' ORDER BY raw_config_sql), 'NULL')
+		FROM crdb_internal.zones
+		WHERE database_name = 't' AND table_name = 'test'
+	`
 
 	// alterState is a struct that contains an action for a base test case
 	// to execute ALTER TABLE t.test SET LOCALITY <locality> against.
@@ -262,18 +390,18 @@ USE t;
 							require.Error(t, err)
 							require.Contains(t, err.Error(), errorMode.errorContains)
 
-							// Grab a copy of SHOW CREATE TABLE and SHOW ZONE CONFIGURATION before we run
+							// Grab a copy of SHOW CREATE TABLE and zone configuration data before we run
 							// any ALTER query. The result should match if the operation fails.
 							var originalCreateTableOutput string
 							require.NoError(
 								t,
 								sqlDB.QueryRow(showCreateTableStringSQL).Scan(&originalCreateTableOutput),
 							)
 
-							var originalTarget, originalZoneConfig string
+							var originalZoneConfig string
 							require.NoError(
 								t,
-								sqlDB.QueryRow(showZoneConfigurationSQL).Scan(&originalTarget, &originalZoneConfig),
+								sqlDB.QueryRow(zoneConfigureSQLStatements).Scan(&originalZoneConfig),
 							)
 
 							// Ensure that the mutations corresponding to the primary key change are cleaned up and
@@ -316,16 +444,14 @@ USE t;
 								}
 
 								// Ensure SHOW ZONE CONFIGURATION has not changed.
-								var target, zoneConfig string
+								var zoneConfig string
 								require.NoError(
 									t,
-									sqlDB.QueryRow(showZoneConfigurationSQL).Scan(&target, &zoneConfig),
+									sqlDB.QueryRow(zoneConfigureSQLStatements).Scan(&zoneConfig),
 								)
-								if !(target == originalTarget && zoneConfig == originalZoneConfig) {
+								if zoneConfig != originalZoneConfig {
 									return errors.Errorf(
-										"expected zone configuration to not have changed, got %s/%s, sql %s/%s",
-										originalTarget,
-										target,
+										"expected zone configuration statements to not have changed, got %s, sql %s",
 										originalZoneConfig,
 										zoneConfig,
 									)

pkg/ccl/partitionccl/BUILD.bazel

@@ -30,6 +30,7 @@ go_test(
     name = "partitionccl_test",
     size = "medium",
     srcs = [
+        "alter_primary_key_test.go",
         "drop_test.go",
         "main_test.go",
         "partition_test.go",
@@ -40,6 +41,7 @@ go_test(
         "//pkg/base",
         "//pkg/ccl/importccl",
         "//pkg/ccl/storageccl",
+        "//pkg/ccl/testutilsccl",
         "//pkg/ccl/utilccl",
         "//pkg/config",
         "//pkg/config/zonepb",

pkg/ccl/partitionccl/alter_primary_key_test.go

@@ -0,0 +1,51 @@
+// Copyright 2021 The Cockroach Authors.
+//
+// Licensed as a CockroachDB Enterprise file under the Cockroach Community
+// License (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+//     https://github.com/cockroachdb/cockroach/blob/master/licenses/CCL.txt
+
+package partitionccl
+
+import (
+	"testing"
+
+	"github.com/cockroachdb/cockroach/pkg/ccl/testutilsccl"
+	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+	"github.com/cockroachdb/cockroach/pkg/util/log"
+)
+
+func TestAlterPrimaryKeyCorrectZoneConfigBeforeBackfill(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	testCases := []testutilsccl.AlterPrimaryKeyCorrectZoneConfigTestCase{
+		{
+			Desc: "ALTER PRIMARY KEY",
+			SetupQuery: `CREATE TABLE t.test (k INT NOT NULL, v INT NOT NULL, INDEX v_idx (v));
+ALTER INDEX t.test@v_idx CONFIGURE ZONE USING gc.ttlseconds = 888
+			`,
+			AlterQuery: `ALTER TABLE t.test ALTER PRIMARY KEY USING COLUMNS (v)`,
+			ExpectedIntermediateZoneConfigs: []testutilsccl.AlterPrimaryKeyCorrectZoneConfigIntermediateZoneConfig{
+				{
+					ShowConfigStatement: `SHOW ZONE CONFIGURATION FOR INDEX t.test@v_idx_rewrite_for_primary_key_change`,
+					ExpectedTarget:      `INDEX t.public.test@v_idx_rewrite_for_primary_key_change`,
+					ExpectedSQL: `ALTER INDEX t.public.test@v_idx_rewrite_for_primary_key_change CONFIGURE ZONE USING
+	range_min_bytes = 134217728,
+	range_max_bytes = 536870912,
+	gc.ttlseconds = 888,
+	num_replicas = 1,
+	constraints = '[]',
+	lease_preferences = '[]'`,
+				},
+			},
+		},
+	}
+
+	testutilsccl.AlterPrimaryKeyCorrectZoneConfigTest(
+		t,
+		`CREATE DATABASE t`,
+		testCases,
+	)
+}