Merge #73802 #73803

73802: sql: use 'NONVOTERS' as keyword for 'RELOCATE', not 'NON_VOTERS' r=rafiss,RaduBerinde a=knz

Informs #73315.
Fixes #63219.

The idiom for positional keywords in SQL is to either use words
separated by spaces (e.g. NOT NULL), or to concatenate the words
together (ISERROR, NOLOGIN, LINESTRING).

Release note (sql change): In the experimental RELOCATE syntax
forms, the positional keyword that indicates that the statement
should move non-voter replicas is now spelled `NONVOTERS`, instead
of `NON_VOTERS` previously.

Release note (sql change): The inline help for the ALTER
statements now mentions the RELOCATE syntax.

73803: sql: use a 3-valued type instead of 2 booleans for relocate statements r=RaduBerinde a=knz

Informs #73315. 
First 3 commits from #73802 (reviewers, focus on the last commit)

By using a 3-valued types, we greatly simplify the code to print out
statement types, and also clarify intent in all the conditionals that
depend on the relocation mode.

Co-authored-by: Raphael 'kena' Poss <[email protected]>

docs/generated/sql/bnf/BUILD.bazel

@@ -18,7 +18,6 @@ FILES = [
     "alter_index_stmt",
     "alter_partition_stmt",
     "alter_primary_key",
-    "alter_range_relocate_lease_stmt",
     "alter_range_relocate_stmt",
     "alter_range_stmt",
     "alter_rename_view_stmt",

docs/generated/sql/bnf/alter_range_relocate_stmt.bnf

@@ -1,5 +1,5 @@
 alter_range_relocate_stmt ::=
-	'ALTER' 'RANGE' relocate_kw voters_kw 'FROM' iconst64 'TO' iconst64 'FOR' select_stmt
-	| 'ALTER' 'RANGE' iconst64 relocate_kw voters_kw 'FROM' iconst64 'TO' iconst64
-	| 'ALTER' 'RANGE' relocate_kw 'NON_VOTERS' 'FROM' iconst64 'TO' iconst64 'FOR' select_stmt
-	| 'ALTER' 'RANGE' iconst64 relocate_kw 'NON_VOTERS' 'FROM' iconst64 'TO' iconst64
+	'ALTER' 'RANGE' relocate_kw 'LEASE' 'TO' iconst64 'FOR' select_stmt
+	| 'ALTER' 'RANGE' iconst64 relocate_kw 'LEASE' 'TO' iconst64
+	| 'ALTER' 'RANGE' relocate_kw relocate_subject_nonlease 'FROM' iconst64 'TO' iconst64 'FOR' select_stmt
+	| 'ALTER' 'RANGE' iconst64 relocate_kw relocate_subject_nonlease 'FROM' iconst64 'TO' iconst64

docs/generated/sql/bnf/alter_range_stmt.bnf

@@ -1,4 +1,3 @@
 alter_range_stmt ::=
 	alter_zone_range_stmt
-	| alter_range_relocate_lease_stmt
 	| alter_range_relocate_stmt

docs/generated/sql/bnf/stmt_block.bnf

@@ -1038,7 +1038,7 @@ unreserved_keyword ::=
 	| 'NOCONTROLJOB'
 	| 'NOLOGIN'
 	| 'NOMODIFYCLUSTERSETTING'
-	| 'NON_VOTERS'
+	| 'NONVOTERS'
 	| 'NOVIEWACTIVITY'
 	| 'NOWAIT'
 	| 'NULLS'
@@ -1354,7 +1354,6 @@ alter_database_stmt ::=
 
 alter_range_stmt ::=
 	alter_zone_range_stmt
-	| alter_range_relocate_lease_stmt
 	| alter_range_relocate_stmt
 
 alter_partition_stmt ::=
@@ -1846,15 +1845,11 @@ alter_database_primary_region_stmt ::=
 alter_zone_range_stmt ::=
 	'ALTER' 'RANGE' zone_name set_zone_config
 
-alter_range_relocate_lease_stmt ::=
+alter_range_relocate_stmt ::=
 	'ALTER' 'RANGE' relocate_kw 'LEASE' 'TO' iconst64 'FOR' select_stmt
 	| 'ALTER' 'RANGE' iconst64 relocate_kw 'LEASE' 'TO' iconst64
-
-alter_range_relocate_stmt ::=
-	'ALTER' 'RANGE' relocate_kw voters_kw 'FROM' iconst64 'TO' iconst64 'FOR' select_stmt
-	| 'ALTER' 'RANGE' iconst64 relocate_kw voters_kw 'FROM' iconst64 'TO' iconst64
-	| 'ALTER' 'RANGE' relocate_kw 'NON_VOTERS' 'FROM' iconst64 'TO' iconst64 'FOR' select_stmt
-	| 'ALTER' 'RANGE' iconst64 relocate_kw 'NON_VOTERS' 'FROM' iconst64 'TO' iconst64
+	| 'ALTER' 'RANGE' relocate_kw relocate_subject_nonlease 'FROM' iconst64 'TO' iconst64 'FOR' select_stmt
+	| 'ALTER' 'RANGE' iconst64 relocate_kw relocate_subject_nonlease 'FROM' iconst64 'TO' iconst64
 
 alter_zone_partition_stmt ::=
 	'ALTER' 'PARTITION' partition_name 'OF' 'TABLE' table_name set_zone_config
@@ -2336,9 +2331,10 @@ relocate_kw ::=
 	| 'EXPERIMENTAL_RELOCATE'
 	| 'RELOCATE'
 
-voters_kw ::=
+relocate_subject_nonlease ::=
 	'VOTERS'
 	| 
+	| 'NONVOTERS'
 
 alter_default_privileges_target_object ::=
 	'TABLES'

pkg/ccl/kvccl/kvfollowerreadsccl/followerreads_test.go

@@ -642,7 +642,7 @@ func TestFollowerReadsWithStaleDescriptor(t *testing.T) {
 	// Remove the follower and add a new non-voter to n3. n2 will no longer have a
 	// replica.
 	n1.Exec(t, `ALTER TABLE test EXPERIMENTAL_RELOCATE VOTERS VALUES (ARRAY[1], 1)`)
-	n1.Exec(t, `ALTER TABLE test EXPERIMENTAL_RELOCATE NON_VOTERS VALUES (ARRAY[3], 1)`)
+	n1.Exec(t, `ALTER TABLE test EXPERIMENTAL_RELOCATE NONVOTERS VALUES (ARRAY[3], 1)`)
 
 	// Execute the query again and assert the cache is updated. This query will
 	// not be executed as a follower read since it attempts to use n2 which

pkg/sql/distsql_spec_exec_factory.go

@@ -1070,13 +1070,13 @@ func (e *distSQLSpecExecFactory) ConstructAlterTableUnsplitAll(index cat.Index)
 }
 
 func (e *distSQLSpecExecFactory) ConstructAlterTableRelocate(
-	index cat.Index, input exec.Node, relocateLease bool, relocateNonVoters bool,
+	index cat.Index, input exec.Node, relocateSubject tree.RelocateSubject,
 ) (exec.Node, error) {
 	return nil, unimplemented.NewWithIssue(47473, "experimental opt-driven distsql planning: alter table relocate")
 }
 
 func (e *distSQLSpecExecFactory) ConstructAlterRangeRelocate(
-	input exec.Node, relocateLease bool, relocateNonVoters bool, toStoreID int64, fromStoreID int64,
+	input exec.Node, relocateSubject tree.RelocateSubject, toStoreID int64, fromStoreID int64,
 ) (exec.Node, error) {
 	return nil, unimplemented.NewWithIssue(47473, "experimental opt-driven distsql planning: alter range relocate")
 }

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

@@ -222,8 +222,7 @@ func (b *Builder) buildAlterTableRelocate(relocate *memo.AlterTableRelocateExpr)
 	node, err := b.factory.ConstructAlterTableRelocate(
 		table.Index(relocate.Index),
 		input.root,
-		relocate.RelocateLease,
-		relocate.RelocateNonVoters,
+		relocate.SubjectReplicas,
 	)
 	if err != nil {
 		return execPlan{}, err
@@ -238,8 +237,7 @@ func (b *Builder) buildAlterRangeRelocate(relocate *memo.AlterRangeRelocateExpr)
 	}
 	node, err := b.factory.ConstructAlterRangeRelocate(
 		input.root,
-		relocate.RelocateLease,
-		relocate.RelocateNonVoters,
+		relocate.SubjectReplicas,
 		relocate.ToStoreID,
 		relocate.FromStoreID,
 	)

pkg/sql/opt/exec/factory.opt

@@ -658,8 +658,7 @@ define AlterTableUnsplitAll {
 define AlterTableRelocate {
     Index cat.Index
     input exec.Node
-    relocateLease bool
-    relocateNonVoters bool
+    subjectReplicas tree.RelocateSubject
 }
 
 # Buffer passes through the input rows but also saves them in a buffer, which
@@ -736,8 +735,7 @@ define Export {
 # AlterTableRelocate implements ALTER RANGE RELOCATE.
 define AlterRangeRelocate {
     input exec.Node
-    relocateLease bool
-    relocateNonVoters bool
+    subjectReplicas tree.RelocateSubject
     toStoreID int64
     fromStoreID int64
 }

pkg/sql/opt/memo/expr_format.go

@@ -1502,11 +1502,12 @@ func FormatPrivate(f *ExprFmtCtx, private interface{}, physProps *physical.Requi
 
 	case *AlterTableRelocatePrivate:
 		FormatPrivate(f, &t.AlterTableSplitPrivate, nil)
-		if t.RelocateLease {
+		switch t.SubjectReplicas {
+		case tree.RelocateLease:
 			f.Buffer.WriteString(" [lease]")
-		} else if t.RelocateNonVoters {
+		case tree.RelocateNonVoters:
 			f.Buffer.WriteString(" [non-voters]")
-		} else {
+		case tree.RelocateVoters:
 			f.Buffer.WriteString(" [voters]")
 		}
 

pkg/sql/opt/memo/interner.go

@@ -568,6 +568,10 @@ func (h *hasher) HashIndexOrdinal(val cat.IndexOrdinal) {
 	h.HashInt(val)
 }
 
+func (h *hasher) HashRelocateSubject(val tree.RelocateSubject) {
+	h.HashUint64(uint64(val))
+}
+
 func (h *hasher) HashIndexOrdinals(val cat.IndexOrdinals) {
 	hash := h.hash
 	for _, ord := range val {
@@ -978,6 +982,10 @@ func (h *hasher) IsIndexOrdinalEqual(l, r cat.IndexOrdinal) bool {
 	return l == r
 }
 
+func (h *hasher) IsRelocateSubjectEqual(l, r tree.RelocateSubject) bool {
+	return l == r
+}
+
 func (h *hasher) IsIndexOrdinalsEqual(l, r cat.IndexOrdinals) bool {
 	if len(l) != len(r) {
 		return false

pkg/sql/opt/ops/statement.opt

@@ -191,8 +191,7 @@ define AlterTableRelocate {
 
 [Private]
 define AlterTableRelocatePrivate {
-    RelocateLease bool
-    RelocateNonVoters bool
+    SubjectReplicas RelocateSubject
     _ AlterTableSplitPrivate
 }
 
@@ -303,8 +302,7 @@ define AlterRangeRelocate {
 
 [Private]
 define AlterRangeRelocatePrivate {
-    RelocateLease bool
-    RelocateNonVoters bool
+    SubjectReplicas RelocateSubject
     ToStoreID int64
     FromStoreID int64
 

pkg/sql/opt/optbuilder/alter_range.go

@@ -33,10 +33,7 @@ func (b *Builder) buildAlterRangeRelocate(
 	outScope = inScope.push()
 	b.synthesizeResultColumns(outScope, colinfo.AlterTableRelocateColumns)
 
-	cmdName := "RELOCATE"
-	if relocate.RelocateLease {
-		cmdName += " LEASE"
-	}
+	cmdName := "RELOCATE " + relocate.SubjectReplicas.String()
 	colNames := []string{"range ids"}
 	colTypes := []*types.T{types.Int}
 
@@ -51,12 +48,11 @@ func (b *Builder) buildAlterRangeRelocate(
 	outScope.expr = b.factory.ConstructAlterRangeRelocate(
 		inputScope.expr,
 		&memo.AlterRangeRelocatePrivate{
-			RelocateLease:     relocate.RelocateLease,
-			RelocateNonVoters: relocate.RelocateNonVoters,
-			ToStoreID:         relocate.ToStoreID,
-			FromStoreID:       relocate.FromStoreID,
-			Columns:           colsToColList(outScope.cols),
-			Props:             physical.MinRequired,
+			SubjectReplicas: relocate.SubjectReplicas,
+			ToStoreID:       relocate.ToStoreID,
+			FromStoreID:     relocate.FromStoreID,
+			Columns:         colsToColList(outScope.cols),
+			Props:           physical.MinRequired,
 		},
 	)
 	return outScope

pkg/sql/opt/optbuilder/alter_table.go

@@ -163,8 +163,8 @@ func (b *Builder) buildAlterTableRelocate(
 
 	// The first column is the target leaseholder or the relocation array,
 	// depending on variant.
-	cmdName := "EXPERIMENTAL_RELOCATE"
-	if relocate.RelocateLease {
+	cmdName := "RELOCATE"
+	if relocate.SubjectReplicas == tree.RelocateLease {
 		cmdName += " LEASE"
 		colNames = append([]string{"target leaseholder"}, colNames...)
 		colTypes = append([]*types.T{types.Int}, colTypes...)
@@ -181,8 +181,7 @@ func (b *Builder) buildAlterTableRelocate(
 	outScope.expr = b.factory.ConstructAlterTableRelocate(
 		inputScope.expr,
 		&memo.AlterTableRelocatePrivate{
-			RelocateLease:     relocate.RelocateLease,
-			RelocateNonVoters: relocate.RelocateNonVoters,
+			SubjectReplicas: relocate.SubjectReplicas,
 			AlterTableSplitPrivate: memo.AlterTableSplitPrivate{
 				Table:   b.factory.Metadata().AddTable(table, &tn),
 				Index:   index.Ordinal(),

pkg/sql/opt/optbuilder/testdata/alter_range

@@ -6,7 +6,7 @@ CREATE TABLE abc (a INT PRIMARY KEY, b INT, c STRING, INDEX b (b), UNIQUE INDEX
 build
 ALTER RANGE 1 RELOCATE FROM 1 TO 2
 ----
-alter-range-relocate &{false false 2 1 [3 4 5] []}
+alter-range-relocate &{VOTERS 2 1 [3 4 5] []}
  ├── columns: range_id:3 pretty:4 result:5
  └── values
       ├── columns: column1:6!null
@@ -15,7 +15,7 @@ alter-range-relocate &{false false 2 1 [3 4 5] []}
 build
 ALTER RANGE RELOCATE FROM 1 TO 2 FOR SELECT a from abc
 ----
-alter-range-relocate &{false false 2 1 [3 4 5] []}
+alter-range-relocate &{VOTERS 2 1 [3 4 5] []}
  ├── columns: range_id:3 pretty:4 result:5
  └── project
       ├── columns: a:6!null
@@ -30,21 +30,21 @@ error (42601): at or near "relocate": syntax error
 build
 ALTER RANGE RELOCATE FROM 1 TO 2 FOR SELECT c from abc
 ----
-error (42601): RELOCATE data column 1 (range ids) must be of type int, not type string
+error (42601): RELOCATE VOTERS data column 1 (range ids) must be of type int, not type string
 
 build
-ALTER RANGE 1 RELOCATE NON_VOTERS FROM 1 TO 2
+ALTER RANGE 1 RELOCATE NONVOTERS FROM 1 TO 2
 ----
-alter-range-relocate &{false true 2 1 [3 4 5] []}
+alter-range-relocate &{NONVOTERS 2 1 [3 4 5] []}
  ├── columns: range_id:3 pretty:4 result:5
  └── values
       ├── columns: column1:6!null
       └── (1,)
 
 build
-ALTER RANGE RELOCATE NON_VOTERS FROM 1 TO 2 FOR SELECT a from abc
+ALTER RANGE RELOCATE NONVOTERS FROM 1 TO 2 FOR SELECT a from abc
 ----
-alter-range-relocate &{false true 2 1 [3 4 5] []}
+alter-range-relocate &{NONVOTERS 2 1 [3 4 5] []}
  ├── columns: range_id:3 pretty:4 result:5
  └── project
       ├── columns: a:6!null
@@ -55,7 +55,7 @@ alter-range-relocate &{false true 2 1 [3 4 5] []}
 build
 ALTER RANGE 1 RELOCATE LEASE TO 2
 ----
-alter-range-relocate &{true false 2 0 [3 4 5] []}
+alter-range-relocate &{LEASE 2 0 [3 4 5] []}
  ├── columns: range_id:3 pretty:4 result:5
  └── values
       ├── columns: column1:6!null
@@ -64,7 +64,7 @@ alter-range-relocate &{true false 2 0 [3 4 5] []}
 build
 ALTER RANGE RELOCATE LEASE TO 2 FOR SELECT a from abc
 ----
-alter-range-relocate &{true false 2 0 [3 4 5] []}
+alter-range-relocate &{LEASE 2 0 [3 4 5] []}
  ├── columns: range_id:3 pretty:4 result:5
  └── project
       ├── columns: a:6!null

pkg/sql/opt/optbuilder/testdata/alter_table

@@ -137,7 +137,7 @@ alter-table-relocate abc [voters]
       └── (ARRAY[4], 2)
 
 build
-ALTER TABLE abc EXPERIMENTAL_RELOCATE NON_VOTERS VALUES (ARRAY[1,2,3], 1), (ARRAY[4], 2)
+ALTER TABLE abc EXPERIMENTAL_RELOCATE NONVOTERS VALUES (ARRAY[1,2,3], 1), (ARRAY[4], 2)
 ----
 alter-table-relocate abc [non-voters]
  ├── columns: key:1 pretty:2
@@ -149,12 +149,12 @@ alter-table-relocate abc [non-voters]
 build
 ALTER TABLE abc EXPERIMENTAL_RELOCATE LEASE VALUES (10), (11)
 ----
-error (42601): less than 2 columns in EXPERIMENTAL_RELOCATE LEASE data
+error (42601): less than 2 columns in RELOCATE LEASE data
 
 build
 ALTER TABLE abc EXPERIMENTAL_RELOCATE LEASE VALUES (10, 1, 2), (11, 3, 4)
 ----
-error (42601): too many columns in EXPERIMENTAL_RELOCATE LEASE data
+error (42601): too many columns in RELOCATE LEASE data
 
 build
 ALTER INDEX abc@bc EXPERIMENTAL_RELOCATE VALUES (ARRAY[5], 1, 'foo'), (ARRAY[6,7,8], 2, 'bar')
@@ -167,7 +167,7 @@ alter-table-relocate abc@bc [voters]
       └── (ARRAY[6,7,8], 2, 'bar')
 
 build
-ALTER INDEX abc@bc EXPERIMENTAL_RELOCATE NON_VOTERS VALUES (ARRAY[5], 1, 'foo'), (ARRAY[6,7,8], 2, 'bar')
+ALTER INDEX abc@bc EXPERIMENTAL_RELOCATE NONVOTERS VALUES (ARRAY[5], 1, 'foo'), (ARRAY[6,7,8], 2, 'bar')
 ----
 alter-table-relocate abc@bc [non-voters]
  ├── columns: key:1 pretty:2
@@ -179,22 +179,22 @@ alter-table-relocate abc@bc [non-voters]
 build
 ALTER INDEX abc@bc EXPERIMENTAL_RELOCATE VALUES (5, 1, 'foo'), (6, 2, 'bar')
 ----
-error (42601): EXPERIMENTAL_RELOCATE data column 1 (relocation array) must be of type int[], not type int
+error (42601): RELOCATE data column 1 (relocation array) must be of type int[], not type int
 
 build
 ALTER INDEX abc@bc EXPERIMENTAL_RELOCATE LEASE VALUES (ARRAY[5], 1, 'foo'), (ARRAY[6,7,8], 2, 'bar')
 ----
-error (42601): EXPERIMENTAL_RELOCATE LEASE data column 1 (target leaseholder) must be of type int, not type int[]
+error (42601): RELOCATE LEASE data column 1 (target leaseholder) must be of type int, not type int[]
 
 build
 ALTER INDEX abc@bc EXPERIMENTAL_RELOCATE VALUES (1, 2), (3, 4)
 ----
-error (42601): EXPERIMENTAL_RELOCATE data column 1 (relocation array) must be of type int[], not type int
+error (42601): RELOCATE data column 1 (relocation array) must be of type int[], not type int
 
 build
 ALTER INDEX abc@bc EXPERIMENTAL_RELOCATE VALUES (ARRAY[1,2], 1, 2), (ARRAY[3,4], 3, 4)
 ----
-error (42601): EXPERIMENTAL_RELOCATE data column 3 (c) must be of type string, not type int
+error (42601): RELOCATE data column 3 (c) must be of type string, not type int
 
 build
 ALTER INDEX abc@bc EXPERIMENTAL_RELOCATE LEASE VALUES (10, 1, 'foo'), (11, 3, 'bar')

pkg/sql/opt/optgen/cmd/optgen/metadata.go

@@ -238,6 +238,7 @@ func newMetadata(compiled *lang.CompiledExpr, pkg string) *metadata {
 		"ScheduleCommand":     {fullName: "tree.ScheduleCommand", passByVal: true},
 		"IndexOrdinal":        {fullName: "cat.IndexOrdinal", passByVal: true},
 		"IndexOrdinals":       {fullName: "cat.IndexOrdinals", passByVal: true},
+		"RelocateSubject":     {fullName: "tree.RelocateSubject", passByVal: true},
 		"UniqueOrdinals":      {fullName: "cat.UniqueOrdinals", passByVal: true},
 		"ViewDeps":            {fullName: "opt.ViewDeps", passByVal: true},
 		"ViewTypeDeps":        {fullName: "opt.ViewTypeDeps", passByVal: true},