From 42f7f08a30207ebb97e63437255554ae519f7a25 Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Mon, 29 Mar 2021 18:51:50 -0400 Subject: [PATCH] sql: gate global_reads zone attribute on cluster version and enterprise license Fixes #61039. This commit gates the use of the global_reads zone configuration attribute on the cluster version and on the use of an enterprise license. The first half of this is necessary for correctness. The second half is not, because follower reads won't be served from global read followers without an enterprise license, but a check when the zone configs are set improves UX. --- .../logic_test/multi_region_mixed_20.2_21.1 | 18 +++ pkg/ccl/logictestccl/testdata/logic_test/zone | 29 ++++ pkg/ccl/multiregionccl/multiregion_test.go | 43 ++++++ .../logictest/testdata/logic_test/zone_config | 10 +- pkg/sql/set_zone_config.go | 133 +++++++++++++----- 5 files changed, 191 insertions(+), 42 deletions(-) diff --git a/pkg/ccl/logictestccl/testdata/logic_test/multi_region_mixed_20.2_21.1 b/pkg/ccl/logictestccl/testdata/logic_test/multi_region_mixed_20.2_21.1 index 1d344181e1ff..21f37f972e27 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/multi_region_mixed_20.2_21.1 +++ b/pkg/ccl/logictestccl/testdata/logic_test/multi_region_mixed_20.2_21.1 @@ -26,3 +26,21 @@ CREATE TABLE t() statement error cannot alter a table's LOCALITY if its database is not multi-region enabled ALTER TABLE t SET LOCALITY REGIONAL BY ROW + +statement error pgcode 0A000 global_reads cannot be used until cluster version is finalized +ALTER TABLE t CONFIGURE ZONE USING global_reads = true + +statement error pgcode 0A000 global_reads cannot be used until cluster version is finalized +ALTER TABLE t CONFIGURE ZONE USING global_reads = false + +statement error pgcode 0A000 num_voters cannot be used until cluster version is finalized +ALTER TABLE t CONFIGURE ZONE USING num_voters = 3 + +statement error pgcode 0A000 num_voters cannot be used until cluster version is finalized +ALTER TABLE t CONFIGURE ZONE USING num_voters = 0 + +statement error pgcode 0A000 voter_constraints cannot be used until cluster version is finalized +ALTER TABLE t CONFIGURE ZONE USING voter_constraints = '[+region=us-east-1]' + +statement error pgcode 0A000 voter_constraints cannot be used until cluster version is finalized +ALTER TABLE t CONFIGURE ZONE USING voter_constraints = '' diff --git a/pkg/ccl/logictestccl/testdata/logic_test/zone b/pkg/ccl/logictestccl/testdata/logic_test/zone index a1b7a76d0de3..274300d4d4b1 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/zone +++ b/pkg/ccl/logictestccl/testdata/logic_test/zone @@ -1126,3 +1126,32 @@ ALTER PARTITION p3 OF INDEX test.public.t@i2 CONFIGURE ZONE USING gc.ttlseconds = 15411; ALTER PARTITION p4 OF INDEX test.public.t@i2 CONFIGURE ZONE USING gc.ttlseconds = 15418 + +# Test that the global_reads attribute can be set with an enterprise license. +# This same test fails in pkg/sql/logictest/testdata/logic_test/zone_config. +statement ok +CREATE TABLE global (id INT PRIMARY KEY) + +statement ok +ALTER TABLE global CONFIGURE ZONE USING global_reads = true + +# This should reflect in the metrics. +query T +SELECT feature_name FROM crdb_internal.feature_usage +WHERE feature_name IN ( + 'sql.schema.zone_config.table.global_reads' +) AND usage_count > 0 ORDER BY feature_name +---- +sql.schema.zone_config.table.global_reads + +query TT +SHOW ZONE CONFIGURATION FOR TABLE global +---- +TABLE global ALTER TABLE global CONFIGURE ZONE USING + range_min_bytes = 134217728, + range_max_bytes = 536870912, + gc.ttlseconds = 90000, + global_reads = true, + num_replicas = 7, + constraints = '[]', + lease_preferences = '[]' diff --git a/pkg/ccl/multiregionccl/multiregion_test.go b/pkg/ccl/multiregionccl/multiregion_test.go index 85710015de04..eb56bb1d92d6 100644 --- a/pkg/ccl/multiregionccl/multiregion_test.go +++ b/pkg/ccl/multiregionccl/multiregion_test.go @@ -117,3 +117,46 @@ func TestMultiRegionAfterEnterpriseDisabled(t *testing.T) { } }) } + +func TestGlobalReadsAfterEnterpriseDisabled(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + defer utilccl.TestingEnableEnterprise()() + + _, sqlDB, cleanup := multiregionccltestutils.TestingCreateMultiRegionCluster( + t, 1 /* numServers */, base.TestingKnobs{}, nil, /* baseDir */ + ) + defer cleanup() + + for _, setupQuery := range []string{ + `CREATE DATABASE test`, + `USE test`, + `CREATE TABLE t1 ()`, + `CREATE TABLE t2 ()`, + } { + _, err := sqlDB.Exec(setupQuery) + require.NoError(t, err) + } + + // Can set global_reads with enterprise license enabled. + _, err := sqlDB.Exec(`ALTER TABLE t1 CONFIGURE ZONE USING global_reads = true`) + require.NoError(t, err) + + _, err = sqlDB.Exec(`ALTER TABLE t2 CONFIGURE ZONE USING global_reads = true`) + require.NoError(t, err) + + // Can unset global_reads with enterprise license enabled. + _, err = sqlDB.Exec(`ALTER TABLE t1 CONFIGURE ZONE USING global_reads = false`) + require.NoError(t, err) + + defer utilccl.TestingDisableEnterprise()() + + // Cannot set global_reads with enterprise license disabled. + _, err = sqlDB.Exec(`ALTER TABLE t1 CONFIGURE ZONE USING global_reads = true`) + require.Error(t, err) + require.Regexp(t, "use of global_reads requires an enterprise license", err) + + // Can unset global_reads with enterprise license disabled. + _, err = sqlDB.Exec(`ALTER TABLE t2 CONFIGURE ZONE USING global_reads = false`) + require.NoError(t, err) +} diff --git a/pkg/sql/logictest/testdata/logic_test/zone_config b/pkg/sql/logictest/testdata/logic_test/zone_config index c09367cb7473..6474b2d8bf14 100644 --- a/pkg/sql/logictest/testdata/logic_test/zone_config +++ b/pkg/sql/logictest/testdata/logic_test/zone_config @@ -75,7 +75,6 @@ ALTER TABLE a CONFIGURE ZONE USING range_min_bytes = 200000 + 1, range_max_bytes = 300000 + 1, gc.ttlseconds = 3000 + 600, - global_reads = true, num_replicas = floor(1.2)::int, constraints = '[+region=test]', lease_preferences = '[[+region=test]]' @@ -87,14 +86,12 @@ WHERE feature_name IN ( 'sql.schema.zone_config.table.range_min_bytes', 'sql.schema.zone_config.table.range_max_bytes', 'sql.schema.zone_config.table.gc.ttlseconds', - 'sql.schema.zone_config.table.global_reads', 'sql.schema.zone_config.table.num_replicas', 'sql.schema.zone_config.table.constraints' ) AND usage_count > 0 ORDER BY feature_name ---- sql.schema.zone_config.table.constraints sql.schema.zone_config.table.gc.ttlseconds -sql.schema.zone_config.table.global_reads sql.schema.zone_config.table.num_replicas sql.schema.zone_config.table.range_max_bytes sql.schema.zone_config.table.range_min_bytes @@ -106,7 +103,6 @@ SELECT zone_id, raw_config_sql FROM [SHOW ZONE CONFIGURATION FOR TABLE a] range_min_bytes = 200001, range_max_bytes = 300001, gc.ttlseconds = 3600, - global_reads = true, num_replicas = 1, constraints = '[+region=test]', lease_preferences = '[[+region=test]]' @@ -122,7 +118,6 @@ SELECT zone_id, raw_config_sql FROM [SHOW ZONE CONFIGURATION FOR TABLE a] range_min_bytes = 200001, range_max_bytes = 400000, gc.ttlseconds = 3600, - global_reads = true, num_replicas = 1, constraints = '[+region=test]', lease_preferences = '[[+region=test]]' @@ -159,7 +154,6 @@ SELECT zone_id, raw_config_sql FROM [SHOW ZONE CONFIGURATION FOR TABLE a] range_min_bytes = 200001, range_max_bytes = 400000, gc.ttlseconds = 3600, - global_reads = true, num_replicas = 1, constraints = '[+region=test]', lease_preferences = '[[+region=test]]' @@ -266,6 +260,10 @@ SELECT zone_id, raw_config_sql FROM [SHOW ZONE CONFIGURATION FOR TABLE a] voter_constraints = '{+region=test: 1}', lease_preferences = '[]' +# Check that global_reads cannot be set without a CCL binary and enterprise license. +statement error OSS binaries do not include enterprise features +ALTER TABLE a CONFIGURE ZONE USING global_reads = true + # Check entities for which we can set zone configs. subtest test_entity_validity diff --git a/pkg/sql/set_zone_config.go b/pkg/sql/set_zone_config.go index 88276b855908..a5e250fdb3a4 100644 --- a/pkg/sql/set_zone_config.go +++ b/pkg/sql/set_zone_config.go @@ -16,6 +16,7 @@ import ( "sort" "strings" + "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/config" "github.com/cockroachdb/cockroach/pkg/config/zonepb" @@ -63,37 +64,86 @@ type setZoneConfigNode struct { var supportedZoneConfigOptions = map[tree.Name]struct { requiredType *types.T setter func(*zonepb.ZoneConfig, tree.Datum) + checkAllowed func(context.Context, *ExecutorConfig, tree.Datum) error // optional }{ - "range_min_bytes": {types.Int, func(c *zonepb.ZoneConfig, d tree.Datum) { c.RangeMinBytes = proto.Int64(int64(tree.MustBeDInt(d))) }}, - "range_max_bytes": {types.Int, func(c *zonepb.ZoneConfig, d tree.Datum) { c.RangeMaxBytes = proto.Int64(int64(tree.MustBeDInt(d))) }}, - "global_reads": {types.Bool, func(c *zonepb.ZoneConfig, d tree.Datum) { c.GlobalReads = proto.Bool(bool(tree.MustBeDBool(d))) }}, - "num_replicas": {types.Int, func(c *zonepb.ZoneConfig, d tree.Datum) { c.NumReplicas = proto.Int32(int32(tree.MustBeDInt(d))) }}, - "num_voters": {types.Int, func(c *zonepb.ZoneConfig, d tree.Datum) { c.NumVoters = proto.Int32(int32(tree.MustBeDInt(d))) }}, - "gc.ttlseconds": {types.Int, func(c *zonepb.ZoneConfig, d tree.Datum) { - c.GC = &zonepb.GCPolicy{TTLSeconds: int32(tree.MustBeDInt(d))} - }}, - "constraints": {types.String, 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 - }}, - "voter_constraints": {types.String, 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.InheritedVoterConstraints = false - }}, - "lease_preferences": {types.String, func(c *zonepb.ZoneConfig, d tree.Datum) { - loadYAML(&c.LeasePreferences, string(tree.MustBeDString(d))) - c.InheritedLeasePreferences = false - }}, + "range_min_bytes": { + requiredType: types.Int, + setter: func(c *zonepb.ZoneConfig, d tree.Datum) { c.RangeMinBytes = proto.Int64(int64(tree.MustBeDInt(d))) }, + }, + "range_max_bytes": { + requiredType: types.Int, + setter: func(c *zonepb.ZoneConfig, d tree.Datum) { c.RangeMaxBytes = proto.Int64(int64(tree.MustBeDInt(d))) }, + }, + "global_reads": { + requiredType: types.Bool, + setter: func(c *zonepb.ZoneConfig, d tree.Datum) { c.GlobalReads = proto.Bool(bool(tree.MustBeDBool(d))) }, + checkAllowed: func(ctx context.Context, execCfg *ExecutorConfig, d tree.Datum) error { + if err := checkVersionActive(ctx, execCfg, clusterversion.NonVotingReplicas, "global_reads"); err != nil { + return err + } + if !tree.MustBeDBool(d) { + // Always allow the value to be unset. + return nil + } + return base.CheckEnterpriseEnabled( + execCfg.Settings, + execCfg.ClusterID(), + execCfg.Organization(), + "global_reads", + ) + }, + }, + "num_replicas": { + requiredType: types.Int, + setter: func(c *zonepb.ZoneConfig, d tree.Datum) { c.NumReplicas = proto.Int32(int32(tree.MustBeDInt(d))) }, + }, + "num_voters": { + requiredType: types.Int, + setter: func(c *zonepb.ZoneConfig, d tree.Datum) { c.NumVoters = proto.Int32(int32(tree.MustBeDInt(d))) }, + checkAllowed: func(ctx context.Context, execCfg *ExecutorConfig, _ tree.Datum) error { + return checkVersionActive(ctx, execCfg, clusterversion.NonVotingReplicas, "num_voters") + }, + }, + "gc.ttlseconds": { + requiredType: types.Int, + setter: func(c *zonepb.ZoneConfig, d tree.Datum) { + c.GC = &zonepb.GCPolicy{TTLSeconds: int32(tree.MustBeDInt(d))} + }, + }, + "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 + }, + }, + "voter_constraints": { + 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.InheritedVoterConstraints = false + }, + checkAllowed: func(ctx context.Context, execCfg *ExecutorConfig, _ tree.Datum) error { + return checkVersionActive(ctx, execCfg, clusterversion.NonVotingReplicas, "voter_constraints") + }, + }, + "lease_preferences": { + requiredType: types.String, + setter: func(c *zonepb.ZoneConfig, d tree.Datum) { + loadYAML(&c.LeasePreferences, string(tree.MustBeDString(d))) + c.InheritedLeasePreferences = false + }, + }, } // zoneOptionKeys contains the keys from suportedZoneConfigOptions in @@ -114,6 +164,16 @@ func loadYAML(dst interface{}, yamlString string) { } } +func checkVersionActive( + ctx context.Context, execCfg *ExecutorConfig, minVersion clusterversion.Key, option string, +) error { + if !execCfg.Settings.Version.IsActive(ctx, minVersion) { + return pgerror.Newf(pgcode.FeatureNotSupported, + "%s cannot be used until cluster version is finalized", option) + } + return nil +} + func (p *planner) SetZoneConfig(ctx context.Context, n *tree.SetZoneConfig) (planNode, error) { if err := checkSchemaChangeEnabled( ctx, @@ -182,11 +242,6 @@ func (p *planner) SetZoneConfig(ctx context.Context, n *tree.SetZoneConfig) (pla return nil, pgerror.Newf(pgcode.InvalidParameterValue, "unsupported zone config parameter: %q", tree.ErrString(&opt.Key)) } - if (opt.Key == "num_voters" || opt.Key == "voter_constraints") && - !p.ExecCfg().Settings.Version.IsActive(ctx, clusterversion.NonVotingReplicas) { - return nil, pgerror.Newf(pgcode.FeatureNotSupported, - "num_voters and voter_constraints cannot be used until cluster version is finalized") - } telemetry.Inc( sqltelemetry.SchemaSetZoneConfigCounter( n.ZoneSpecifier.TelemetryName(), @@ -334,7 +389,13 @@ func (n *setZoneConfigNode) startExec(params runParams) error { return pgerror.Newf(pgcode.InvalidParameterValue, "unsupported NULL value for %q", tree.ErrString(name)) } - setter := supportedZoneConfigOptions[*name].setter + opt := supportedZoneConfigOptions[*name] + if opt.checkAllowed != nil { + if err := opt.checkAllowed(params.ctx, params.ExecCfg(), datum); err != nil { + return err + } + } + setter := opt.setter setters = append(setters, func(c *zonepb.ZoneConfig) { setter(c, datum) }) optionsStr = append(optionsStr, fmt.Sprintf("%s = %s", name, datum)) }