sql: add validation to cascading zone configs #32861

ridwanmsharif · 2018-12-05T19:14:30Z

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:

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
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.

cockroach-teamcity · 2018-12-05T19:14:36Z

This change is

knz

Code LGTM. Well done. A few comments below.

Reviewed 10 of 10 files at r1, 6 of 6 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/cmd/roachtest/split.go, line 143 at r2 (raw file):

		setRangeMaxBytes := func(maxBytes int) {
			stmtZone := fmt.Sprintf(
				"ALTER RANGE default CONFIGURE ZONE USING range_max_bytes = %d, range_min_bytes = COPY FROM PARENT",

please check with the powers that be that this test code is only used with 2.2/master; if it is also used with 2.1 you'll need to backport the syntax change to 2.1 before you can merge the test update.

pkg/sql/set_zone_config.go, line 472 at r2 (raw file):

	// Per-replica constraints cannot be set unless num_replicas is explicitly set
	if err := zoneToWrite.ValidateTandemFields(); err != nil {
		return pgerror.NewErrorf(pgerror.CodeInternalError,

This is not an internal error. Internal errors are bugs in CockroachDB; also internal errors get uploaded to our telemetry data.

Here you are simply reporting an error to the client/user. You can use one of the other error codes instead.

knz

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/sql/parser/sql.y, line 1416 at r1 (raw file):

var_set_list:
  var_name '=' COPY FROM PARENT

Also add the missing tests to parse_test.go.

ridwanmsharif

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/cmd/roachtest/split.go, line 143 at r2 (raw file):

Previously, knz (kena) wrote…

please check with the powers that be that this test code is only used with 2.2/master; if it is also used with 2.1 you'll need to backport the syntax change to 2.1 before you can merge the test update.

This test uses 2.2 but good note, changes the other one to use the min bytes explicitly.

pkg/sql/set_zone_config.go, line 472 at r2 (raw file):

Previously, knz (kena) wrote…

This is not an internal error. Internal errors are bugs in CockroachDB; also internal errors get uploaded to our telemetry data.

Here you are simply reporting an error to the client/user. You can use one of the other error codes instead.

Done.

pkg/sql/parser/sql.y, line 1416 at r1 (raw file):

Previously, knz (kena) wrote…

Also add the missing tests to parse_test.go.

Done.

benesch · 2018-12-06T17:36:11Z

Approach looks fantastic to me! If @knz is happy, I'm happy.

knz · 2018-12-07T18:56:08Z

pkg/sql/sem/tree/zone.go

-		ctx.FormatNode(&node.Options)
+		kvOptions := node.Options
+		for i, kv := range kvOptions {
+			if i > 0 {


comma := "" for ... { WriteString(comma) ... comma = ", " }

knz · 2018-12-07T18:57:38Z

pkg/sql/set_zone_config.go

+				if optionStr.Len() > 0 {
+					optionStr.WriteString(", ")
+				}
+				fmt.Fprintf(&optionStr, "%s = COPY FROM PARENT", string(*name))


wait what no you should not convert the name to a string!!

it's fmt("%s = ...", name)

knz

Reviewed 7 of 7 files at r3, 5 of 5 files at r4, 1 of 3 files at r5, 2 of 2 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/sql/set_zone_config.go, line 473 at r3 (raw file):

	if err := zoneToWrite.ValidateTandemFields(); err != nil {
		return pgerror.NewErrorf(pgerror.CodeInvalidParameterValueError,
			fmt.Sprintf("could not validate zone config: %s", err)).SetHintf(

we use %v for errors not %s
NewErrorf means you can use formatting -- so it's pgerror.NewErrorf(code, "foo %v", err) not NewErrorf(code, fmt.Sprintf(...))

pkg/sql/set_zone_config.go, line 134 at r6 (raw file):

		for _, opt := range n.Options {
			if _, alreadyExists := options[opt.Key]; alreadyExists {
				return nil, fmt.Errorf("duplicate zone config parameter: %q", tree.ErrString(&opt.Key))

while you're at it please convert the errors around here to pgerrors

pkg/sql/set_zone_config.go, line 216 at r6 (raw file):

					optionStr.WriteString(", ")
				}
				fmt.Fprintf(&optionStr, "%s = COPY FROM PARENT", *name)

pass name not *name, they are not the same.

pkg/sql/set_zone_config.go, line 224 at r6 (raw file):

			}
			if datum == tree.DNull {
				return fmt.Errorf("unsupported NULL value for %q", tree.ErrString(name))

ditto pgerror

pkg/sql/set_zone_config.go, line 231 at r6 (raw file):

				optionStr.WriteString(", ")
			}
			fmt.Fprintf(&optionStr, "%s = %s", *name, datum)

ditto name vs *name

Adds a `COPY FROM PARENT` command to zone configs to solidify a field. Solidifying a field means, the field will now be explicitly set to the value that applied to it. Any change to a parent zone on the field will no longer affect it. This change will allow people to do the following: `ALTER .... CONFIGURE ZONE USING range_min_bytes = ..., range_max_bytes = COPY FROM PARENT` instead of figuring out what the `RangeMaxBytes`should be and populating it manually. This change will allow us to add a validation constraint that tandem fields are set together (for more details see the cascading zone config RFC). Release note (sql change): New SQL command added. `ALTER ... CONFIGURE ZONE USING field_name = COPY FROM PARENT` now solidifies/explicitly sets the field.

As described in the cascading zone RFC, a few additional constraints are required on zone config usage so no zone is ever in an invalid state if their parent zone changes a value. Release note: None

The formatter is updated to now recognise the COPY FROM PARENT command when the value in the KVOption is nil. Release note: None

ridwanmsharif

TFTR!

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/sql/set_zone_config.go, line 473 at r3 (raw file):

Previously, knz (kena) wrote…

we use %v for errors not %s

NewErrorf means you can use formatting -- so it's pgerror.NewErrorf(code, "foo %v", err) not NewErrorf(code, fmt.Sprintf(...))

Done.

pkg/sql/set_zone_config.go, line 134 at r6 (raw file):

Previously, knz (kena) wrote…

while you're at it please convert the errors around here to pgerrors

Done.

pkg/sql/set_zone_config.go, line 216 at r6 (raw file):

Previously, knz (kena) wrote…

pass name not *name, they are not the same.

Done.

pkg/sql/set_zone_config.go, line 224 at r6 (raw file):

Previously, knz (kena) wrote…

ditto pgerror

Done.

pkg/sql/set_zone_config.go, line 231 at r6 (raw file):

Previously, knz (kena) wrote…

ditto name vs *name

Done.

knz

Reviewed 6 of 9 files at r7, 4 of 5 files at r8, 3 of 3 files at r9.
Reviewable status: complete! 1 of 0 LGTMs obtained

ridwanmsharif · 2018-12-12T21:05:04Z

bors r+

@knz

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>

craig · 2018-12-12T21:24:13Z

Build succeeded

GitHub CI (Cockroach)

ridwanmsharif requested review from a team December 5, 2018 19:14

ridwanmsharif requested a review from knz December 5, 2018 19:14

ridwanmsharif mentioned this pull request Dec 5, 2018

config, sql: completing partial zone configs #32059

Closed

ridwanmsharif force-pushed the zone-validation-2 branch 3 times, most recently from 1ffde96 to 3bd4344 Compare December 5, 2018 23:09

ridwanmsharif requested review from a team and benesch December 6, 2018 03:58

ridwanmsharif force-pushed the zone-validation-2 branch from 97a7de3 to 25d3f0e Compare December 6, 2018 05:03

ridwanmsharif requested a review from a team as a code owner December 6, 2018 05:03

knz reviewed Dec 6, 2018

View reviewed changes

ridwanmsharif force-pushed the zone-validation-2 branch from 25d3f0e to 6f8e49d Compare December 6, 2018 17:32

ridwanmsharif commented Dec 6, 2018

View reviewed changes

ridwanmsharif requested a review from knz December 6, 2018 17:51

ridwanmsharif force-pushed the zone-validation-2 branch from 6f8e49d to 3389c8c Compare December 6, 2018 17:58

knz reviewed Dec 7, 2018

View reviewed changes

ridwanmsharif force-pushed the zone-validation-2 branch from 244999a to a13ac50 Compare December 7, 2018 22:34

ridwanmsharif requested a review from knz December 7, 2018 22:50

knz reviewed Dec 7, 2018

View reviewed changes

Ridwan Sharif added 3 commits December 12, 2018 10:02

config: Add zone validation for tandem fields

5c2f145

As described in the cascading zone RFC, a few additional constraints are required on zone config usage so no zone is ever in an invalid state if their parent zone changes a value. Release note: None

sql: Update the zone specifier formatter

733cfb7

The formatter is updated to now recognise the COPY FROM PARENT command when the value in the KVOption is nil. Release note: None

ridwanmsharif force-pushed the zone-validation-2 branch from a13ac50 to 733cfb7 Compare December 12, 2018 15:40

ridwanmsharif commented Dec 12, 2018

View reviewed changes

ridwanmsharif requested a review from knz December 12, 2018 16:11

knz approved these changes Dec 12, 2018

View reviewed changes

craig bot merged commit 733cfb7 into cockroachdb:master Dec 12, 2018

ridwanmsharif deleted the zone-validation-2 branch December 12, 2018 21:55

jseldess mentioned this pull request Jan 15, 2019

sql: add validation to cascading zone configs cockroachdb/docs#4255

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: add validation to cascading zone configs #32861

sql: add validation to cascading zone configs #32861

ridwanmsharif commented Dec 5, 2018 •

edited

Loading

cockroach-teamcity commented Dec 5, 2018

knz left a comment

knz left a comment

ridwanmsharif left a comment

benesch commented Dec 6, 2018

knz Dec 7, 2018

knz Dec 7, 2018

knz left a comment

ridwanmsharif left a comment

knz left a comment

ridwanmsharif commented Dec 12, 2018

craig bot commented Dec 12, 2018

sql: add validation to cascading zone configs #32861

sql: add validation to cascading zone configs #32861

Conversation

ridwanmsharif commented Dec 5, 2018 • edited Loading

cockroach-teamcity commented Dec 5, 2018

knz left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

ridwanmsharif left a comment

Choose a reason for hiding this comment

benesch commented Dec 6, 2018

knz Dec 7, 2018

Choose a reason for hiding this comment

knz Dec 7, 2018

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

ridwanmsharif left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

ridwanmsharif commented Dec 12, 2018

craig bot commented Dec 12, 2018

Build succeeded

ridwanmsharif commented Dec 5, 2018 •

edited

Loading