diff --git a/pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs b/pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs index 372a9787a91f..3a46c1eba759 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs +++ b/pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs @@ -1106,9 +1106,6 @@ CREATE TABLE tbl2 ( statement ok SET override_multi_region_zone_config = true; -statement ok -SET use_declarative_schema_changer = off; - statement ok ALTER INDEX tbl2@tbl2_i_idx CONFIGURE ZONE USING num_replicas=10; diff --git a/pkg/ccl/logictestccl/testdata/logic_test/zone b/pkg/ccl/logictestccl/testdata/logic_test/zone index 38af8ae7605d..44b914634514 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/zone +++ b/pkg/ccl/logictestccl/testdata/logic_test/zone @@ -1354,7 +1354,7 @@ CREATE TABLE person ( ) ); -skipif config local-mixed-24.3 +skipif config local-mixed-24.3 local-legacy-schema-changer statement ok SET use_declarative_schema_changer = unsafe_always; diff --git a/pkg/ccl/partitionccl/drop_test.go b/pkg/ccl/partitionccl/drop_test.go index 618f74a52b72..30004e31e0c8 100644 --- a/pkg/ccl/partitionccl/drop_test.go +++ b/pkg/ccl/partitionccl/drop_test.go @@ -84,10 +84,11 @@ func TestDropIndexWithZoneConfigCCL(t *testing.T) { // Set zone configs on the primary index, secondary index, and one partition // of the secondary index. - ttlYaml := "gc: {ttlseconds: 1}" - sqlutils.SetZoneConfig(t, sqlDB, "INDEX t.kv@kv_pkey", "") - sqlutils.SetZoneConfig(t, sqlDB, "INDEX t.kv@i", ttlYaml) - sqlutils.SetZoneConfig(t, sqlDB, "PARTITION p2 OF INDEX t.kv@i", ttlYaml) + ttl := "gc.ttlseconds = 1" + sqlutils.SetZoneConfig(t, sqlDB, "INDEX t.kv@kv_pkey", + fmt.Sprintf("num_replicas = %d", *s.DefaultZoneConfig().NumReplicas)) + sqlutils.SetZoneConfig(t, sqlDB, "INDEX t.kv@i", ttl) + sqlutils.SetZoneConfig(t, sqlDB, "PARTITION p2 OF INDEX t.kv@i", ttl) // Drop the index and verify that the zone config for the secondary index and // its partition are removed but the zone config for the primary index diff --git a/pkg/ccl/partitionccl/zone_test.go b/pkg/ccl/partitionccl/zone_test.go index 0db9cc8283b1..90e3a61eb764 100644 --- a/pkg/ccl/partitionccl/zone_test.go +++ b/pkg/ccl/partitionccl/zone_test.go @@ -64,8 +64,8 @@ func TestValidIndexPartitionSetShowZones(t *testing.T) { PARTITION p1 VALUES IN (DEFAULT) )`) - yamlDefault := fmt.Sprintf("gc: {ttlseconds: %d}", s.DefaultZoneConfig().GC.TTLSeconds) - yamlOverride := "gc: {ttlseconds: 42}" + gcDefault := fmt.Sprintf("gc.ttlseconds = %d", s.DefaultZoneConfig().GC.TTLSeconds) + gcOverride := "gc.ttlseconds = 42" zoneOverride := s.DefaultZoneConfig() zoneOverride.GC = &zonepb.GCPolicy{TTLSeconds: 42} partialZoneOverride := *zonepb.NewZoneConfig() @@ -140,7 +140,7 @@ func TestValidIndexPartitionSetShowZones(t *testing.T) { // Ensure a database zone config applies to that database, its tables, and its // tables' indices and partitions. - sqlutils.SetZoneConfig(t, sqlDB, "DATABASE d", yamlOverride) + sqlutils.SetZoneConfig(t, sqlDB, "DATABASE d", gcOverride) sqlutils.VerifyAllZoneConfigs(t, sqlDB, defaultRow, partialDbRow) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "DATABASE d", dbRow) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "TABLE d.t", dbRow) @@ -150,7 +150,7 @@ func TestValidIndexPartitionSetShowZones(t *testing.T) { // Ensure a table zone config applies to that table and its indices and // partitions, but no other zones. - sqlutils.SetZoneConfig(t, sqlDB, "TABLE d.t", yamlOverride) + sqlutils.SetZoneConfig(t, sqlDB, "TABLE d.t", gcOverride) sqlutils.VerifyAllZoneConfigs(t, sqlDB, defaultRow, partialDbRow, partialTableRow) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "DATABASE d", dbRow) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "TABLE d.t", tableRow) @@ -160,7 +160,7 @@ func TestValidIndexPartitionSetShowZones(t *testing.T) { // Ensure an index zone config applies to that index and its partitions, but // no other zones. - sqlutils.SetZoneConfig(t, sqlDB, "INDEX d.t@t_pkey", yamlOverride) + sqlutils.SetZoneConfig(t, sqlDB, "INDEX d.t@t_pkey", gcOverride) sqlutils.VerifyAllZoneConfigs(t, sqlDB, defaultRow, partialDbRow, partialTableRow, partialPrimaryRow) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "DATABASE d", dbRow) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "TABLE d.t", tableRow) @@ -170,7 +170,7 @@ func TestValidIndexPartitionSetShowZones(t *testing.T) { // Ensure a partition zone config applies to that partition, but no other // zones. - sqlutils.SetZoneConfig(t, sqlDB, "PARTITION p0 OF TABLE d.t", yamlOverride) + sqlutils.SetZoneConfig(t, sqlDB, "PARTITION p0 OF TABLE d.t", gcOverride) sqlutils.VerifyAllZoneConfigs(t, sqlDB, defaultRow, partialDbRow, partialTableRow, partialPrimaryRow, partialP0Row) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "DATABASE d", dbRow) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "TABLE d.t", tableRow) @@ -180,7 +180,7 @@ func TestValidIndexPartitionSetShowZones(t *testing.T) { // Ensure updating the default zone propagates to zones without an override, // but not to those with overrides. - sqlutils.SetZoneConfig(t, sqlDB, "RANGE default", yamlOverride) + sqlutils.SetZoneConfig(t, sqlDB, "RANGE default", gcOverride) sqlutils.VerifyAllZoneConfigs(t, sqlDB, defaultOverrideRow, partialDbRow, partialTableRow, partialPrimaryRow, partialP0Row) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "DATABASE d", dbRow) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "TABLE d.t", tableRow) @@ -223,7 +223,7 @@ func TestValidIndexPartitionSetShowZones(t *testing.T) { // Ensure updating the default zone config applies to zones that have had // overrides added and removed. - sqlutils.SetZoneConfig(t, sqlDB, "RANGE default", yamlDefault) + sqlutils.SetZoneConfig(t, sqlDB, "RANGE default", gcDefault) sqlutils.VerifyAllZoneConfigs(t, sqlDB, defaultRow) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "RANGE default", defaultRow) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "DATABASE d", defaultRow) @@ -233,19 +233,19 @@ func TestValidIndexPartitionSetShowZones(t *testing.T) { sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "PARTITION p1 OF TABLE d.t", defaultRow) // Ensure subzones can be created even when no table zone exists. - sqlutils.SetZoneConfig(t, sqlDB, "PARTITION p0 OF TABLE d.t", yamlOverride) - sqlutils.SetZoneConfig(t, sqlDB, "PARTITION p1 OF TABLE d.t", yamlOverride) + sqlutils.SetZoneConfig(t, sqlDB, "PARTITION p0 OF TABLE d.t", gcOverride) + sqlutils.SetZoneConfig(t, sqlDB, "PARTITION p1 OF TABLE d.t", gcOverride) sqlutils.VerifyAllZoneConfigs(t, sqlDB, defaultRow, partialP0Row, partialP1Row) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "TABLE d.t", defaultRow) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "PARTITION p0 OF TABLE d.t", p0Row) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "PARTITION p1 OF TABLE d.t", p1Row) // Ensure the shorthand index syntax works. - sqlutils.SetZoneConfig(t, sqlDB, `INDEX "t_pkey"`, yamlOverride) + sqlutils.SetZoneConfig(t, sqlDB, `INDEX "t_pkey"`, gcOverride) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, `INDEX "t_pkey"`, primaryRow) // Ensure the session database is respected. - sqlutils.SetZoneConfig(t, sqlDB, "PARTITION p0 OF TABLE t", yamlOverride) + sqlutils.SetZoneConfig(t, sqlDB, "PARTITION p0 OF TABLE t", gcOverride) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "PARTITION p0 OF TABLE t", p0Row) } diff --git a/pkg/ccl/testutilsccl/BUILD.bazel b/pkg/ccl/testutilsccl/BUILD.bazel index c15eaf1ba979..1764726e27b4 100644 --- a/pkg/ccl/testutilsccl/BUILD.bazel +++ b/pkg/ccl/testutilsccl/BUILD.bazel @@ -17,6 +17,7 @@ go_library( "//pkg/sql", "//pkg/sql/execinfra", "//pkg/sql/sqltestutils", + "//pkg/testutils", "//pkg/testutils/serverutils", "//pkg/testutils/skip", "//pkg/util", diff --git a/pkg/ccl/testutilsccl/alter_primary_key.go b/pkg/ccl/testutilsccl/alter_primary_key.go index 08d59b10ebdb..771683d54382 100644 --- a/pkg/ccl/testutilsccl/alter_primary_key.go +++ b/pkg/ccl/testutilsccl/alter_primary_key.go @@ -18,7 +18,9 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/execinfra" "github.com/cockroachdb/cockroach/pkg/sql/sqltestutils" + "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" + "github.com/cockroachdb/cockroach/pkg/testutils/skip" "github.com/cockroachdb/cockroach/pkg/util" "github.com/stretchr/testify/require" ) @@ -115,9 +117,18 @@ USE t; // Insert some rows so we can interrupt inspect state during backfill. require.NoError(t, sqltestutils.BulkInsertIntoTable(sqlDB, maxValue)) - runCheck = true - _, err := sqlDB.Exec(tc.AlterQuery) - require.NoError(t, err) + testutils.RunTrueAndFalse(t, "uses-declarative-for-alter-table", + func(t *testing.T, useDeclarativeSchemaChangerForAlter bool) { + if useDeclarativeSchemaChangerForAlter { + skip.WithIssue(t, 136846) + } else { + _, err := sqlDB.Exec("SET CLUSTER SETTING sql.schema.force_declarative_statements = '!ALTER TABLE';") + require.NoError(t, err) + } + runCheck = true + _, err := sqlDB.Exec(tc.AlterQuery) + require.NoError(t, err) + }) }) } diff --git a/pkg/kv/kvserver/client_decommission_test.go b/pkg/kv/kvserver/client_decommission_test.go index ccf6cf46997f..8841fb92b05c 100644 --- a/pkg/kv/kvserver/client_decommission_test.go +++ b/pkg/kv/kvserver/client_decommission_test.go @@ -109,7 +109,7 @@ func TestDecommission(t *testing.T) { // off from the five-fold replicated system ranges. requireOnlyAtomicChanges(t, runner, tc.LookupRangeOrFatal(t, k).RangeID, triplicated, ts) - sqlutils.SetZoneConfig(t, runner, "RANGE default", "num_replicas: 1") + sqlutils.SetZoneConfig(t, runner, "RANGE default", "num_replicas = 1") const single = 1 diff --git a/pkg/sql/drop_index.go b/pkg/sql/drop_index.go index 42e9f1fe299c..69d13014da1f 100644 --- a/pkg/sql/drop_index.go +++ b/pkg/sql/drop_index.go @@ -345,26 +345,6 @@ func (p *planner) dropIndexByName( ) } - // Check if requires CCL binary for eventual zone config removal. - _, zone, _, err := GetZoneConfigInTxn( - ctx, p.txn, p.Descriptors(), tableDesc.ID, nil /* index */, "", false, - ) - if err != nil { - return err - } - - for _, s := range zone.Subzones { - if s.IndexID != uint32(idx.GetID()) { - _, err = GenerateSubzoneSpans(p.ExecCfg().Codec, tableDesc, zone.Subzones) - if sqlerrors.IsCCLRequiredError(err) { - return sqlerrors.NewCCLRequiredError(fmt.Errorf("schema change requires a CCL binary "+ - "because table %q has at least one remaining index or partition with a zone config", - tableDesc.Name)) - } - break - } - } - // Remove all foreign key references and backreferences from the index. // TODO (lucy): This is incorrect for two reasons: The first is that FKs won't // be restored if the DROP INDEX is rolled back, and the second is that diff --git a/pkg/sql/logictest/testdata/logic_test/zone_config b/pkg/sql/logictest/testdata/logic_test/zone_config index 5a62b5c5925f..e9c196d8eee4 100644 --- a/pkg/sql/logictest/testdata/logic_test/zone_config +++ b/pkg/sql/logictest/testdata/logic_test/zone_config @@ -417,7 +417,7 @@ subtest end subtest txn_zone_config -skipif config local-mixed-24.3 +skipif config local-mixed-24.3 local-legacy-schema-changer statement ok SET use_declarative_schema_changer = unsafe_always; @@ -450,3 +450,78 @@ statement ok RESET use_declarative_schema_changer; subtest end + +# Ensure that temp indexes created during a backfill can properly be +# zone-configured. +subtest zc_backfill + +statement ok +CREATE TABLE t(i INT, j INT NOT NULL, INDEX idx (j)) + +skipif config local-mixed-24.3 local-legacy-schema-changer +statement ok +SET use_declarative_schema_changer = unsafe_always; + +statement ok +BEGIN; +ALTER TABLE t ALTER PRIMARY KEY USING COLUMNS (j); +ALTER INDEX t@t_pkey CONFIGURE ZONE USING num_replicas = 11; +COMMIT; + +query I colnames +WITH subzones AS ( + SELECT + json_array_elements( + crdb_internal.pb_to_json('cockroach.config.zonepb.ZoneConfig', config) -> 'subzones' + ) AS config + FROM system.zones + WHERE id = 't'::REGCLASS::OID +), +subzone_configs AS ( + SELECT + (config -> 'config' ->> 'numReplicas')::INT AS replicas + FROM subzones +) +SELECT * +FROM subzone_configs +---- +replicas +11 + +skipif config local-mixed-24.3 local-legacy-schema-changer +# Ensure that the subzone config exists on the new primary index. The legacy +# schema changer does not do this; so, we skip those test configs. +query B colnames +WITH subzones AS ( + SELECT + json_array_elements( + crdb_internal.pb_to_json('cockroach.config.zonepb.ZoneConfig', config) -> 'subzones' + ) AS config + FROM system.zones + WHERE id = 't'::REGCLASS::OID +), +subzone_indexes AS ( + SELECT + (config -> 'indexId')::INT AS indexID + FROM subzones +), +primary_index AS ( + SELECT + (crdb_internal.pb_to_json( + 'cockroach.sql.sqlbase.Descriptor', + descriptor + )->'table'->'primaryIndex'->>'id')::INT AS primaryID + FROM system.descriptor + WHERE id = 't'::regclass::oid +) +SELECT + (primaryID = indexID) AS match_found +FROM primary_index, subzone_indexes; +---- +match_found +true + +statement ok +RESET use_declarative_schema_changer; + +subtest end diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go index 5a7a8037f9d7..7241d817f4ea 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go @@ -44,9 +44,6 @@ func alterTableAddColumn( b BuildCtx, tn *tree.TableName, tbl *scpb.Table, stmt tree.Statement, t *tree.AlterTableAddColumn, ) { d := t.ColumnDef - // We don't support handling zone config related properties for tables, so - // throw an unsupported error. - fallBackIfSubZoneConfigExists(b, t, tbl.TableID) fallBackIfRegionalByRowTable(b, t, tbl.TableID) // Check column non-existence. diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go index 7a9fdcae4766..8e64542b1389 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go @@ -72,7 +72,6 @@ func alterPrimaryKey( fallBackIfShardedIndexExists(b, t, tbl.TableID) fallBackIfPartitionedIndexExists(b, t, tbl.TableID) fallBackIfRegionalByRowTable(b, t.n, tbl.TableID) - fallBackIfSubZoneConfigExists(b, t.n, tbl.TableID) inflatedChain := getInflatedPrimaryIndexChain(b, tbl.TableID) if !haveSameIndexColsByKind(b, tbl.TableID, inflatedChain.oldSpec.primary.IndexID, diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go index 509355f8dac6..7f8482d54dcd 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go @@ -32,7 +32,6 @@ func alterTableDropColumn( stmt tree.Statement, n *tree.AlterTableDropColumn, ) { - fallBackIfSubZoneConfigExists(b, n, tbl.TableID) fallBackIfRegionalByRowTable(b, n, tbl.TableID) checkSafeUpdatesForDropColumn(b) checkRegionalByRowColumnConflict(b, tbl, n) diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go index 6502a1ef4ade..fc533bbd64af 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go @@ -148,12 +148,6 @@ func astToZoneConfigObject(b BuildCtx, n *tree.SetZoneConfig) (zoneConfigObject, return nil, scerrors.NotImplementedErrorf(n, "referencing an index without a table "+ "prefix is not supported in the DSC") } - - if !zs.TargetsTable() { - return nil, scerrors.NotImplementedErrorf(n, "zone configurations on system ranges "+ - "are not supported in the DSC") - } - // If this is an ALTER ALL PARTITIONS statement, fallback to the legacy schema // changer. if zs.TargetsPartition() && zs.StarIndex { diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_index.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_index.go index 01ab1ea80ebd..f829156e1ad9 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_index.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_index.go @@ -88,14 +88,15 @@ func maybeDropIndex( "use DROP CONSTRAINT ... PRIMARY KEY followed by ADD CONSTRAINT ... PRIMARY KEY in a transaction", )) } - // TODO (Xiang): Check if requires CCL binary for eventual zone config removal. _, _, sie := scpb.FindSecondaryIndex(toBeDroppedIndexElms) if sie == nil { panic(errors.AssertionFailedf("programming error: cannot find secondary index element.")) } - // We don't support handling zone config related properties for tables, so - // throw an unsupported error. - fallBackIfSubZoneConfigExists(b, nil, sie.TableID) + // TODO(136320): If a region is being added or dropped on this database + // concurrently, we have to block dropping an index for RBR tables. The logic + // for detecting a concurrent ADD/DROP region is not yet in the DSC; falling + // back for now. + fallBackIfRegionalByRowTable(b, nil, sie.TableID) panicIfSchemaChangeIsDisallowed(b.QueryByID(sie.TableID), n) // Cannot drop the index if not CASCADE and a unique constraint depends on it. if n.DropBehavior != tree.DropCascade && sie.IsUnique && !sie.IsCreatedExplicitly { diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go index 6b3753eefd39..8a24ba27de3f 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go @@ -862,24 +862,6 @@ func makeSwapIndexSpec( return in, temp } -// fallBackIfSubZoneConfigExists determines if the table has a subzone -// config. Normally this logic is used to limit index related operations, -// since dropping indexes will need to remove entries of sub zones from -// the zone config. -func fallBackIfSubZoneConfigExists(b BuildCtx, n tree.NodeFormatter, id catid.DescID) { - { - tableElts := b.QueryByID(id) - if _, _, elem := scpb.FindIndexZoneConfig(tableElts); elem != nil { - panic(scerrors.NotImplementedErrorf(n, - "sub zone configs are not supported")) - } - if _, _, elem := scpb.FindPartitionZoneConfig(tableElts); elem != nil { - panic(scerrors.NotImplementedErrorf(n, - "sub zone configs are not supported")) - } - } -} - // ExtractColumnIDsInExpr extracts column IDs used in expr. It's similar to // schemaexpr.ExtractColumnIDs but this function can also extract columns // added in the same transaction (e.g. for `ADD COLUMN j INT CHECK (j > 0);`, diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/index_zone_config.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/index_zone_config.go index ea3a78b65fe3..dfb4f98887d8 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/index_zone_config.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/index_zone_config.go @@ -410,7 +410,7 @@ func (izo *indexZoneConfigObj) fillIndexFromZoneSpecifier(b BuildCtx, zs tree.Zo indexID = primaryIndexElem.IndexID } else { indexElems := b.ResolveIndex(tableID, tree.Name(indexName), ResolveParams{}) - indexID = indexElems.FilterIndexName().MustGetOneElement().IndexID + indexID = indexElems.Filter(publicStatusFilter).FilterIndexName().MustGetOneElement().IndexID } izo.indexID = indexID } diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/process.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/process.go index 953666c1f811..21ebb48adaa2 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/process.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/process.go @@ -37,34 +37,36 @@ type supportedStatement struct { // Tracks operations which are fully supported when the declarative schema // changer is enabled. Operations marked as non-fully supported can only be // with the use_declarative_schema_changer session variable. +// +// Please keep this list alphabetized for easier navigation. var supportedStatements = map[reflect.Type]supportedStatement{ // Alter table will have commands individually whitelisted via the - // supportedAlterTableStatements list, so wwe will consider it fully supported + // supportedAlterTableStatements list, so we will consider it fully supported // here. reflect.TypeOf((*tree.AlterTable)(nil)): {fn: AlterTable, statementTags: []string{tree.AlterTableTag}, on: true, checks: alterTableChecks}, reflect.TypeOf((*tree.CreateIndex)(nil)): {fn: CreateIndex, statementTags: []string{tree.CreateIndexTag}, on: true, checks: nil}, - reflect.TypeOf((*tree.DropDatabase)(nil)): {fn: DropDatabase, statementTags: []string{tree.DropDatabaseTag}, on: true, checks: nil}, - reflect.TypeOf((*tree.DropOwnedBy)(nil)): {fn: DropOwnedBy, statementTags: []string{tree.DropOwnedByTag}, on: true, checks: nil}, - reflect.TypeOf((*tree.DropSchema)(nil)): {fn: DropSchema, statementTags: []string{tree.DropSchemaTag}, on: true, checks: nil}, - reflect.TypeOf((*tree.DropSequence)(nil)): {fn: DropSequence, statementTags: []string{tree.DropSequenceTag}, on: true, checks: nil}, - reflect.TypeOf((*tree.DropTable)(nil)): {fn: DropTable, statementTags: []string{tree.DropTableTag}, on: true, checks: nil}, - reflect.TypeOf((*tree.DropType)(nil)): {fn: DropType, statementTags: []string{tree.DropTypeTag}, on: true, checks: nil}, - reflect.TypeOf((*tree.DropView)(nil)): {fn: DropView, statementTags: []string{tree.DropViewTag}, on: true, checks: nil}, + reflect.TypeOf((*tree.CommentOnColumn)(nil)): {fn: CommentOnColumn, statementTags: []string{tree.CommentOnColumnTag}, on: true, checks: nil}, reflect.TypeOf((*tree.CommentOnConstraint)(nil)): {fn: CommentOnConstraint, statementTags: []string{tree.CommentOnConstraintTag}, on: true, checks: nil}, reflect.TypeOf((*tree.CommentOnDatabase)(nil)): {fn: CommentOnDatabase, statementTags: []string{tree.CommentOnDatabaseTag}, on: true, checks: nil}, + reflect.TypeOf((*tree.CommentOnIndex)(nil)): {fn: CommentOnIndex, statementTags: []string{tree.CommentOnIndexTag}, on: true, checks: nil}, reflect.TypeOf((*tree.CommentOnSchema)(nil)): {fn: CommentOnSchema, statementTags: []string{tree.CommentOnSchemaTag}, on: true, checks: nil}, reflect.TypeOf((*tree.CommentOnTable)(nil)): {fn: CommentOnTable, statementTags: []string{tree.CommentOnTableTag}, on: true, checks: nil}, reflect.TypeOf((*tree.CommentOnType)(nil)): {fn: CommentOnType, statementTags: []string{tree.CommentOnTypeTag}, on: true, checks: nil}, - reflect.TypeOf((*tree.CommentOnColumn)(nil)): {fn: CommentOnColumn, statementTags: []string{tree.CommentOnColumnTag}, on: true, checks: nil}, - reflect.TypeOf((*tree.CommentOnIndex)(nil)): {fn: CommentOnIndex, statementTags: []string{tree.CommentOnIndexTag}, on: true, checks: nil}, - reflect.TypeOf((*tree.DropIndex)(nil)): {fn: DropIndex, statementTags: []string{tree.DropIndexTag}, on: true, checks: nil}, - reflect.TypeOf((*tree.DropRoutine)(nil)): {fn: DropFunction, statementTags: []string{tree.DropFunctionTag, tree.DropProcedureTag}, on: true, checks: nil}, + reflect.TypeOf((*tree.CreateDatabase)(nil)): {fn: CreateDatabase, statementTags: []string{tree.CreateDatabaseTag}, on: true, checks: nil}, reflect.TypeOf((*tree.CreateRoutine)(nil)): {fn: CreateFunction, statementTags: []string{tree.CreateFunctionTag, tree.CreateProcedureTag}, on: true, checks: nil}, reflect.TypeOf((*tree.CreateSchema)(nil)): {fn: CreateSchema, statementTags: []string{tree.CreateSchemaTag}, on: true, checks: nil}, reflect.TypeOf((*tree.CreateSequence)(nil)): {fn: CreateSequence, statementTags: []string{tree.CreateSequenceTag}, on: true, checks: nil}, - reflect.TypeOf((*tree.CreateDatabase)(nil)): {fn: CreateDatabase, statementTags: []string{tree.CreateDatabaseTag}, on: true, checks: nil}, reflect.TypeOf((*tree.CreateTrigger)(nil)): {fn: CreateTrigger, statementTags: []string{tree.CreateTriggerTag}, on: true, checks: isV243Active}, + reflect.TypeOf((*tree.DropDatabase)(nil)): {fn: DropDatabase, statementTags: []string{tree.DropDatabaseTag}, on: true, checks: nil}, + reflect.TypeOf((*tree.DropRoutine)(nil)): {fn: DropFunction, statementTags: []string{tree.DropFunctionTag, tree.DropProcedureTag}, on: true, checks: nil}, + reflect.TypeOf((*tree.DropIndex)(nil)): {fn: DropIndex, statementTags: []string{tree.DropIndexTag}, on: true, checks: nil}, + reflect.TypeOf((*tree.DropOwnedBy)(nil)): {fn: DropOwnedBy, statementTags: []string{tree.DropOwnedByTag}, on: true, checks: nil}, + reflect.TypeOf((*tree.DropSchema)(nil)): {fn: DropSchema, statementTags: []string{tree.DropSchemaTag}, on: true, checks: nil}, + reflect.TypeOf((*tree.DropSequence)(nil)): {fn: DropSequence, statementTags: []string{tree.DropSequenceTag}, on: true, checks: nil}, + reflect.TypeOf((*tree.DropTable)(nil)): {fn: DropTable, statementTags: []string{tree.DropTableTag}, on: true, checks: nil}, reflect.TypeOf((*tree.DropTrigger)(nil)): {fn: DropTrigger, statementTags: []string{tree.DropTriggerTag}, on: true, checks: isV243Active}, + reflect.TypeOf((*tree.DropType)(nil)): {fn: DropType, statementTags: []string{tree.DropTypeTag}, on: true, checks: nil}, + reflect.TypeOf((*tree.DropView)(nil)): {fn: DropView, statementTags: []string{tree.DropViewTag}, on: true, checks: nil}, reflect.TypeOf((*tree.SetZoneConfig)(nil)): {fn: SetZoneConfig, statementTags: []string{tree.ConfigureZoneTag}, on: true, checks: isV251Active}, } diff --git a/pkg/sql/schemachanger/scbuild/testdata/unimplemented_zone_cfg b/pkg/sql/schemachanger/scbuild/testdata/unimplemented_zone_cfg deleted file mode 100644 index d3e17269787e..000000000000 --- a/pkg/sql/schemachanger/scbuild/testdata/unimplemented_zone_cfg +++ /dev/null @@ -1,43 +0,0 @@ -setup -SET experimental_enable_unique_without_index_constraints = true; -CREATE TABLE defaultdb.foo ( - i INT8 PRIMARY KEY, - -- j gets added by the test - k INT8 CHECK (k > 10), - l INT8 NOT NULL UNIQUE, - m INT8 REFERENCES foo (l), - n UUID, - UNIQUE WITHOUT INDEX (n), - o INT -- this column can be dropped -); -ALTER TABLE defaultdb.foo CONFIGURE ZONE USING gc.ttlseconds=10; -CREATE INDEX idx ON defaultdb.foo(k, l); -CREATE TABLE defaultdb.foo_index_zone_cfg ( - i INT8 PRIMARY KEY, - -- j gets added by the test - k INT8 CHECK (k > 10), - l INT8 NOT NULL UNIQUE, - m INT8 REFERENCES foo (l), - n UUID, - UNIQUE WITHOUT INDEX (n), - o INT -- this column can be dropped -); -CREATE INDEX idx ON defaultdb.foo_index_zone_cfg(k, l); -ALTER INDEX defaultdb.foo_index_zone_cfg@idx CONFIGURE ZONE USING gc.ttlseconds=10; ----- - -unimplemented -ALTER TABLE defaultdb.foo_index_zone_cfg ADD COLUMN j INT ----- - -unimplemented -ALTER TABLE defaultdb.foo_index_zone_cfg ADD PRIMARY KEY (i, n) ----- - -unimplemented -ALTER TABLE defaultdb.foo_index_zone_cfg DROP COLUMN k ----- - -unimplemented -DROP INDEX defaultdb.foo_index_zone_cfg@idx ----- diff --git a/pkg/sql/schemachanger/scbuild/testdata/zone_cfg b/pkg/sql/schemachanger/scbuild/testdata/zone_cfg new file mode 100644 index 000000000000..b94391945953 --- /dev/null +++ b/pkg/sql/schemachanger/scbuild/testdata/zone_cfg @@ -0,0 +1,153 @@ +setup +SET experimental_enable_unique_without_index_constraints = true; +CREATE TABLE defaultdb.foo ( + i INT8 PRIMARY KEY, + -- j gets added by the test + k INT8 CHECK (k > 10), + l INT8 NOT NULL UNIQUE, + m INT8 REFERENCES foo (l), + n UUID, + UNIQUE WITHOUT INDEX (n), + o INT -- this column can be dropped +); +ALTER TABLE defaultdb.foo CONFIGURE ZONE USING gc.ttlseconds=10; +CREATE INDEX idx ON defaultdb.foo(k, l); +CREATE TABLE defaultdb.foo_index_zone_cfg ( + i INT8 PRIMARY KEY, + -- j gets added by the test + k INT8 CHECK (k > 10), + l INT8 NOT NULL UNIQUE, + m INT8 REFERENCES foo (l), + n UUID, + UNIQUE WITHOUT INDEX (n), + o INT -- this column can be dropped +); +CREATE INDEX idx ON defaultdb.foo_index_zone_cfg(k, l); +ALTER INDEX defaultdb.foo_index_zone_cfg@idx CONFIGURE ZONE USING gc.ttlseconds=10; +---- + +build +ALTER TABLE defaultdb.foo_index_zone_cfg ADD COLUMN j INT +---- +- [[IndexData:{DescID: 105, IndexID: 1}, PUBLIC], PUBLIC] + {indexId: 1, tableId: 105} +- [[IndexData:{DescID: 105, IndexID: 2}, PUBLIC], PUBLIC] + {indexId: 2, tableId: 105} +- [[IndexData:{DescID: 105, IndexID: 3}, PUBLIC], PUBLIC] + {indexId: 3, tableId: 105} +- [[TableData:{DescID: 105, ReferencedDescID: 100}, PUBLIC], PUBLIC] + {databaseId: 100, tableId: 105} +- [[Column:{DescID: 105, ColumnID: 7}, PUBLIC], ABSENT] + {columnId: 7, tableId: 105} +- [[ColumnName:{DescID: 105, Name: j, ColumnID: 7}, PUBLIC], ABSENT] + {columnId: 7, name: j, tableId: 105} +- [[ColumnType:{DescID: 105, ColumnFamilyID: 0, ColumnID: 7, TypeName: INT8}, PUBLIC], ABSENT] + {columnId: 7, elementCreationMetadata: {in231OrLater: true, in243OrLater: true}, isNullable: true, tableId: 105, type: {family: IntFamily, oid: 20, width: 64}, typeName: INT8} +- [[IndexColumn:{DescID: 105, ColumnID: 7, IndexID: 1}, PUBLIC], ABSENT] + {columnId: 7, indexId: 1, kind: STORED, ordinalInKind: 5, tableId: 105} + +unimplemented +ALTER TABLE defaultdb.foo_index_zone_cfg ADD PRIMARY KEY (i, n) +---- + +build +ALTER TABLE defaultdb.foo_index_zone_cfg DROP COLUMN k +---- +- [[Column:{DescID: 105, ColumnID: 2}, ABSENT], PUBLIC] + {columnId: 2, tableId: 105} +- [[ColumnName:{DescID: 105, Name: k, ColumnID: 2}, ABSENT], PUBLIC] + {columnId: 2, name: k, tableId: 105} +- [[ColumnType:{DescID: 105, ColumnFamilyID: 0, ColumnID: 2, TypeName: INT8}, ABSENT], PUBLIC] + {columnId: 2, elementCreationMetadata: {in231OrLater: true, in243OrLater: true}, isNullable: true, tableId: 105, type: {family: IntFamily, oid: 20, width: 64}, typeName: INT8} +- [[IndexColumn:{DescID: 105, ColumnID: 1, IndexID: 1}, ABSENT], PUBLIC] + {columnId: 1, indexId: 1, tableId: 105} +- [[IndexColumn:{DescID: 105, ColumnID: 2, IndexID: 1}, ABSENT], PUBLIC] + {columnId: 2, indexId: 1, kind: STORED, tableId: 105} +- [[IndexColumn:{DescID: 105, ColumnID: 3, IndexID: 1}, ABSENT], PUBLIC] + {columnId: 3, indexId: 1, kind: STORED, ordinalInKind: 1, tableId: 105} +- [[IndexColumn:{DescID: 105, ColumnID: 4, IndexID: 1}, ABSENT], PUBLIC] + {columnId: 4, indexId: 1, kind: STORED, ordinalInKind: 2, tableId: 105} +- [[IndexColumn:{DescID: 105, ColumnID: 5, IndexID: 1}, ABSENT], PUBLIC] + {columnId: 5, indexId: 1, kind: STORED, ordinalInKind: 3, tableId: 105} +- [[IndexColumn:{DescID: 105, ColumnID: 6, IndexID: 1}, ABSENT], PUBLIC] + {columnId: 6, indexId: 1, kind: STORED, ordinalInKind: 4, tableId: 105} +- [[PrimaryIndex:{DescID: 105, IndexID: 1, ConstraintID: 2}, ABSENT], PUBLIC] + {constraintId: 2, indexId: 1, isUnique: true, tableId: 105} +- [[IndexName:{DescID: 105, Name: foo_index_zone_cfg_pkey, IndexID: 1}, ABSENT], PUBLIC] + {indexId: 1, name: foo_index_zone_cfg_pkey, tableId: 105} +- [[IndexData:{DescID: 105, IndexID: 1}, ABSENT], PUBLIC] + {indexId: 1, tableId: 105} +- [[IndexData:{DescID: 105, IndexID: 2}, PUBLIC], PUBLIC] + {indexId: 2, tableId: 105} +- [[IndexColumn:{DescID: 105, ColumnID: 2, IndexID: 3}, ABSENT], PUBLIC] + {columnId: 2, indexId: 3, tableId: 105} +- [[IndexColumn:{DescID: 105, ColumnID: 3, IndexID: 3}, ABSENT], PUBLIC] + {columnId: 3, indexId: 3, ordinalInKind: 1, tableId: 105} +- [[IndexColumn:{DescID: 105, ColumnID: 1, IndexID: 3}, ABSENT], PUBLIC] + {columnId: 1, indexId: 3, kind: KEY_SUFFIX, tableId: 105} +- [[SecondaryIndex:{DescID: 105, IndexID: 3, ConstraintID: 0, RecreateSourceIndexID: 0}, ABSENT], PUBLIC] + {indexId: 3, isCreatedExplicitly: true, tableId: 105} +- [[IndexName:{DescID: 105, Name: idx, IndexID: 3}, ABSENT], PUBLIC] + {indexId: 3, name: idx, tableId: 105} +- [[IndexData:{DescID: 105, IndexID: 3}, ABSENT], PUBLIC] + {indexId: 3, tableId: 105} +- [[CheckConstraint:{DescID: 105, IndexID: 0, ConstraintID: 4, ReferencedColumnIDs: [2]}, ABSENT], PUBLIC] + {columnIds: [2], constraintId: 4, expr: 'k > 10:::INT8', referencedColumnIds: [2], tableId: 105} +- [[IndexZoneConfig:{DescID: 105, IndexID: 3, SeqNum: 0}, ABSENT], PUBLIC] + {indexId: 3, oldIdxRef: -1, subzone: {config: {gc: {ttlSeconds: 10}, inheritedConstraints: true, inheritedLeasePreferences: true}, indexId: 3}, subzoneSpans: [{key: iw==}], tableId: 105} +- [[TableData:{DescID: 105, ReferencedDescID: 100}, PUBLIC], PUBLIC] + {databaseId: 100, tableId: 105} +- [[PrimaryIndex:{DescID: 105, IndexID: 5, ConstraintID: 6, TemporaryIndexID: 6, SourceIndexID: 1}, PUBLIC], ABSENT] + {constraintId: 6, indexId: 5, isUnique: true, sourceIndexId: 1, tableId: 105, temporaryIndexId: 6} +- [[IndexName:{DescID: 105, Name: foo_index_zone_cfg_pkey, IndexID: 5}, PUBLIC], ABSENT] + {indexId: 5, name: foo_index_zone_cfg_pkey, tableId: 105} +- [[IndexColumn:{DescID: 105, ColumnID: 1, IndexID: 5}, PUBLIC], ABSENT] + {columnId: 1, indexId: 5, tableId: 105} +- [[IndexColumn:{DescID: 105, ColumnID: 3, IndexID: 5}, PUBLIC], ABSENT] + {columnId: 3, indexId: 5, kind: STORED, tableId: 105} +- [[IndexColumn:{DescID: 105, ColumnID: 4, IndexID: 5}, PUBLIC], ABSENT] + {columnId: 4, indexId: 5, kind: STORED, ordinalInKind: 1, tableId: 105} +- [[IndexColumn:{DescID: 105, ColumnID: 5, IndexID: 5}, PUBLIC], ABSENT] + {columnId: 5, indexId: 5, kind: STORED, ordinalInKind: 2, tableId: 105} +- [[IndexColumn:{DescID: 105, ColumnID: 6, IndexID: 5}, PUBLIC], ABSENT] + {columnId: 6, indexId: 5, kind: STORED, ordinalInKind: 3, tableId: 105} +- [[IndexData:{DescID: 105, IndexID: 5}, PUBLIC], ABSENT] + {indexId: 5, tableId: 105} +- [[TemporaryIndex:{DescID: 105, IndexID: 6, ConstraintID: 7, SourceIndexID: 1}, TRANSIENT_ABSENT], ABSENT] + {constraintId: 7, indexId: 6, isUnique: true, sourceIndexId: 1, tableId: 105} +- [[IndexColumn:{DescID: 105, ColumnID: 1, IndexID: 6}, TRANSIENT_ABSENT], ABSENT] + {columnId: 1, indexId: 6, tableId: 105} +- [[IndexColumn:{DescID: 105, ColumnID: 3, IndexID: 6}, TRANSIENT_ABSENT], ABSENT] + {columnId: 3, indexId: 6, kind: STORED, tableId: 105} +- [[IndexColumn:{DescID: 105, ColumnID: 4, IndexID: 6}, TRANSIENT_ABSENT], ABSENT] + {columnId: 4, indexId: 6, kind: STORED, ordinalInKind: 1, tableId: 105} +- [[IndexColumn:{DescID: 105, ColumnID: 5, IndexID: 6}, TRANSIENT_ABSENT], ABSENT] + {columnId: 5, indexId: 6, kind: STORED, ordinalInKind: 2, tableId: 105} +- [[IndexColumn:{DescID: 105, ColumnID: 6, IndexID: 6}, TRANSIENT_ABSENT], ABSENT] + {columnId: 6, indexId: 6, kind: STORED, ordinalInKind: 3, tableId: 105} +- [[IndexData:{DescID: 105, IndexID: 6}, TRANSIENT_ABSENT], ABSENT] + {indexId: 6, tableId: 105} + +build +DROP INDEX defaultdb.foo_index_zone_cfg@idx +---- +- [[IndexData:{DescID: 105, IndexID: 1}, PUBLIC], PUBLIC] + {indexId: 1, tableId: 105} +- [[IndexData:{DescID: 105, IndexID: 2}, PUBLIC], PUBLIC] + {indexId: 2, tableId: 105} +- [[IndexColumn:{DescID: 105, ColumnID: 2, IndexID: 3}, ABSENT], PUBLIC] + {columnId: 2, indexId: 3, tableId: 105} +- [[IndexColumn:{DescID: 105, ColumnID: 3, IndexID: 3}, ABSENT], PUBLIC] + {columnId: 3, indexId: 3, ordinalInKind: 1, tableId: 105} +- [[IndexColumn:{DescID: 105, ColumnID: 1, IndexID: 3}, ABSENT], PUBLIC] + {columnId: 1, indexId: 3, kind: KEY_SUFFIX, tableId: 105} +- [[SecondaryIndex:{DescID: 105, IndexID: 3, ConstraintID: 0, RecreateSourceIndexID: 0}, ABSENT], PUBLIC] + {indexId: 3, isCreatedExplicitly: true, tableId: 105} +- [[IndexName:{DescID: 105, Name: idx, IndexID: 3}, ABSENT], PUBLIC] + {indexId: 3, name: idx, tableId: 105} +- [[IndexData:{DescID: 105, IndexID: 3}, ABSENT], PUBLIC] + {indexId: 3, tableId: 105} +- [[IndexZoneConfig:{DescID: 105, IndexID: 3, SeqNum: 0}, ABSENT], PUBLIC] + {indexId: 3, oldIdxRef: -1, subzone: {config: {gc: {ttlSeconds: 10}, inheritedConstraints: true, inheritedLeasePreferences: true}, indexId: 3}, subzoneSpans: [{key: iw==}], tableId: 105} +- [[TableData:{DescID: 105, ReferencedDescID: 100}, PUBLIC], PUBLIC] + {databaseId: 100, tableId: 105} diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/op_funcs.go b/pkg/sql/schemachanger/scplan/internal/opgen/op_funcs.go index 4b02f9b25cc8..34914fcd502d 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/op_funcs.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/op_funcs.go @@ -192,14 +192,55 @@ func checkIfDescriptorIsWithoutData(id descpb.ID, md *opGenContext) bool { return !doesDescriptorHaveData } -// checkIfDescriptorHasGCDependents will determine if a descriptor has data -// dependencies it still needs to GC. This allows us to determine when we can -// need to skip certain operations like deleting a zone config. -func checkIfDescriptorHasGCDependents(_ descpb.ID, md *opGenContext) bool { - for _, t := range md.Targets { - switch t.Element().(type) { - case *scpb.IndexData, *scpb.TableData: - return true +// checkIfZoneConfigHasGCDependents will determine if a table/database +// descriptor has data dependencies it still needs to GC. This allows us to +// determine when we need to skip certain operations like deleting a zone +// config. +func checkIfZoneConfigHasGCDependents(elem scpb.ZoneConfigElement, md *opGenContext) bool { + isValidTableData := func(td *scpb.TableData) bool { + switch e := elem.(type) { + case *scpb.DatabaseZoneConfig: + return e.DatabaseID == td.DatabaseID + case *scpb.TableZoneConfig: + return e.TableID == td.TableID + default: + panic(errors.AssertionFailedf( + "element type %T not allowed for checkIfZoneConfigHasGCDependents", e)) + } + } + for idx, t := range md.Targets { + switch e := t.Element().(type) { + case *scpb.TableData: + // Filter out any elements we do not want to consider. + if !isValidTableData(e) { + continue + } + // Check if this descriptor has any data marked for an absent state. + if t.TargetStatus == scpb.Status_ABSENT && + md.Initial[idx] == scpb.Status_PUBLIC { + return true + } + } + } + return false +} + +// checkIfIndexHasGCDependents is like checkIfZoneConfigHasGCDependents, but +// for indexes. We also ensure that we filter out irrelevant indexes here. +func checkIfIndexHasGCDependents(tableID descpb.ID, md *opGenContext) bool { + for idx, t := range md.Targets { + switch e := t.Element().(type) { + case *scpb.IndexData: + // Validate this is the table ID we are + // looking for. + if e.TableID != tableID { + continue + } + // Check if this descriptor has any data marked for an absent state. + if t.TargetStatus == scpb.Status_ABSENT && + md.Initial[idx] == scpb.Status_PUBLIC { + return true + } } } return false diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_database_zone_config.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_database_zone_config.go index d2b7d5d61800..74900e6d7444 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_database_zone_config.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_database_zone_config.go @@ -27,10 +27,9 @@ func init() { scpb.Status_PUBLIC, to(scpb.Status_ABSENT, emit(func(this *scpb.DatabaseZoneConfig, md *opGenContext) *scop.DiscardZoneConfig { - // If we are dropping a database with table dependencies, let the GC - // job take care of dropping this zone config -- as its table - // dependencies need this zone config. - if checkIfDescriptorHasGCDependents(this.DatabaseID, md) { + // If this belongs to a drop instead of a CONFIGURE ZONE DISCARD, let + // the GC job take care of dropping the zone config. + if checkIfZoneConfigHasGCDependents(this, md) { return nil } return &scop.DiscardZoneConfig{ diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_index_zone_config.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_index_zone_config.go index 369a6824c29a..d05e1f19a850 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_index_zone_config.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_index_zone_config.go @@ -28,7 +28,12 @@ func init() { toAbsent( scpb.Status_PUBLIC, to(scpb.Status_ABSENT, - emit(func(this *scpb.IndexZoneConfig) *scop.DiscardSubzoneConfig { + emit(func(this *scpb.IndexZoneConfig, md *opGenContext) *scop.DiscardSubzoneConfig { + // If this belongs to a drop instead of a CONFIGURE ZONE DISCARD, let + // the GC job take care of dropping the zone config. + if checkIfIndexHasGCDependents(this.TableID, md) { + return nil + } return &scop.DiscardSubzoneConfig{ TableID: this.TableID, Subzone: this.Subzone, diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_partition_zone_config.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_partition_zone_config.go index 9139abf35c53..e2e2e61ee474 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_partition_zone_config.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_partition_zone_config.go @@ -28,7 +28,12 @@ func init() { toAbsent( scpb.Status_PUBLIC, to(scpb.Status_ABSENT, - emit(func(this *scpb.PartitionZoneConfig) *scop.DiscardSubzoneConfig { + emit(func(this *scpb.PartitionZoneConfig, md *opGenContext) *scop.DiscardSubzoneConfig { + // If this belongs to a drop instead of a CONFIGURE ZONE DISCARD, let + // the GC job take care of dropping the zone config. + if checkIfIndexHasGCDependents(this.TableID, md) { + return nil + } return &scop.DiscardSubzoneConfig{ TableID: this.TableID, Subzone: this.Subzone, diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_zone_config.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_zone_config.go index bef346f6cef3..fa810e488978 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_zone_config.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_zone_config.go @@ -27,6 +27,11 @@ func init() { scpb.Status_PUBLIC, to(scpb.Status_ABSENT, emit(func(this *scpb.TableZoneConfig, md *opGenContext) *scop.DiscardTableZoneConfig { + // If this belongs to a drop instead of a CONFIGURE ZONE DISCARD, let + // the GC job take care of dropping the zone config. + if checkIfZoneConfigHasGCDependents(this, md) { + return nil + } return &scop.DiscardTableZoneConfig{ TableID: this.TableID, ZoneConfig: *this.ZoneConfig, diff --git a/pkg/sql/zone_test.go b/pkg/sql/zone_test.go index c75da6089752..f0bd2a02bd98 100644 --- a/pkg/sql/zone_test.go +++ b/pkg/sql/zone_test.go @@ -33,8 +33,8 @@ func TestValidSetShowZones(t *testing.T) { sqlDB := sqlutils.MakeSQLRunner(db) sqlDB.Exec(t, `CREATE DATABASE d; USE d; CREATE TABLE t ();`) - yamlDefault := fmt.Sprintf("gc: {ttlseconds: %d}", s.DefaultZoneConfig().GC.TTLSeconds) - yamlOverride := "gc: {ttlseconds: 42}" + gcDefault := fmt.Sprintf("gc.ttlseconds = %d", s.DefaultZoneConfig().GC.TTLSeconds) + gcOverride := "gc.ttlseconds = 42" zoneOverride := s.DefaultZoneConfig() zoneOverride.GC = &zonepb.GCPolicy{TTLSeconds: 42} partialZoneOverride := *zonepb.NewZoneConfig() @@ -110,7 +110,7 @@ func TestValidSetShowZones(t *testing.T) { // Ensure a database zone config applies to that database and its tables, and // no other zones. - sqlutils.SetZoneConfig(t, sqlDB, "DATABASE d", yamlOverride) + sqlutils.SetZoneConfig(t, sqlDB, "DATABASE d", gcOverride) sqlutils.VerifyAllZoneConfigs(t, sqlDB, defaultRow, partialDbRow) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "RANGE meta", defaultRow) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "DATABASE system", defaultRow) @@ -119,7 +119,7 @@ func TestValidSetShowZones(t *testing.T) { sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "TABLE d.t", dbRow) // Ensure a table zone config applies to that table and no others. - sqlutils.SetZoneConfig(t, sqlDB, "TABLE d.t", yamlOverride) + sqlutils.SetZoneConfig(t, sqlDB, "TABLE d.t", gcOverride) sqlutils.VerifyAllZoneConfigs(t, sqlDB, defaultRow, partialDbRow, partialTableRow) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "RANGE meta", defaultRow) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "DATABASE system", defaultRow) @@ -128,7 +128,7 @@ func TestValidSetShowZones(t *testing.T) { sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "TABLE d.t", tableRow) // Ensure a named zone config applies to that named zone and no others. - sqlutils.SetZoneConfig(t, sqlDB, "RANGE meta", yamlOverride) + sqlutils.SetZoneConfig(t, sqlDB, "RANGE meta", gcOverride) sqlutils.VerifyAllZoneConfigs(t, sqlDB, defaultRow, partialMetaRow, partialDbRow, partialTableRow) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "RANGE meta", metaRow) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "DATABASE system", defaultRow) @@ -138,7 +138,7 @@ func TestValidSetShowZones(t *testing.T) { // Ensure updating the default zone propagates to zones without an override, // but not to those with overrides. - sqlutils.SetZoneConfig(t, sqlDB, "RANGE default", yamlOverride) + sqlutils.SetZoneConfig(t, sqlDB, "RANGE default", gcOverride) sqlutils.VerifyAllZoneConfigs(t, sqlDB, defaultOverrideRow, partialMetaRow, partialDbRow, partialTableRow) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "RANGE meta", metaRow) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "DATABASE system", defaultOverrideRow) @@ -170,7 +170,7 @@ func TestValidSetShowZones(t *testing.T) { // Ensure updating the default zone config applies to zones that have had // overrides added and removed. - sqlutils.SetZoneConfig(t, sqlDB, "RANGE default", yamlDefault) + sqlutils.SetZoneConfig(t, sqlDB, "RANGE default", gcDefault) sqlutils.VerifyAllZoneConfigs(t, sqlDB, defaultRow) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "RANGE default", defaultRow) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "RANGE meta", defaultRow) @@ -181,7 +181,7 @@ func TestValidSetShowZones(t *testing.T) { // Ensure the system database zone can be configured, even though zones on // config tables are disallowed. - sqlutils.SetZoneConfig(t, sqlDB, "DATABASE system", yamlOverride) + sqlutils.SetZoneConfig(t, sqlDB, "DATABASE system", gcOverride) sqlutils.VerifyAllZoneConfigs(t, sqlDB, defaultRow, partialSystemRow) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "DATABASE system", systemRow) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "TABLE system.namespace", systemRow) @@ -189,19 +189,19 @@ func TestValidSetShowZones(t *testing.T) { // Ensure zones for non-config tables in the system database can be // configured. - sqlutils.SetZoneConfig(t, sqlDB, "TABLE system.jobs", yamlOverride) + sqlutils.SetZoneConfig(t, sqlDB, "TABLE system.jobs", gcOverride) sqlutils.VerifyAllZoneConfigs(t, sqlDB, defaultRow, partialSystemRow, partialJobsRow) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "TABLE system.jobs", jobsRow) // Verify that the session database is respected. - sqlutils.SetZoneConfig(t, sqlDB, "TABLE t", yamlOverride) + sqlutils.SetZoneConfig(t, sqlDB, "TABLE t", gcOverride) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "TABLE t", tableRow) sqlutils.DeleteZoneConfig(t, sqlDB, "TABLE t") sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "TABLE t", defaultRow) // Verify we can use composite values. sqlDB.Exec(t, fmt.Sprintf("ALTER TABLE t CONFIGURE ZONE = '' || %s || ''", - lexbase.EscapeSQLString(yamlOverride))) + lexbase.EscapeSQLString("gc: {ttlseconds: 42}"))) sqlutils.VerifyZoneConfigForTarget(t, sqlDB, "TABLE t", tableRow) // Ensure zone configs are read transactionally instead of from the cached @@ -210,8 +210,9 @@ func TestValidSetShowZones(t *testing.T) { if err != nil { t.Fatal(err) } - sqlutils.TxnSetZoneConfig(t, sqlDB, txn, "RANGE default", yamlOverride) - sqlutils.TxnSetZoneConfig(t, sqlDB, txn, "TABLE d.t", "") // this should pick up the overridden default config + sqlutils.TxnSetZoneConfig(t, sqlDB, txn, "RANGE default", gcOverride) + sqlutils.TxnSetZoneConfig(t, sqlDB, txn, "TABLE d.t", + fmt.Sprintf("num_replicas = %d", *s.DefaultZoneConfig().NumReplicas)) // this should pick up the overridden default config if err := txn.Commit(); err != nil { t.Fatal(err) } diff --git a/pkg/testutils/sqlutils/BUILD.bazel b/pkg/testutils/sqlutils/BUILD.bazel index 7dfa360b0258..60226453b704 100644 --- a/pkg/testutils/sqlutils/BUILD.bazel +++ b/pkg/testutils/sqlutils/BUILD.bazel @@ -23,7 +23,6 @@ go_library( "//pkg/security/certnames", "//pkg/security/securitytest", "//pkg/sql/catalog/descpb", - "//pkg/sql/lexbase", "//pkg/sql/parser", "//pkg/sql/parser/statements", "//pkg/sql/pgwire/pgerror", diff --git a/pkg/testutils/sqlutils/zone.go b/pkg/testutils/sqlutils/zone.go index 38bda8a5030a..7345892a4129 100644 --- a/pkg/testutils/sqlutils/zone.go +++ b/pkg/testutils/sqlutils/zone.go @@ -11,7 +11,6 @@ import ( "testing" "github.com/cockroachdb/cockroach/pkg/config/zonepb" - "github.com/cockroachdb/cockroach/pkg/sql/lexbase" "github.com/cockroachdb/cockroach/pkg/sql/protoreflect" ) @@ -58,16 +57,16 @@ func DeleteZoneConfig(t testing.TB, sqlDB *SQLRunner, target string) { // SetZoneConfig updates the specified zone config through the SQL interface. func SetZoneConfig(t testing.TB, sqlDB *SQLRunner, target string, config string) { t.Helper() - sqlDB.Exec(t, fmt.Sprintf("ALTER %s CONFIGURE ZONE = %s", - target, lexbase.EscapeSQLString(config))) + sqlDB.Exec(t, fmt.Sprintf("ALTER %s CONFIGURE ZONE USING %s", + target, config)) } // TxnSetZoneConfig updates the specified zone config through the SQL interface // using the provided transaction. func TxnSetZoneConfig(t testing.TB, sqlDB *SQLRunner, txn *gosql.Tx, target string, config string) { t.Helper() - _, err := txn.Exec(fmt.Sprintf("ALTER %s CONFIGURE ZONE = %s", - target, lexbase.EscapeSQLString(config))) + _, err := txn.Exec(fmt.Sprintf("ALTER %s CONFIGURE ZONE USING %s", + target, config)) if err != nil { t.Fatal(err) }