sql: Require FORCE to modify protected zone config fields by ajstorm · Pull Request #61499 · cockroachdb/cockroach

ajstorm · 2021-03-04T20:28:09Z

With the introduction of the multi-region simplification in 21.1, there
are a set of fields in the zone configurations which are protected by the
system. These fields are transparently modified by the system when
certain multi-region abstractions are used (e.g. when a region is added
to the database, the constraints field is modified to add the new
region). Due the protected status of these field, we want to prevent
users from setting them if they're not aware of the impact in doing so
(that they could be overwritten by the system, and that they could
result in sub-optimal performance). To make this more explicit to users,
we block modifications to these fields and if the users wish to modify
them anyway, they must provide a FORCE argument along with the
modification statement. This impacts both the setting of the field in
the zone configuration, as well as any eventual multi-region statement
which follows and will result in over-writing the user's zone
configuration update.

Release justification: Prevent bad user experience with new feature.

Release note (sql change): Updates to certain fields in the zone
configurations are blocked for multi-region enabled databases. This
block can be overridden through the use of the FORCE keyword on the
blocked statement.

Resolves #60447

cockroach-teamcity · 2021-03-04T20:28:17Z

This change is

otan

First pass, tidier than you hyped it out to be.
I think I found a flaw that requires more work, and a testing change that is optional.

Reviewed 12 of 13 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm and @arulajmani)

pkg/config/zonepb/zone.go, line 603 at r1 (raw file):

// receiver ZoneConfig. Returns true if all are equal, and false if there is a
// difference (along with a string which represents the first difference found).
func (z *ZoneConfig) DiffWithZone(other ZoneConfig, fieldList []tree.Name) (string, bool, error) {

nit: prefer order (bool, string, error), or fix the comment to reflect the order.

this is also a strangely rare case where we can unit test this entire function to avoid the longer and harder to follow logic_test.

pkg/config/zonepb/zone.go, line 685 at r1 (raw file):

				}
			}
		case "lease_preferences":

you're missing subzone comparisons in here. this validates table level but not index level AFAICT.

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

	}

	if string(n.Database) != "" {

nit: cast not necessary

ajstorm

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @otan)

pkg/config/zonepb/zone.go, line 603 at r1 (raw file):

nit: prefer order (bool, string, error), or fix the comment to reflect the order.

Done

this is also a strangely rare case where we can unit test this entire function to avoid the longer and harder to follow logic_test.

Do you have an example of that I could follow?

pkg/config/zonepb/zone.go, line 685 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

you're missing subzone comparisons in here. this validates table level but not index level AFAICT.

Interesting. You cool with me handling this in the next PR? This one is just meant to handle database level zone configs, which, if I'm understanding things correctly, can't contain sub-zones.

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

Previously, otan (Oliver Tan) wrote…

nit: cast not necessary

Fixed.

otan

Reviewed 3 of 5 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @arulajmani, and @otan)

pkg/config/zonepb/zone.go, line 603 at r1 (raw file):

Previously, ajstorm (Adam Storm) wrote…

nit: prefer order (bool, string, error), or fix the comment to reflect the order.

Done

this is also a strangely rare case where we can unit test this entire function to avoid the longer and harder to follow logic_test.

Do you have an example of that I could follow?

uhh, i guess something like

func TestDiffWIthZone(t *testing.T) {
   testCases := []struct{
       desc string
       a, b ZoneConfig
fieldList []tree.Name // or make this constant
       
       expectedInconsistentField string
       expectedSame bool
   } {
      { desc: "same zone configs ok" a: zonepb.ZoneConfig{...}, b: zonepb.ZoneConfig{...}, expectedSame: true}
   }
 
   for _, tc := range testCases {
       t.Run(tc.desc, func(t *testing.T) {
            str, same, err :=  tc.a.DiffWithZone(tc.b, tc.fieldList)
require.NoError(t, err)
require.Equal(t, tc.expectedSame, same)
require.Equal(t, tc.expectedInconsistentField, str)
       })
    }
}

pkg/sql/region_util.go, line 870 at r2 (raw file):

		)
		err = errors.WithDetail(err, "the attempted operation will overwrite a user modified field")
		return errors.WithHint(err, "to override this error and proceed with the overwrite, specify the FORCE option")

s/option/at the end of the query?

ajstorm

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @otan)

pkg/config/zonepb/zone.go, line 603 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

uhh, i guess something like

func TestDiffWIthZone(t *testing.T) {
   testCases := []struct{
       desc string
       a, b ZoneConfig
fieldList []tree.Name // or make this constant
       
       expectedInconsistentField string
       expectedSame bool
   } {
      { desc: "same zone configs ok" a: zonepb.ZoneConfig{...}, b: zonepb.ZoneConfig{...}, expectedSame: true}
   }
 
   for _, tc := range testCases {
       t.Run(tc.desc, func(t *testing.T) {
            str, same, err :=  tc.a.DiffWithZone(tc.b, tc.fieldList)
require.NoError(t, err)
require.Equal(t, tc.expectedSame, same)
require.Equal(t, tc.expectedInconsistentField, str)
       })
    }
}

I like this idea, but would prefer handling it once we get this fix in (since reworking test cases seems like bread-and-butter stability work). You OK with me opening an issue to handle next week?

pkg/sql/region_util.go, line 870 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…

s/option/at the end of the query?

I went with "specify FORCE at the end of the statement", though I was very tempted to go with "...use the FORCE" :-P

otan

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @arulajmani, and @otan)

pkg/config/zonepb/zone.go, line 603 at r1 (raw file):

Previously, ajstorm (Adam Storm) wrote…

I like this idea, but would prefer handling it once we get this fix in (since reworking test cases seems like bread-and-butter stability work). You OK with me opening an issue to handle next week?

the approved blessing was because i know we're tight and we can wait :D

With the introduction of the multi-region simplification in 21.1, there are a set of fields in the zone configurations which are protected by the system. These fields are transparently modified by the system when certain multi-region abstractions are used (e.g. when a region is added to the database, the constraints field is modified to add the new region). Due the protected status of these field, we want to prevent users from setting them if they're not aware of the impact in doing so (that they could be overwritten by the system, and that they could result in sub-optimal performance). To make this more explicit to users, we block modifications to these fields and if the users wish to modify them anyway, they must provide a FORCE argument along with the modification statement. This impacts both the setting of the field in the zone configuration, as well as any eventual multi-region statement which follows and will result in over-writing the user's zone configuration update. Release justification: Prevent bad user experience with new feature. Release note (sql change): Updates to certain fields in the zone configurations are blocked for multi-region enabled databases. This block can be overridden through the use of the FORCE keyword on the blocked statement.

ajstorm · 2021-03-05T16:20:34Z

TFTR!

bors r=otan

craig · 2021-03-05T17:17:35Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2021-03-05T19:45:09Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2021-03-05T22:06:58Z

Build succeeded:

GitHub CI (Cockroach)

With cockroachdb#61499 we introduced new syntax (FORCE) to override setting the zone configurations on multi-region databases. Upon further reflection, it was decided that a session variable would be better suited to the task. This commit pulls out the FORCE syntax and replaces it with the use of override_multi_region_zone_config; Release note (sql change): Revert the release notes on cockroachdb#61499. We now use a session variable (override_multi_region_zone_config) to override the zone configuration on multi-region databases (and tables).

62008: sql: Change to using a session variable instead of FORCE syntax for overriding zone configuration settings for MR databases and tables r=otan a=ajstorm sql: Block zone config updates to multi-region tables Block users from updating the zone configurations of multi-region tables, without first having them set the session variable `override_multi_region_zone_config` to true. We block updates to multi-region table/partition/index zone configurations because we don't want users to accidentally override the prescribed settings, leaving them open to sub-optimal performance. Note that only the multi-region fields of the zone configuration are blocked behind this variable. All other fields (gc.ttlseconds, range_min/max_bytes, etc) can be updated without overriding. Release note (sql change): Block users from updating the zone configurations of multi-region tables. sql: Use session variable instead of FORCE to override zone configs With #61499 we introduced new syntax (FORCE) to override setting the zone configurations on multi-region databases. Upon further reflection, it was decided that a session variable would be better suited to the task. This commit pulls out the FORCE syntax and replaces it with the use of override_multi_region_zone_config; Release note (sql change): Revert the release notes on #61499. We now use a session variable (override_multi_region_zone_config) to override the zone configuration on multi-region databases (and tables). Resolves: #57668. Note to reviewers: Please ignore the first commit, as it's being separately reviewed as part of #61889. That commit was pulle out into a separate PR as it's a release blocker, and there was a need to get it in urgently. Co-authored-by: Adam Storm <storm@cockroachlabs.com>

With cockroachdb#61499 we introduced new syntax (FORCE) to override setting the zone configurations on multi-region databases. Upon further reflection, it was decided that a session variable would be better suited to the task. This commit pulls out the FORCE syntax and replaces it with the use of override_multi_region_zone_config; Release note (sql change): Revert the release notes on cockroachdb#61499. We now use a session variable (override_multi_region_zone_config) to override the zone configuration on multi-region databases (and tables).

ajstorm requested review from arulajmani and otan March 4, 2021 20:28

ajstorm force-pushed the ajstorm_zone_config_force branch from f267bda to df7b0d8 Compare March 4, 2021 20:54

otan reviewed Mar 4, 2021

View reviewed changes

ajstorm force-pushed the ajstorm_zone_config_force branch from df7b0d8 to d20facd Compare March 4, 2021 21:47

blathers-crl bot requested a review from otan March 4, 2021 21:47

ajstorm force-pushed the ajstorm_zone_config_force branch from d20facd to 1d6e803 Compare March 4, 2021 22:06

ajstorm commented Mar 4, 2021

View reviewed changes

otan approved these changes Mar 4, 2021

View reviewed changes

ajstorm force-pushed the ajstorm_zone_config_force branch from 1d6e803 to 838558d Compare March 4, 2021 22:29

ajstorm requested a review from otan March 4, 2021 22:30

ajstorm commented Mar 4, 2021

View reviewed changes

otan approved these changes Mar 4, 2021

View reviewed changes

ajstorm force-pushed the ajstorm_zone_config_force branch from 838558d to b3bf86d Compare March 5, 2021 15:22

ajstorm force-pushed the ajstorm_zone_config_force branch from b3bf86d to 5cc6900 Compare March 5, 2021 15:33

craig bot merged commit 90d6ba4 into cockroachdb:master Mar 5, 2021

ajstorm mentioned this pull request Mar 15, 2021

sql: Change to using a session variable instead of FORCE syntax for overriding zone configuration settings for MR databases and tables #62008

Merged

ajstorm mentioned this pull request Mar 16, 2021

release-21.1: sql: Change to using a session variable instead of FORCE syntax for overriding zone configuration settings for MR databases and tables #62102

Closed

ajstorm mentioned this pull request Mar 17, 2021

release-21.1: sql: Change to using a session variable instead of FORCE syntax for overriding zone configuration settings for MR databases and tables #62119

Merged

jseldess mentioned this pull request Mar 23, 2021

sql: Require FORCE to modify protected zone config fields cockroachdb/docs#10131

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: Require FORCE to modify protected zone config fields#61499

sql: Require FORCE to modify protected zone config fields#61499
craig[bot] merged 1 commit intocockroachdb:masterfrom
ajstorm:ajstorm_zone_config_force

ajstorm commented Mar 4, 2021

Uh oh!

cockroach-teamcity commented Mar 4, 2021

Uh oh!

otan left a comment

Uh oh!

ajstorm left a comment

Uh oh!

otan left a comment

Uh oh!

ajstorm left a comment

Uh oh!

otan left a comment

Uh oh!

ajstorm commented Mar 5, 2021

Uh oh!

craig bot commented Mar 5, 2021

Uh oh!

craig bot commented Mar 5, 2021

Uh oh!

craig bot commented Mar 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

ajstorm commented Mar 4, 2021

Uh oh!

cockroach-teamcity commented Mar 4, 2021

Uh oh!

otan left a comment

Choose a reason for hiding this comment

Uh oh!

ajstorm left a comment

Choose a reason for hiding this comment

Uh oh!

otan left a comment

Choose a reason for hiding this comment

Uh oh!

ajstorm left a comment

Choose a reason for hiding this comment

Uh oh!

otan left a comment

Choose a reason for hiding this comment

Uh oh!

ajstorm commented Mar 5, 2021

Uh oh!

craig bot commented Mar 5, 2021

Uh oh!

craig bot commented Mar 5, 2021

Uh oh!

craig bot commented Mar 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants