Merge #32861

32861: sql: add validation to cascading zone configs r=ridwanmsharif a=ridwanmsharif

I found a few hours free and so took a second stab at #32059. Thought it'd be fun and new to try adding something to SQL for this, so @knz please feel free to tell me if what I've done here makes no sense. 

There are to be two changes to be made: 
1) Allow for the `COPY FROM PARENT` command so users can do something like:
` ALTER ... CONFIGURE ZONE USING range_min_bytes = ..., range_max_bytes = COPY FROM PARENT`

2) Add validation to the zone configs so certain fields are written in tandem. The fields are:
      - `NumReplicas` and `Constraints` (Only when per-replica constraints are in use)
      - `RangeMinBytes` and `RangeMaxBytes`
      - `LeasePreferences` and `Constraints`

Each of the changes are contained in their own commits.

Co-authored-by: Ridwan Sharif <ridwan@cockroachlabs.com>

docs/generated/sql/bnf/alter_zone_database_stmt.bnf

@@ -1,3 +1,4 @@
 alter_zone_database_stmt ::=
-	'ALTER' 'DATABASE' database_name 'CONFIGURE' 'ZONE' 'USING' variable '=' value ( ( ',' variable '=' value ) )*
+	'ALTER' 'DATABASE' database_name 'CONFIGURE' 'ZONE' 'USING' variable '=' 'COPY' 'FROM' 'PARENT' ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )*
+	| 'ALTER' 'DATABASE' database_name 'CONFIGURE' 'ZONE' 'USING' variable '=' value ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )*
 	| 'ALTER' 'DATABASE' database_name 'CONFIGURE' 'ZONE' 'DISCARD'

docs/generated/sql/bnf/alter_zone_index_stmt.bnf

@@ -1,5 +1,7 @@
 alter_zone_index_stmt ::=
-	'ALTER' 'INDEX' table_name '@' index_name 'CONFIGURE' 'ZONE' 'USING' variable '=' value ( ( ',' variable '=' value ) )*
+	'ALTER' 'INDEX' table_name '@' index_name 'CONFIGURE' 'ZONE' 'USING' variable '=' 'COPY' 'FROM' 'PARENT' ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )*
+	| 'ALTER' 'INDEX' table_name '@' index_name 'CONFIGURE' 'ZONE' 'USING' variable '=' value ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )*
 	| 'ALTER' 'INDEX' table_name '@' index_name 'CONFIGURE' 'ZONE' 'DISCARD'
-	| 'ALTER' 'INDEX' table_name 'CONFIGURE' 'ZONE' 'USING' variable '=' value ( ( ',' variable '=' value ) )*
+	| 'ALTER' 'INDEX' table_name 'CONFIGURE' 'ZONE' 'USING' variable '=' 'COPY' 'FROM' 'PARENT' ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )*
+	| 'ALTER' 'INDEX' table_name 'CONFIGURE' 'ZONE' 'USING' variable '=' value ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )*
 	| 'ALTER' 'INDEX' table_name 'CONFIGURE' 'ZONE' 'DISCARD'

docs/generated/sql/bnf/alter_zone_range_stmt.bnf

@@ -1,3 +1,4 @@
 alter_zone_range_stmt ::=
-	'ALTER' 'RANGE' range_name 'CONFIGURE' 'ZONE' 'USING' variable '=' value ( ( ',' variable '=' value ) )*
+	'ALTER' 'RANGE' range_name 'CONFIGURE' 'ZONE' 'USING' variable '=' 'COPY' 'FROM' 'PARENT' ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )*
+	| 'ALTER' 'RANGE' range_name 'CONFIGURE' 'ZONE' 'USING' variable '=' value ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )*
 	| 'ALTER' 'RANGE' range_name 'CONFIGURE' 'ZONE' 'DISCARD'

docs/generated/sql/bnf/alter_zone_table_stmt.bnf

@@ -1,5 +1,7 @@
 alter_zone_table_stmt ::=
-	'ALTER' 'TABLE' table_name 'CONFIGURE' 'ZONE' 'USING' variable '=' value ( ( ',' variable '=' value ) )*
+	'ALTER' 'TABLE' table_name 'CONFIGURE' 'ZONE' 'USING' variable '=' 'COPY' 'FROM' 'PARENT' ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )*
+	| 'ALTER' 'TABLE' table_name 'CONFIGURE' 'ZONE' 'USING' variable '=' value ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )*
 	| 'ALTER' 'TABLE' table_name 'CONFIGURE' 'ZONE' 'DISCARD'
-	| 'ALTER' 'PARTITION' partition_name 'OF' 'TABLE' table_name 'CONFIGURE' 'ZONE' 'USING' variable '=' value ( ( ',' variable '=' value ) )*
+	| 'ALTER' 'PARTITION' partition_name 'OF' 'TABLE' table_name 'CONFIGURE' 'ZONE' 'USING' variable '=' 'COPY' 'FROM' 'PARENT' ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )*
+	| 'ALTER' 'PARTITION' partition_name 'OF' 'TABLE' table_name 'CONFIGURE' 'ZONE' 'USING' variable '=' value ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )*
 	| 'ALTER' 'PARTITION' partition_name 'OF' 'TABLE' table_name 'CONFIGURE' 'ZONE' 'DISCARD'

docs/generated/sql/bnf/stmt_block.bnf

@@ -1829,7 +1829,7 @@ alter_table_cmd ::=
 	| partition_by
 
 var_set_list ::=
-	( var_name '=' var_value ) ( ( ',' var_name '=' var_value ) )*
+	( var_name '=' 'COPY' 'FROM' 'PARENT' | var_name '=' var_value ) ( ( ',' var_name '=' var_value | ',' var_name '=' 'COPY' 'FROM' 'PARENT' ) )*
 
 alter_index_cmd ::=
 	partition_by

pkg/cli/testdata/zone_range_max_bytes.yaml

@@ -1,2 +1,3 @@
 range_max_bytes: 134217728
+range_min_bytes: 16777216
 num_replicas: 3

pkg/cmd/roachtest/split.go

@@ -138,8 +138,11 @@ func runLoadSplits(ctx context.Context, t *test, c *cluster, params splitParams)
 		}
 
 		t.Status("increasing range_max_bytes")
+		minBytes := 16 << 20 // 16 MB
 		setRangeMaxBytes := func(maxBytes int) {
-			stmtZone := fmt.Sprintf("ALTER RANGE default CONFIGURE ZONE USING range_max_bytes = %d", maxBytes)
+			stmtZone := fmt.Sprintf(
+				"ALTER RANGE default CONFIGURE ZONE USING range_max_bytes = %d, range_min_bytes = %d",
+				maxBytes, minBytes)
 			if _, err := db.Exec(stmtZone); err != nil {
 				t.Fatalf("failed to set range_max_bytes: %v", err)
 			}
@@ -252,8 +255,11 @@ func runLargeRangeSplits(ctx context.Context, t *test, c *cluster, size int) {
 		}
 
 		t.Status("increasing range_max_bytes")
+		minBytes := 16 << 20 // 16 MB
 		setRangeMaxBytes := func(maxBytes int) {
-			stmtZone := fmt.Sprintf("ALTER RANGE default CONFIGURE ZONE USING range_max_bytes = %d", maxBytes)
+			stmtZone := fmt.Sprintf(
+				"ALTER RANGE default CONFIGURE ZONE USING range_max_bytes = %d, range_min_bytes = %d",
+				maxBytes, minBytes)
 			_, err := db.Exec(stmtZone)
 			if err != nil && strings.Contains(err.Error(), "syntax error") {
 				// Pre-2.1 was EXPERIMENTAL.

pkg/config/zone.go

@@ -379,6 +379,28 @@ func (z *ZoneConfig) IsComplete() bool {
 		(!z.InheritedConstraints) && (!z.InheritedLeasePreferences))
 }
 
+// ValidateTandemFields returns an error if the ZoneConfig to be written
+// specifies a configuration that could cause problems with the introduction
+// of cascading zone configs.
+func (z *ZoneConfig) ValidateTandemFields() error {
+	var numConstrainedRepls int32
+	for _, constraint := range z.Constraints {
+		numConstrainedRepls += constraint.NumReplicas
+	}
+
+	if numConstrainedRepls > 0 && z.NumReplicas == nil {
+		return fmt.Errorf("when per-replica constraints are set, num_replicas must be set as well")
+	}
+	if (z.RangeMinBytes != nil || z.RangeMaxBytes != nil) &&
+		(z.RangeMinBytes == nil || z.RangeMaxBytes == nil) {
+		return fmt.Errorf("range_min_bytes and range_max_bytes must be set together")
+	}
+	if !z.InheritedLeasePreferences && z.InheritedConstraints {
+		return fmt.Errorf("lease preferences can not be set unless the constraints are explicitly set as well")
+	}
+	return nil
+}
+
 // Validate returns an error if the ZoneConfig specifies a known-dangerous or
 // disallowed configuration.
 func (z *ZoneConfig) Validate() error {
@@ -510,6 +532,45 @@ func (z *ZoneConfig) InheritFromParent(parent ZoneConfig) {
 	}
 }
 
+// CopyFromZone copies over the specified fields from the other zone.
+func (z *ZoneConfig) CopyFromZone(other ZoneConfig, fieldList []tree.Name) {
+	for _, fieldName := range fieldList {
+		if fieldName == "num_replicas" {
+			z.NumReplicas = nil
+			if other.NumReplicas != nil {
+				z.NumReplicas = proto.Int32(*other.NumReplicas)
+			}
+		}
+		if fieldName == "range_min_bytes" {
+			z.RangeMinBytes = nil
+			if other.RangeMinBytes != nil {
+				z.RangeMinBytes = proto.Int64(*other.RangeMinBytes)
+			}
+		}
+		if fieldName == "range_max_bytes" {
+			z.RangeMaxBytes = nil
+			if other.RangeMaxBytes != nil {
+				z.RangeMaxBytes = proto.Int64(*other.RangeMaxBytes)
+			}
+		}
+		if fieldName == "gc.ttlseconds" {
+			z.GC = nil
+			if other.GC != nil {
+				tempGC := *other.GC
+				z.GC = &tempGC
+			}
+		}
+		if fieldName == "constraints" {
+			z.Constraints = other.Constraints
+			z.InheritedConstraints = other.InheritedConstraints
+		}
+		if fieldName == "lease_preferences" {
+			z.LeasePreferences = other.LeasePreferences
+			z.InheritedLeasePreferences = other.InheritedLeasePreferences
+		}
+	}
+}
+
 // StoreMatchesConstraint returns whether a store matches the given constraint.
 func StoreMatchesConstraint(store roachpb.StoreDescriptor, constraint Constraint) bool {
 	hasConstraint := storeHasConstraint(store, constraint)

pkg/config/zone_test.go

@@ -245,6 +245,58 @@ func TestZoneConfigValidate(t *testing.T) {
 	}
 }
 
+func TestZoneConfigValidateTandemFields(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	testCases := []struct {
+		cfg      ZoneConfig
+		expected string
+	}{
+		{
+			ZoneConfig{
+				RangeMaxBytes: DefaultZoneConfig().RangeMaxBytes,
+			},
+			"range_min_bytes and range_max_bytes must be set together",
+		},
+		{
+			ZoneConfig{
+				RangeMinBytes: DefaultZoneConfig().RangeMinBytes,
+			},
+			"range_min_bytes and range_max_bytes must be set together",
+		},
+		{
+			ZoneConfig{
+				Constraints: []Constraints{
+					{
+						Constraints: []Constraint{{Value: "a", Type: Constraint_REQUIRED}},
+						NumReplicas: 2,
+					},
+				},
+			},
+			"when per-replica constraints are set, num_replicas must be set as well",
+		},
+		{
+			ZoneConfig{
+				InheritedConstraints:      true,
+				InheritedLeasePreferences: false,
+				LeasePreferences: []LeasePreference{
+					{
+						Constraints: []Constraint{},
+					},
+				},
+			},
+			"lease preferences can not be set unless the constraints are explicitly set as well",
+		},
+	}
+
+	for i, c := range testCases {
+		err := c.cfg.ValidateTandemFields()
+		if !testutils.IsError(err, c.expected) {
+			t.Errorf("%d: expected %q, got %v", i, c.expected, err)
+		}
+	}
+}
+
 func TestZoneConfigSubzones(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 

pkg/sql/logictest/testdata/logic_test/event_log

@@ -358,7 +358,7 @@ statement ok
 CREATE TABLE a (id INT PRIMARY KEY)
 
 statement ok
-ALTER TABLE a CONFIGURE ZONE USING range_max_bytes = 67108865
+ALTER TABLE a CONFIGURE ZONE USING range_max_bytes = 67108865, range_min_bytes = 16777216
 
 statement ok
 ALTER TABLE a CONFIGURE ZONE DISCARD
@@ -371,7 +371,7 @@ FROM system.eventlog
 WHERE "eventType" = 'set_zone_config'
 ORDER BY "timestamp"
 ----
-60  1  {"Target":"test.a","Options":"range_max_bytes = 67108865","User":"root"}
+60  1  {"Target":"test.a","Options":"range_max_bytes = 67108865, range_min_bytes = 16777216","User":"root"}
 
 query IIT
 SELECT "targetID", "reportingID", "info"

pkg/sql/parser/parse_test.go

@@ -1102,6 +1102,10 @@ func TestParse(t *testing.T) {
 		{`ALTER INDEX db.t@i CONFIGURE ZONE USING foo = bar, baz = yay`},
 		{`ALTER INDEX t@i CONFIGURE ZONE USING foo = bar, baz = yay`},
 		{`ALTER INDEX i CONFIGURE ZONE USING foo = bar, baz = yay`},
+		{`ALTER INDEX i CONFIGURE ZONE USING foo = COPY FROM PARENT`},
+		{`ALTER INDEX i CONFIGURE ZONE USING foo = bar, baz = COPY FROM PARENT`},
+		{`ALTER INDEX i CONFIGURE ZONE USING foo = COPY FROM PARENT, baz = COPY FROM PARENT`},
+		{`ALTER INDEX i CONFIGURE ZONE USING foo = bar, other = COPY FROM PARENT, baz = yay`},
 
 		{`ALTER RANGE default CONFIGURE ZONE DISCARD`},
 		{`ALTER RANGE meta CONFIGURE ZONE DISCARD`},
@@ -1688,6 +1692,8 @@ func TestParse2(t *testing.T) {
 			`ALTER INDEX t@i CONFIGURE ZONE USING "foo.bar" = yay`},
 		{`ALTER INDEX i CONFIGURE ZONE USING foo.bar = yay`,
 			`ALTER INDEX i CONFIGURE ZONE USING "foo.bar" = yay`},
+		{`ALTER INDEX i CONFIGURE ZONE USING foo = COPY FROM PARENT`,
+			`ALTER INDEX i CONFIGURE ZONE USING foo = COPY FROM PARENT`},
 
 		// Alternative forms for table patterns.
 

pkg/sql/parser/sql.y

@@ -1123,6 +1123,7 @@ alter_ddl_stmt:
 // Zone configurations:
 //   DISCARD
 //   USING <var> = <expr> [, ...]
+//   USING <var> = COPY FROM PARENT [, ...]
 //   { TO | = } <expr>
 //
 // %SeeAlso: WEBDOCS/alter-table.html
@@ -1217,6 +1218,7 @@ alter_database_stmt:
 // Zone configurations:
 //   DISCARD
 //   USING <var> = <expr> [, ...]
+//   USING <var> = COPY FROM PARENT [, ...]
 //   { TO | = } <expr>
 //
 // %SeeAlso: ALTER TABLE
@@ -1412,14 +1414,22 @@ alter_zone_index_stmt:
   }
 
 var_set_list:
-  var_name '=' var_value
+  var_name '=' COPY FROM PARENT
+  {
+    $$.val = []tree.KVOption{tree.KVOption{Key: tree.Name(strings.Join($1.strs(), "."))}}
+  }
+| var_name '=' var_value
   {
     $$.val = []tree.KVOption{tree.KVOption{Key: tree.Name(strings.Join($1.strs(), ".")), Value: $3.expr()}}
   }
 | var_set_list ',' var_name '=' var_value
   {
     $$.val = append($1.kvOptions(), tree.KVOption{Key: tree.Name(strings.Join($3.strs(), ".")), Value: $5.expr()})
   }
+| var_set_list ',' var_name '=' COPY FROM PARENT
+  {
+    $$.val = append($1.kvOptions(), tree.KVOption{Key: tree.Name(strings.Join($3.strs(), "."))})
+  }
 
 alter_scatter_stmt:
   ALTER TABLE table_name SCATTER

pkg/sql/sem/tree/zone.go

@@ -102,6 +102,18 @@ func (node *SetZoneConfig) Format(ctx *FmtCtx) {
 		}
 	} else {
 		ctx.WriteString("USING ")
-		ctx.FormatNode(&node.Options)
+		kvOptions := node.Options
+		comma := ""
+		for _, kv := range kvOptions {
+			ctx.WriteString(comma)
+			comma = ", "
+			ctx.FormatNode(&kv.Key)
+			if kv.Value != nil {
+				ctx.WriteString(` = `)
+				ctx.FormatNode(kv.Value)
+			} else {
+				ctx.WriteString(` = COPY FROM PARENT`)
+			}
+		}
 	}
 }