Merge #118575 #118679 #118680 #118769

118575: schemachanger: Prep work for ALTER DATABASE ... CONFIGURE ZONE r=Xiang-Gu a=Xiang-Gu

This PR contains three preparation commits for supporting zone configs in DSC. They should be rather uncontroversial and easy to review. See commit message for details.

Informs: #117574
Epic: CRDB-31473

118679: sql: minor improvements around estimated row count r=yuzefovich a=yuzefovich

This commit makes it so that we use the ceiling of the estimated row count in scans and aggregation. It also fixes recently-introduced bug that could cause integer overflow in the agg alloc code.

Fixes: #118724.

Epic: None

Release note: None

118680: ui: change start and end values on stmt details charts r=maryliag a=maryliag

Previously, we were using the data values to define the start and end date of charts, which could cause confusion, since users might think that is a bug of now showing data, when instead there is no data to show.
This commit forces the start and end of the chart to be the selected period, this way we always show the correct period even if we don't have data to show.

Fixes #102214

https://www.loom.com/share/3f463d82407b4ab186a5b0c1e1ab2369

Release note (ui change): On Statement Details page always show the entire selected period, instead of just the period that had data.

118769: logictest: skip `distsql_tenant_locality` under duress r=yuzefovich,michae2 a=rickystewart

Flaky test: see #118627

Epic: None
Release note: None

Co-authored-by: Xiang Gu <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: maryliag <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>

pkg/cmd/generate-logictest/templates.go

@@ -72,6 +72,9 @@ func runCCLLogicTest(t *testing.T, file string) {
 {{- if .ExecBuildLogicTest -}}
 func runExecBuildLogicTest(t *testing.T, file string) {
 	defer sql.TestingOverrideExplainEnvVersion("CockroachDB execbuilder test version")()
+	if file == "distsql_tenant_locality" {
+		skip.UnderDuressWithIssue(t, 118627)
+	}
 	skip.UnderDeadlock(t, "times out and/or hangs")
 	serverArgs := logictest.TestServerArgs{
 		DisableWorkmemRandomization: true,{{ if .ForceProductionValues }}

pkg/sql/alter_database.go

@@ -28,6 +28,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/multiregion"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/zone"
 	"github.com/cockroachdb/cockroach/pkg/sql/decodeusername"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
@@ -2217,7 +2218,7 @@ type alterDatabaseSetZoneConfigExtensionNode struct {
 	n          *tree.AlterDatabaseSetZoneConfigExtension
 	desc       *dbdesc.Mutable
 	yamlConfig tree.TypedExpr
-	options    map[tree.Name]optionValue
+	options    map[tree.Name]zone.OptionValue
 }
 
 // AlterDatabaseSetZoneConfigExtension transforms a

pkg/sql/authorization.go

@@ -1103,10 +1103,7 @@ func insufficientPrivilegeError(
 			"user %s does not have %s system privilege",
 			user, kind)
 	}
-
-	return pgerror.Newf(pgcode.InsufficientPrivilege,
-		"user %s does not have %s privilege on %s %s",
-		user, kind, objTypeStr, object.GetName())
+	return sqlerrors.NewInsufficientPrivilegeOnDescriptorError(user, []privilege.Kind{kind}, objTypeStr, object.GetName())
 }
 
 // IsInsufficientPrivilegeError returns true if the error is a pgerror

pkg/sql/catalog/zone/BUILD.bazel

@@ -2,12 +2,23 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library")
 
 go_library(
     name = "zone",
-    srcs = ["zone_config.go"],
+    srcs = [
+        "zone_config.go",
+        "zones.go",
+    ],
     importpath = "github.com/cockroachdb/cockroach/pkg/sql/catalog/zone",
     visibility = ["//visibility:public"],
     deps = [
+        "//pkg/base",
+        "//pkg/config",
         "//pkg/config/zonepb",
+        "//pkg/settings/cluster",
         "//pkg/sql/catalog",
+        "//pkg/sql/sem/tree",
+        "//pkg/sql/types",
         "//pkg/util/protoutil",
+        "@com_github_cockroachdb_errors//:errors",
+        "@com_github_gogo_protobuf//proto",
+        "@in_gopkg_yaml_v2//:yaml_v2",
     ],
 )

pkg/sql/catalog/zone/zones.go

@@ -0,0 +1,148 @@
+// Copyright 2024 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package zone
+
+import (
+	"context"
+	"sort"
+
+	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/config"
+	"github.com/cockroachdb/cockroach/pkg/config/zonepb"
+	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+	"github.com/cockroachdb/cockroach/pkg/sql/types"
+	"github.com/cockroachdb/errors"
+	"github.com/gogo/protobuf/proto"
+	yaml "gopkg.in/yaml.v2"
+)
+
+type OptionValue struct {
+	InheritValue  bool
+	ExplicitValue tree.TypedExpr
+}
+
+// ZoneConfigOption describes a Field one can set on a ZoneConfig.
+type ZoneConfigOption struct {
+	Field        config.Field
+	RequiredType *types.T
+	Setter       func(*zonepb.ZoneConfig, tree.Datum)
+	CheckAllowed func(context.Context, *cluster.Settings, tree.Datum) error
+}
+
+func loadYAML(dst interface{}, yamlString string) {
+	if err := yaml.UnmarshalStrict([]byte(yamlString), dst); err != nil {
+		panic(err)
+	}
+}
+
+// SupportedZoneConfigOptions indicates how to translate SQL variable
+// assignments in ALTER CONFIGURE ZONE to assignments to the member
+// fields of zonepb.ZoneConfig.
+var SupportedZoneConfigOptions map[tree.Name]ZoneConfigOption
+
+// ZoneOptionKeys contains the keys from suportedZoneConfigOptions in
+// deterministic order. Needed to make the event log output
+// deterministic.
+var ZoneOptionKeys []string
+
+func init() {
+	opts := []ZoneConfigOption{
+		{
+			Field:        config.RangeMinBytes,
+			RequiredType: types.Int,
+			Setter:       func(c *zonepb.ZoneConfig, d tree.Datum) { c.RangeMinBytes = proto.Int64(int64(tree.MustBeDInt(d))) },
+		},
+		{
+			Field:        config.RangeMaxBytes,
+			RequiredType: types.Int,
+			Setter:       func(c *zonepb.ZoneConfig, d tree.Datum) { c.RangeMaxBytes = proto.Int64(int64(tree.MustBeDInt(d))) },
+		},
+		{
+			Field:        config.GlobalReads,
+			RequiredType: types.Bool,
+			Setter:       func(c *zonepb.ZoneConfig, d tree.Datum) { c.GlobalReads = proto.Bool(bool(tree.MustBeDBool(d))) },
+			CheckAllowed: func(ctx context.Context, settings *cluster.Settings, d tree.Datum) error {
+				if !tree.MustBeDBool(d) {
+					// Always allow the value to be unset.
+					return nil
+				}
+				return base.CheckEnterpriseEnabled(
+					settings,
+					"global_reads",
+				)
+			},
+		},
+		{
+			Field:        config.NumReplicas,
+			RequiredType: types.Int,
+			Setter:       func(c *zonepb.ZoneConfig, d tree.Datum) { c.NumReplicas = proto.Int32(int32(tree.MustBeDInt(d))) },
+		},
+		{
+			Field:        config.NumVoters,
+			RequiredType: types.Int,
+			Setter:       func(c *zonepb.ZoneConfig, d tree.Datum) { c.NumVoters = proto.Int32(int32(tree.MustBeDInt(d))) },
+		},
+		{
+			Field:        config.GCTTL,
+			RequiredType: types.Int,
+			Setter: func(c *zonepb.ZoneConfig, d tree.Datum) {
+				c.GC = &zonepb.GCPolicy{TTLSeconds: int32(tree.MustBeDInt(d))}
+			},
+		},
+		{
+			Field:        config.Constraints,
+			RequiredType: types.String,
+			Setter: func(c *zonepb.ZoneConfig, d tree.Datum) {
+				constraintsList := zonepb.ConstraintsList{
+					Constraints: c.Constraints,
+					Inherited:   c.InheritedConstraints,
+				}
+				loadYAML(&constraintsList, string(tree.MustBeDString(d)))
+				c.Constraints = constraintsList.Constraints
+				c.InheritedConstraints = false
+			},
+		},
+		{
+			Field:        config.VoterConstraints,
+			RequiredType: types.String,
+			Setter: func(c *zonepb.ZoneConfig, d tree.Datum) {
+				voterConstraintsList := zonepb.ConstraintsList{
+					Constraints: c.VoterConstraints,
+					Inherited:   c.InheritedVoterConstraints(),
+				}
+				loadYAML(&voterConstraintsList, string(tree.MustBeDString(d)))
+				c.VoterConstraints = voterConstraintsList.Constraints
+				c.NullVoterConstraintsIsEmpty = true
+			},
+		},
+		{
+			Field:        config.LeasePreferences,
+			RequiredType: types.String,
+			Setter: func(c *zonepb.ZoneConfig, d tree.Datum) {
+				loadYAML(&c.LeasePreferences, string(tree.MustBeDString(d)))
+				c.InheritedLeasePreferences = false
+			},
+		},
+	}
+	SupportedZoneConfigOptions = make(map[tree.Name]ZoneConfigOption, len(opts))
+	ZoneOptionKeys = make([]string, len(opts))
+	for i, opt := range opts {
+		name := opt.Field.String()
+		key := tree.Name(name)
+		if _, exists := SupportedZoneConfigOptions[key]; exists {
+			panic(errors.AssertionFailedf("duplicate entry for key %s", name))
+		}
+		SupportedZoneConfigOptions[key] = opt
+		ZoneOptionKeys[i] = name
+	}
+	sort.Strings(ZoneOptionKeys)
+}

pkg/sql/colexec/hash_aggregator.go

@@ -201,9 +201,12 @@ func NewHashAggregator(
 ) colexecop.ResettableOperator {
 	initialAllocSize, maxAllocSize := int64(1), int64(hashAggregatorAllocSize)
 	if args.EstimatedRowCount != 0 {
-		initialAllocSize = int64(args.EstimatedRowCount)
-		if initialAllocSize > maxAllocSize {
+		// Use uint64s for comparison to prevent overflow in case
+		// args.EstimatedRowCount is larger than MaxInt64.
+		if args.EstimatedRowCount >= uint64(maxAllocSize) {
 			initialAllocSize = maxAllocSize
+		} else {
+			initialAllocSize = int64(args.EstimatedRowCount)
 		}
 	}
 	aggFnsAlloc, inputArgsConverter, toClose, err := colexecagg.NewAggregateFuncsAlloc(

pkg/sql/distsql_spec_exec_factory.go

@@ -286,7 +286,7 @@ func (e *distSQLSpecExecFactory) ConstructScan(
 			spans:             spans,
 			reverse:           params.Reverse,
 			parallelize:       params.Parallelize,
-			estimatedRowCount: uint64(params.EstimatedRowCount),
+			estimatedRowCount: params.EstimatedRowCount,
 			reqOrdering:       ReqOrdering(reqOrdering),
 		},
 	)

pkg/sql/logictest/testdata/logic_test/grant_sequence

@@ -12,7 +12,7 @@ SET ROLE readwrite
 statement error pq: user readwrite does not have SELECT privilege on relation a
 SELECT * FROM a;
 
-statement error pq: nextval\(\): user readwrite does not have UPDATE or USAGE privilege on relation a
+statement error pq: nextval\(\): user readwrite does not have USAGE or UPDATE privilege on relation a
 SELECT nextval('a')
 
 statement ok

pkg/sql/logictest/testdata/logic_test/sequences

@@ -932,7 +932,7 @@ user testuser
 statement error pq: user testuser does not have SELECT privilege on relation priv_test
 SELECT * FROM priv_test
 
-statement error pq: nextval\(\): user testuser does not have UPDATE or USAGE privilege on relation priv_test
+statement error pq: nextval\(\): user testuser does not have USAGE or UPDATE privilege on relation priv_test
 SELECT nextval('priv_test')
 
 statement error pq: setval\(\): user testuser does not have UPDATE privilege on relation priv_test

pkg/sql/opt/exec/execbuilder/relational.go

@@ -646,9 +646,9 @@ func (b *Builder) scanParams(
 	// statement or was stored in the query cache, the column stats would have
 	// been removed by DetachMemo. Update that function if the column stats are
 	// needed here in the future.
-	var rowCount float64
+	var rowCount uint64
 	if relProps.Statistics().Available {
-		rowCount = relProps.Statistics().RowCount
+		rowCount = uint64(math.Ceil(relProps.Statistics().RowCount))
 	}
 
 	if scan.PartitionConstrainedScan {
@@ -1643,7 +1643,7 @@ func (b *Builder) buildGroupBy(groupBy memo.RelExpr) (execPlan, error) {
 		orderType := exec.GroupingOrderType(groupBy.GroupingOrderType(&groupBy.RequiredPhysical().Ordering))
 		var rowCount uint64
 		if relProps := groupBy.Relational(); relProps.Statistics().Available {
-			rowCount = uint64(relProps.Statistics().RowCount)
+			rowCount = uint64(math.Ceil(relProps.Statistics().RowCount))
 		}
 		ep.root, err = b.factory.ConstructGroupBy(
 			input.root, groupingColIdx, groupingColOrder, aggInfos, reqOrdering, orderType, rowCount,

pkg/sql/opt/exec/execbuilder/testdata/distsql_tenant_locality

@@ -40,12 +40,12 @@ statement ok
 SELECT * FROM t
 
 # Check sql instance locality in the secondary tenant.
-query ITB retry
-SELECT id, locality, crdb_internal.sql_liveness_is_alive(session_id) FROM system.sql_instances WHERE locality IS NOT NULL ORDER BY id
+query IT
+SELECT id, locality FROM system.sql_instances WHERE locality IS NOT NULL ORDER BY id
 ----
-1  {"Tiers": "region=test"}   true
-2  {"Tiers": "region=test1"}  true
-3  {"Tiers": "region=test2"}  true
+1  {"Tiers": "region=test"}
+2  {"Tiers": "region=test1"}
+3  {"Tiers": "region=test2"}
 
 # Ensure that we plan TableReaders in the regions according to the leaseholder
 # of each range, namely we want