Merge #63129 #63231 #63259 #63268

63129: sql: block initial multi-region if zone configs have multi-region fields set r=ajstorm,aayushshah15 a=otan

Refs: #63071

See individual commits for details.

Note I have not done overwriting. This is not going to be planned for 21.1 unless we deem it a GA-blocker.

63231: coldata: do not allocate wasteful memory for UUIDs r=yuzefovich a=yuzefovich

Previously, whenever we're allocating a new `Bytes` vector, we would use
64 bytes as an average element size to decide how large of a flat byte
slice to allocate initially. This is wasteful in case of Uuids because
then we know that each element will be exactly of 16 bytes. This commit
uses that knowledge to remove wasteful allocations.

Release note: None

63259: sql: add pgcode for DROP REGION on PRIMARY REGION r=ajstorm a=otan

Release note (sql change): Introduce a pgcode when attempting to DROP
REGION when the the region being dropped is the PRIMARY REGION.

63268: sql: deflake test with statement_timeout r=RaduBerinde a=RaduBerinde

I saw a CI failure in this test, caused by the `SHOW` statement itself
hitting the 100ms timeout. This points to something being unreasonably
slow on the agent; but it is easy enough to increase this arbitrary
value.

Release note: None

Co-authored-by: Oliver Tan <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Radu Berinde <[email protected]>

pkg/ccl/logictestccl/testdata/logic_test/multi_region

@@ -1015,7 +1015,7 @@ statement ok
 CREATE DATABASE drop_region_db PRIMARY REGION "ca-central-1" REGIONS "ap-southeast-2", "us-east-1";
 USE drop_region_db
 
-statement error pq: cannot drop region "ca-central-1"\nHINT: You must designate another region as the primary region using ALTER DATABASE drop_region_db PRIMARY REGION <region name> or remove all other regions before attempting to drop region "ca-central-1"
+statement error pgcode 42P12 cannot drop region "ca-central-1"\nHINT: You must designate another region as the primary region using ALTER DATABASE drop_region_db PRIMARY REGION <region name> or remove all other regions before attempting to drop region "ca-central-1"
 ALTER DATABASE drop_region_db DROP REGION "ca-central-1"
 
 statement ok
@@ -1108,7 +1108,7 @@ CREATE TABLE start_off_non_multi_region.public.t(a INT);
 ALTER DATABASE start_off_non_multi_region PRIMARY REGION "ca-central-1";
 ALTER DATABASE start_off_non_multi_region ADD REGION "ap-southeast-2"
 
-statement error pq: cannot drop region "ca-central-1"\nHINT: You must designate another region as the primary region using ALTER DATABASE start_off_non_multi_region PRIMARY REGION <region name> or remove all other regions before attempting to drop region "ca-central-1"
+statement error pgcode 42P12 cannot drop region "ca-central-1"\nHINT: You must designate another region as the primary region using ALTER DATABASE start_off_non_multi_region PRIMARY REGION <region name> or remove all other regions before attempting to drop region "ca-central-1"
 ALTER DATABASE start_off_non_multi_region DROP REGION "ca-central-1"
 
 statement ok
@@ -1203,7 +1203,7 @@ statement ok
 ALTER DATABASE drop_primary_regions_db DROP REGION "ca-central-1"
 
 # Cannot drop the primary region yet, as there are other regions in the db.
-statement error pq: cannot drop region "us-east-1"\nHINT: You must designate another region as the primary region using ALTER DATABASE drop_primary_regions_db PRIMARY REGION <region name> or remove all other regions before attempting to drop region "us-east-1"
+statement error pgcode 42P12 cannot drop region "us-east-1"\nHINT: You must designate another region as the primary region using ALTER DATABASE drop_primary_regions_db PRIMARY REGION <region name> or remove all other regions before attempting to drop region "us-east-1"
 ALTER DATABASE drop_primary_regions_db DROP REGION "us-east-1"
 
 statement ok

pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs

@@ -484,13 +484,13 @@ SET override_multi_region_zone_config = true;
 ALTER index regional_by_row@primary CONFIGURE ZONE USING num_replicas = 10;
 SET override_multi_region_zone_config = false
 
-statement error attempting to update zone config which contains an extra zone configuration for index regional_by_row@"primary"
+statement error attempting to update zone config which contains an extra zone configuration for index regional_by_row@"primary" with field num_replicas populated
 ALTER TABLE regional_by_row SET LOCALITY GLOBAL
 
-statement error extraneous zone configuration for index regional_by_row@"primary"
+statement error extraneous zone configuration for index regional_by_row@"primary" with field num_replicas populated
 SELECT crdb_internal.validate_multi_region_zone_configs()
 
-statement error attempting to update zone config which contains an extra zone configuration for index regional_by_row@"primary"
+statement error attempting to update zone config which contains an extra zone configuration for index regional_by_row@"primary" with field num_replicas populated
 ALTER TABLE regional_by_row SET LOCALITY REGIONAL BY ROW
 
 statement ok
@@ -525,7 +525,7 @@ INDEX regional_by_row_as@primary  ALTER INDEX regional_by_row_as@primary CONFIGU
                                   voter_constraints = '[+region=us-east-1]',
                                   lease_preferences = '[[+region=us-east-1]]'
 
-statement error attempting to update zone config which contains an extra zone configuration for index regional_by_row_as@"primary"
+statement error attempting to update zone config which contains an extra zone configuration for index regional_by_row_as@"primary" with field num_replicas populated
 ALTER TABLE regional_by_row_as SET LOCALITY REGIONAL BY ROW
 
 statement ok
@@ -629,3 +629,79 @@ statement ok
 SET override_multi_region_zone_config = true;
 ALTER PARTITION "ca-central-1" OF INDEX regional_by_row@primary CONFIGURE ZONE DISCARD;
 SET override_multi_region_zone_config = false
+
+# Test validation for initial SET PRIMARY REGION
+statement ok
+CREATE DATABASE initial_multiregion_db;
+USE initial_multiregion_db;
+CREATE TABLE tbl (a INT PRIMARY KEY, INDEX a_idx (a));
+ALTER DATABASE initial_multiregion_db CONFIGURE ZONE USING gc.ttlseconds = 5;
+ALTER TABLE tbl CONFIGURE ZONE USING gc.ttlseconds = 5;
+ALTER INDEX tbl@a_idx CONFIGURE ZONE USING gc.ttlseconds = 5
+
+statement ok
+ALTER DATABASE initial_multiregion_db SET PRIMARY REGION "us-east-1"
+
+query TT
+SHOW ZONE CONFIGURATION FOR DATABASE initial_multiregion_db
+----
+DATABASE initial_multiregion_db  ALTER DATABASE initial_multiregion_db CONFIGURE ZONE USING
+                                 range_min_bytes = 134217728,
+                                 range_max_bytes = 536870912,
+                                 gc.ttlseconds = 5,
+                                 num_replicas = 3,
+                                 num_voters = 3,
+                                 constraints = '{+region=us-east-1: 1}',
+                                 voter_constraints = '[+region=us-east-1]',
+                                 lease_preferences = '[[+region=us-east-1]]'
+
+query TT
+SHOW ZONE CONFIGURATION FOR TABLE tbl
+----
+TABLE tbl  ALTER TABLE tbl CONFIGURE ZONE USING
+           range_min_bytes = 134217728,
+           range_max_bytes = 536870912,
+           gc.ttlseconds = 5,
+           num_replicas = 3,
+           num_voters = 3,
+           constraints = '{+region=us-east-1: 1}',
+           voter_constraints = '[+region=us-east-1]',
+           lease_preferences = '[[+region=us-east-1]]'
+
+query TT
+SHOW ZONE CONFIGURATION FOR INDEX tbl@a_idx
+----
+INDEX tbl@a_idx  ALTER INDEX tbl@a_idx CONFIGURE ZONE USING
+                 range_min_bytes = 134217728,
+                 range_max_bytes = 536870912,
+                 gc.ttlseconds = 5,
+                 num_replicas = 3,
+                 num_voters = 3,
+                 constraints = '{+region=us-east-1: 1}',
+                 voter_constraints = '[+region=us-east-1]',
+                 lease_preferences = '[[+region=us-east-1]]'
+
+statement ok
+ALTER DATABASE initial_multiregion_db DROP REGION "us-east-1";
+ALTER INDEX tbl@a_idx CONFIGURE ZONE USING num_replicas = 10
+
+statement error zone configuration for index tbl@a_idx has field "num_replicas" set which will be overwritten when setting the initial PRIMARY REGION\nHINT: discard the zone config using CONFIGURE ZONE DISCARD before continuing
+ALTER DATABASE initial_multiregion_db SET PRIMARY REGION "us-east-1"
+
+statement ok
+ALTER INDEX tbl@a_idx CONFIGURE ZONE DISCARD;
+ALTER TABLE tbl CONFIGURE ZONE USING num_replicas = 10
+
+statement error zone configuration for table tbl has field "num_replicas" set which will be overwritten when setting the the initial PRIMARY REGION\nHINT: discard the zone config using CONFIGURE ZONE DISCARD before continuing
+ALTER DATABASE initial_multiregion_db SET PRIMARY REGION "us-east-1"
+
+statement ok
+ALTER TABLE tbl CONFIGURE ZONE DISCARD;
+ALTER DATABASE initial_multiregion_db CONFIGURE ZONE USING num_replicas = 10;
+
+statement error zone configuration for database initial_multiregion_db has field "num_replicas" set which will be overwritten when setting the the initial PRIMARY REGION\nHINT: discard the zone config using CONFIGURE ZONE DISCARD before continuing
+ALTER DATABASE initial_multiregion_db SET PRIMARY REGION "us-east-1"
+
+statement ok
+ALTER DATABASE initial_multiregion_db CONFIGURE ZONE DISCARD;
+ALTER DATABASE initial_multiregion_db SET PRIMARY REGION "us-east-1"

pkg/col/coldata/BUILD.bazel

@@ -19,6 +19,7 @@ go_library(
         "//pkg/sql/types",
         "//pkg/util",
         "//pkg/util/duration",
+        "//pkg/util/uuid",
         "@com_github_cockroachdb_apd_v2//:apd",
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_stretchr_testify//require",

pkg/col/coldata/bytes.go

@@ -45,12 +45,24 @@ const BytesInitialAllocationFactor = 64
 // NewBytes returns a Bytes struct with enough capacity for n zero-length
 // []byte values. It is legal to call Set on the returned Bytes at this point,
 // but Get is undefined until at least one element is Set.
+// BytesInitialAllocationFactor number of bytes are allocated initially for each
+// []byte element.
 func NewBytes(n int) *Bytes {
+	return NewBytesWithAvgLength(n, BytesInitialAllocationFactor)
+}
+
+// NewBytesWithAvgLength returns a Bytes struct with enough capacity for n
+// []byte values with the average length of avgElementLength. It is legal to
+// call Set on the returned Bytes at this point, but Get is undefined until at
+// least one element is Set.
+// - avgElementLength determines the average length of a single []byte element
+// that will be added to this Bytes.
+func NewBytesWithAvgLength(n int, avgElementLength int) *Bytes {
 	return &Bytes{
 		// Given that the []byte slices are of variable length, we multiply the
 		// number of elements by some constant factor.
 		// TODO(asubiotto): Make this tunable.
-		data:    make([]byte, 0, n*BytesInitialAllocationFactor),
+		data:    make([]byte, 0, n*avgElementLength),
 		offsets: make([]int32, n+1),
 	}
 }

pkg/col/coldata/vec.go

@@ -15,6 +15,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/col/typeconv"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
+	"github.com/cockroachdb/cockroach/pkg/util/uuid"
 )
 
 // Column is an interface that represents a raw array of a Go native type.
@@ -162,6 +163,9 @@ func (cf *defaultColumnFactory) MakeColumn(t *types.T, length int) Column {
 	case types.BoolFamily:
 		return make(Bools, length)
 	case types.BytesFamily:
+		if t.Family() == types.UuidFamily {
+			return NewBytesWithAvgLength(length, uuid.Size)
+		}
 		return NewBytes(length)
 	case types.IntFamily:
 		switch t.Width() {

pkg/config/zonepb/zone.go

@@ -611,8 +611,6 @@ type DiffWithZoneMismatch struct {
 	// PartitionName represents a subzone with a mismatching partitionName.
 	PartitionName string
 
-	// NOTE: only one of the below fields is set.
-
 	// IsMissingSubzone indicates a subzone is missing.
 	IsMissingSubzone bool
 	// IsExtraSubzone indicates we have an extraneous subzone.
@@ -782,6 +780,7 @@ func (z *ZoneConfig) DiffWithZone(
 					IndexID:        s.IndexID,
 					PartitionName:  s.PartitionName,
 					IsExtraSubzone: true,
+					Field:          subzoneMismatch.Field,
 				}, nil
 			}
 			continue
@@ -822,6 +821,7 @@ func (z *ZoneConfig) DiffWithZone(
 				IndexID:          o.IndexID,
 				PartitionName:    o.PartitionName,
 				IsMissingSubzone: true,
+				Field:            subzoneMismatch.Field,
 			}, nil
 		}
 	}

pkg/sql/alter_database.go

@@ -332,7 +332,11 @@ func (p *planner) AlterDatabaseDropRegion(
 		}
 		if len(regions) != 1 {
 			return nil, errors.WithHintf(
-				errors.Newf("cannot drop region %q", dbDesc.RegionConfig.PrimaryRegion),
+				pgerror.Newf(
+					pgcode.InvalidDatabaseDefinition,
+					"cannot drop region %q",
+					dbDesc.RegionConfig.PrimaryRegion,
+				),
 				"You must designate another region as the primary region using "+
 					"ALTER DATABASE %s PRIMARY REGION <region name> or remove all other regions before "+
 					"attempting to drop region %q", dbDesc.GetName(), n.Region,
@@ -425,7 +429,7 @@ func (p *planner) checkPrivilegesForMultiRegionOp(
 func (p *planner) checkPrivilegesForRepartitioningRegionalByRowTables(
 	ctx context.Context, dbDesc *dbdesc.Immutable,
 ) error {
-	return p.forEachTableInMultiRegionDatabase(ctx, dbDesc,
+	return p.forEachMutableTableInDatabase(ctx, dbDesc,
 		func(ctx context.Context, tbDesc *tabledesc.Mutable) error {
 			if tbDesc.IsLocalityRegionalByRow() {
 				err := p.checkPrivilegesForMultiRegionOp(ctx, tbDesc)
@@ -455,7 +459,7 @@ func removeLocalityConfigFromAllTablesInDB(
 		)
 	}
 	b := p.Txn().NewBatch()
-	if err := p.forEachTableInMultiRegionDatabase(ctx, desc, func(ctx context.Context, tbDesc *tabledesc.Mutable) error {
+	if err := p.forEachMutableTableInDatabase(ctx, desc, func(ctx context.Context, tbDesc *tabledesc.Mutable) error {
 		// The user must either be an admin or have the requisite privileges.
 		if err := p.checkPrivilegesForMultiRegionOp(ctx, tbDesc); err != nil {
 			return err
@@ -694,7 +698,7 @@ func addDefaultLocalityConfigToAllTables(
 		)
 	}
 	b := p.Txn().NewBatch()
-	if err := p.forEachTableInMultiRegionDatabase(ctx, dbDesc, func(ctx context.Context, tbDesc *tabledesc.Mutable) error {
+	if err := p.forEachMutableTableInDatabase(ctx, dbDesc, func(ctx context.Context, tbDesc *tabledesc.Mutable) error {
 		if err := p.checkPrivilegesForMultiRegionOp(ctx, tbDesc); err != nil {
 			return err
 		}
@@ -756,7 +760,6 @@ func checkCanConvertTableToMultiRegion(
 			)
 		}
 	}
-	// TODO(#57668): check zone configurations are not set here
 	return nil
 }
 
@@ -775,6 +778,15 @@ func (n *alterDatabasePrimaryRegionNode) setInitialPrimaryRegion(params runParam
 		return err
 	}
 
+	// Check we are writing valid zone configurations.
+	if err := params.p.validateAllMultiRegionZoneConfigsInDatabase(
+		params.ctx,
+		&n.desc.Immutable,
+		&zoneConfigForMultiRegionValidatorSetInitialRegion{},
+	); err != nil {
+		return err
+	}
+
 	// Set the region config on the database descriptor.
 	if err := n.desc.SetInitialMultiRegionConfig(regionConfig); err != nil {
 		return err

pkg/sql/colmem/BUILD.bazel

@@ -13,6 +13,7 @@ go_library(
         "//pkg/sql/types",
         "//pkg/util/duration",
         "//pkg/util/mon",
+        "//pkg/util/uuid",
         "@com_github_cockroachdb_apd_v2//:apd",
         "@com_github_cockroachdb_errors//:errors",
     ],

pkg/sql/colmem/allocator.go

@@ -23,6 +23,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
 	"github.com/cockroachdb/cockroach/pkg/util/duration"
 	"github.com/cockroachdb/cockroach/pkg/util/mon"
+	"github.com/cockroachdb/cockroach/pkg/util/uuid"
 	"github.com/cockroachdb/errors"
 )
 
@@ -370,12 +371,19 @@ func EstimateBatchSizeBytes(vecTypes []*types.T, batchLength int) int {
 	// (excluding any Bytes vectors, those are tracked separately).
 	acc := 0
 	numBytesVectors := 0
+	// We will track Uuid vectors separately because they use smaller initial
+	// allocation factor.
+	numUUIDVectors := 0
 	for _, t := range vecTypes {
 		switch typeconv.TypeFamilyToCanonicalTypeFamily(t.Family()) {
 		case types.BoolFamily:
 			acc += sizeOfBool
 		case types.BytesFamily:
-			numBytesVectors++
+			if t.Family() == types.UuidFamily {
+				numUUIDVectors++
+			} else {
+				numBytesVectors++
+			}
 		case types.IntFamily:
 			switch t.Width() {
 			case 16:
@@ -416,15 +424,20 @@ func EstimateBatchSizeBytes(vecTypes []*types.T, batchLength int) int {
 			colexecerror.InternalError(errors.AssertionFailedf("unhandled type %s", t))
 		}
 	}
-	// For byte arrays, we initially allocate BytesInitialAllocationFactor
-	// number of bytes (plus an int32 for the offset) for each row, so we use
-	// the sum of two values as the estimate. However, later, the exact
-	// memory footprint will be used: whenever a modification of Bytes takes
-	// place, the Allocator will measure the old footprint and the updated
-	// one and will update the memory account accordingly. We also account for
-	// the overhead and for the additional offset value that are needed for
-	// Bytes vectors (to be in line with coldata.Bytes.Size() method).
-	bytesVectorsSize := numBytesVectors * (int(coldata.FlatBytesOverhead) +
-		coldata.BytesInitialAllocationFactor*batchLength + sizeOfInt32*(batchLength+1))
+	// For byte arrays, we initially allocate a constant number of bytes (plus
+	// an int32 for the offset) for each row, so we use the sum of two values as
+	// the estimate. However, later, the exact memory footprint will be used:
+	// whenever a modification of Bytes takes place, the Allocator will measure
+	// the old footprint and the updated one and will update the memory account
+	// accordingly. We also account for the overhead and for the additional
+	// offset value that are needed for Bytes vectors (to be in line with
+	// coldata.Bytes.Size() method).
+	var bytesVectorsSize int
+	// Add the overhead.
+	bytesVectorsSize += (numBytesVectors + numUUIDVectors) * (int(coldata.FlatBytesOverhead))
+	// Add the data for both Bytes and Uuids.
+	bytesVectorsSize += (numBytesVectors*coldata.BytesInitialAllocationFactor + numUUIDVectors*uuid.Size) * batchLength
+	// Add the offsets.
+	bytesVectorsSize += (numBytesVectors + numUUIDVectors) * sizeOfInt32 * (batchLength + 1)
 	return acc*batchLength + bytesVectorsSize
 }

pkg/sql/database.go

@@ -18,6 +18,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -94,3 +95,30 @@ func (p *planner) writeDatabaseChangeToBatch(
 		b,
 	)
 }
+
+// forEachMutableTableInDatabase calls the given function on every table
+// descriptor inside the given database. Tables that have been
+// dropped are skipped.
+func (p *planner) forEachMutableTableInDatabase(
+	ctx context.Context,
+	dbDesc *dbdesc.Immutable,
+	fn func(ctx context.Context, tbDesc *tabledesc.Mutable) error,
+) error {
+	allDescs, err := p.Descriptors().GetAllDescriptors(ctx, p.txn)
+	if err != nil {
+		return err
+	}
+
+	lCtx := newInternalLookupCtx(ctx, allDescs, dbDesc, nil /* fallback */)
+	for _, tbID := range lCtx.tbIDs {
+		desc := lCtx.tbDescs[tbID]
+		if desc.Dropped() {
+			continue
+		}
+		mutable := tabledesc.NewBuilder(desc.TableDesc()).BuildExistingMutableTable()
+		if err := fn(ctx, mutable); err != nil {
+			return err
+		}
+	}
+	return nil
+}

pkg/sql/logictest/testdata/logic_test/set

@@ -292,12 +292,12 @@ SET statement_timeout = '0ms'
 # Test that statement_timeout can be set with an interval string, defaulting to
 # milliseconds as a unit.
 statement ok
-SET statement_timeout = '100'
+SET statement_timeout = '10000'
 
 query T
 SHOW statement_timeout
 ----
-100
+10000
 
 # Set the statement timeout to something absurdly small, so that no query would
 # presumably be able to go through. It should still be possible to get out of