Skip to content

Commit

Permalink
sql/schemachangr: dropping column families ran into errors
Browse files Browse the repository at this point in the history 
Previously, dropping column families was not implemented,
when we eliminated oprules, we replaced the ops with
NotImplementedForDrop, which wasn't sufficient for
dropped columns. The column families are cleaned up
when the column type itself is dropped, so we don't
really need to do much here. To address this, this
patch will add code to only assert that either the
table is being dropped or the column family has been
removed earlier.

Fixes: #99796
Release note: None
  • Loading branch information
@fqazi
fqazi committed Mar 28, 2023
1 parent aec78f3 commit 65de860
Show file tree
Hide file tree
Showing 28 changed files with 496 additions and 156 deletions.
4 changes: 2 additions & 2 deletions pkg/ccl/schemachangerccl/testdata/explain/drop_database_multiregion_primary_region
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ Schema change plan for DROP DATABASE ‹multi_region_test_db› CASCADE;
│ ├── MarkDescriptorAsDropped {"DescriptorID":108}
│ ├── RemoveObjectParent {"ObjectID":108,"ParentSchemaID":105}
│ ├── RemoveBackReferenceInTypes {"BackReferencedDescriptorID":108}
│ ├── NotImplementedForPublicObjects {"DescID":108,"ElementType":"scpb.ColumnFamil..."}
│ ├── MakePublicColumnWriteOnly {"ColumnID":1,"TableID":108}
│ ├── SetColumnName {"ColumnID":1,"Name":"crdb_internal_co...","TableID":108}
│ ├── MakePublicColumnNotNullValidated {"ColumnID":1,"TableID":108}
Expand All @@ -97,6 +96,7 @@ Schema change plan for DROP DATABASE ‹multi_region_test_db› CASCADE;
│ ├── NotImplementedForPublicObjects {"DescID":108,"ElementType":"scpb.Owner"}
│ ├── RemoveUserPrivileges {"DescriptorID":108,"User":"admin"}
│ ├── RemoveUserPrivileges {"DescriptorID":108,"User":"root"}
│ ├── AssertColumnFamilyIsRemoved {"TableID":108}
│ ├── MarkDescriptorAsDropped {"DescriptorID":104}
│ ├── DrainDescriptorName {"Namespace":{"DatabaseID":104,"DescriptorID":105,"Name":"public"}}
│ ├── NotImplementedForPublicObjects {"DescID":105,"ElementType":"scpb.Owner"}
Expand Down Expand Up @@ -234,7 +234,6 @@ Schema change plan for DROP DATABASE ‹multi_region_test_db› CASCADE;
│ ├── MarkDescriptorAsDropped {"DescriptorID":108}
│ ├── RemoveObjectParent {"ObjectID":108,"ParentSchemaID":105}
│ ├── RemoveBackReferenceInTypes {"BackReferencedDescriptorID":108}
│ ├── NotImplementedForPublicObjects {"DescID":108,"ElementType":"scpb.ColumnFamil..."}
│ ├── MakePublicColumnWriteOnly {"ColumnID":1,"TableID":108}
│ ├── SetColumnName {"ColumnID":1,"Name":"crdb_internal_co...","TableID":108}
│ ├── MakePublicColumnNotNullValidated {"ColumnID":1,"TableID":108}
Expand All @@ -257,6 +256,7 @@ Schema change plan for DROP DATABASE ‹multi_region_test_db› CASCADE;
│ ├── NotImplementedForPublicObjects {"DescID":108,"ElementType":"scpb.Owner"}
│ ├── RemoveUserPrivileges {"DescriptorID":108,"User":"admin"}
│ ├── RemoveUserPrivileges {"DescriptorID":108,"User":"root"}
│ ├── AssertColumnFamilyIsRemoved {"TableID":108}
│ ├── RemoveColumnNotNull {"ColumnID":1,"TableID":108}
│ ├── MakeWriteOnlyColumnDeleteOnly {"ColumnID":4294967295,"TableID":108}
│ ├── MakeWriteOnlyColumnDeleteOnly {"ColumnID":4294967294,"TableID":108}
Expand Down
6 changes: 3 additions & 3 deletions pkg/ccl/schemachangerccl/testdata/explain/drop_table_multiregion
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ Schema change plan for DROP TABLE ‹multi_region_test_db›.‹public›.‹tab
│ ├── MarkDescriptorAsDropped {"DescriptorID":108}
│ ├── RemoveObjectParent {"ObjectID":108,"ParentSchemaID":105}
│ ├── NotImplementedForPublicObjects {"DescID":108,"ElementType":"scpb.TablePartit..."}
│ ├── NotImplementedForPublicObjects {"DescID":108,"ElementType":"scpb.ColumnFamil..."}
│ ├── MakePublicColumnWriteOnly {"ColumnID":1,"TableID":108}
│ ├── SetColumnName {"ColumnID":1,"Name":"crdb_internal_co...","TableID":108}
│ ├── MakePublicColumnNotNullValidated {"ColumnID":1,"TableID":108}
Expand All @@ -62,7 +61,8 @@ Schema change plan for DROP TABLE ‹multi_region_test_db›.‹public›.‹tab
│ ├── RemoveUserPrivileges {"DescriptorID":108,"User":"admin"}
│ ├── RemoveUserPrivileges {"DescriptorID":108,"User":"root"}
│ ├── RemoveDroppedColumnType {"ColumnID":2,"TableID":108}
│ └── UpdateTableBackReferencesInTypes {"BackReferencedTableID":108}
│ ├── UpdateTableBackReferencesInTypes {"BackReferencedTableID":108}
│ └── AssertColumnFamilyIsRemoved {"TableID":108}
├── PreCommitPhase
│ ├── Stage 1 of 2 in PreCommitPhase
│ │ ├── 27 elements transitioning toward ABSENT
Expand Down Expand Up @@ -130,7 +130,6 @@ Schema change plan for DROP TABLE ‹multi_region_test_db›.‹public›.‹tab
│ ├── MarkDescriptorAsDropped {"DescriptorID":108}
│ ├── RemoveObjectParent {"ObjectID":108,"ParentSchemaID":105}
│ ├── NotImplementedForPublicObjects {"DescID":108,"ElementType":"scpb.TablePartit..."}
│ ├── NotImplementedForPublicObjects {"DescID":108,"ElementType":"scpb.ColumnFamil..."}
│ ├── MakePublicColumnWriteOnly {"ColumnID":1,"TableID":108}
│ ├── SetColumnName {"ColumnID":1,"Name":"crdb_internal_co...","TableID":108}
│ ├── MakePublicColumnNotNullValidated {"ColumnID":1,"TableID":108}
Expand All @@ -155,6 +154,7 @@ Schema change plan for DROP TABLE ‹multi_region_test_db›.‹public›.‹tab
│ ├── RemoveColumnNotNull {"ColumnID":2,"TableID":108}
│ ├── MakeWriteOnlyColumnDeleteOnly {"ColumnID":4294967295,"TableID":108}
│ ├── MakeWriteOnlyColumnDeleteOnly {"ColumnID":4294967294,"TableID":108}
│ ├── AssertColumnFamilyIsRemoved {"TableID":108}
│ ├── MakeWriteOnlyColumnDeleteOnly {"ColumnID":1,"TableID":108}
│ ├── MakeWriteOnlyColumnDeleteOnly {"ColumnID":2,"TableID":108}
│ ├── MakeDeleteOnlyColumnAbsent {"ColumnID":4294967295,"TableID":108}
Expand Down
6 changes: 3 additions & 3 deletions pkg/ccl/schemachangerccl/testdata/explain/drop_table_multiregion_primary_region
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ Schema change plan for DROP TABLE ‹multi_region_test_db›.‹public›.‹tab
│ ├── MarkDescriptorAsDropped {"DescriptorID":108}
│ ├── RemoveObjectParent {"ObjectID":108,"ParentSchemaID":105}
│ ├── RemoveBackReferenceInTypes {"BackReferencedDescriptorID":108}
│ ├── NotImplementedForPublicObjects {"DescID":108,"ElementType":"scpb.ColumnFamil..."}
│ ├── MakePublicColumnWriteOnly {"ColumnID":1,"TableID":108}
│ ├── SetColumnName {"ColumnID":1,"Name":"crdb_internal_co...","TableID":108}
│ ├── MakePublicColumnNotNullValidated {"ColumnID":1,"TableID":108}
Expand All @@ -48,7 +47,8 @@ Schema change plan for DROP TABLE ‹multi_region_test_db›.‹public›.‹tab
│ ├── DrainDescriptorName {"Namespace":{"DatabaseID":104,"DescriptorID":108,"Name":"table_regional_b...","SchemaID":105}}
│ ├── NotImplementedForPublicObjects {"DescID":108,"ElementType":"scpb.Owner"}
│ ├── RemoveUserPrivileges {"DescriptorID":108,"User":"admin"}
│ └── RemoveUserPrivileges {"DescriptorID":108,"User":"root"}
│ ├── RemoveUserPrivileges {"DescriptorID":108,"User":"root"}
│ └── AssertColumnFamilyIsRemoved {"TableID":108}
├── PreCommitPhase
│ ├── Stage 1 of 2 in PreCommitPhase
│ │ ├── 20 elements transitioning toward ABSENT
Expand Down Expand Up @@ -101,7 +101,6 @@ Schema change plan for DROP TABLE ‹multi_region_test_db›.‹public›.‹tab
│ ├── MarkDescriptorAsDropped {"DescriptorID":108}
│ ├── RemoveObjectParent {"ObjectID":108,"ParentSchemaID":105}
│ ├── RemoveBackReferenceInTypes {"BackReferencedDescriptorID":108}
│ ├── NotImplementedForPublicObjects {"DescID":108,"ElementType":"scpb.ColumnFamil..."}
│ ├── MakePublicColumnWriteOnly {"ColumnID":1,"TableID":108}
│ ├── SetColumnName {"ColumnID":1,"Name":"crdb_internal_co...","TableID":108}
│ ├── MakePublicColumnNotNullValidated {"ColumnID":1,"TableID":108}
Expand All @@ -115,6 +114,7 @@ Schema change plan for DROP TABLE ‹multi_region_test_db›.‹public›.‹tab
│ ├── NotImplementedForPublicObjects {"DescID":108,"ElementType":"scpb.Owner"}
│ ├── RemoveUserPrivileges {"DescriptorID":108,"User":"admin"}
│ ├── RemoveUserPrivileges {"DescriptorID":108,"User":"root"}
│ ├── AssertColumnFamilyIsRemoved {"TableID":108}
│ ├── RemoveColumnNotNull {"ColumnID":1,"TableID":108}
│ ├── MakeWriteOnlyColumnDeleteOnly {"ColumnID":4294967295,"TableID":108}
│ ├── MakeWriteOnlyColumnDeleteOnly {"ColumnID":4294967294,"TableID":108}
Expand Down
40 changes: 28 additions & 12 deletions pkg/ccl/schemachangerccl/testdata/explain_verbose/drop_database_multiregion_primary_region
View file
Original file line number Diff line number Diff line change
Expand Up @@ -271,8 +271,17 @@ EXPLAIN (ddl, verbose) DROP DATABASE multi_region_test_db CASCADE;
│ │ ├── • ColumnFamily:{DescID: 108, Name: primary, ColumnFamilyID: 0}
│ │ │ │ PUBLIC → ABSENT
│ │ │ │
│ │ │ └── • Precedence dependency from DROPPED Table:{DescID: 108}
│ │ │ rule: "descriptor dropped before dependent element removal"
│ │ │ ├── • Precedence dependency from DROPPED Table:{DescID: 108}
│ │ │ │ rule: "descriptor dropped before dependent element removal"
│ │ │ │
│ │ │ ├── • Precedence dependency from ABSENT ColumnType:{DescID: 108, ColumnFamilyID: 0, ColumnID: 1}
│ │ │ │ rule: "column type removed before column family"
│ │ │ │
│ │ │ ├── • Precedence dependency from ABSENT ColumnType:{DescID: 108, ColumnFamilyID: 0, ColumnID: 4294967295}
│ │ │ │ rule: "column type removed before column family"
│ │ │ │
│ │ │ └── • Precedence dependency from ABSENT ColumnType:{DescID: 108, ColumnFamilyID: 0, ColumnID: 4294967294}
│ │ │ rule: "column type removed before column family"
│ │ │
│ │ ├── • Column:{DescID: 108, ColumnID: 1}
│ │ │ │ PUBLIC → WRITE_ONLY
Expand Down Expand Up @@ -407,10 +416,6 @@ EXPLAIN (ddl, verbose) DROP DATABASE multi_region_test_db CASCADE;
│ │ TypeIDs:
│ │ - 106
│ │
│ ├── • NotImplementedForPublicObjects
│ │ DescID: 108
│ │ ElementType: scpb.ColumnFamily
│ │
│ ├── • MakePublicColumnWriteOnly
│ │ ColumnID: 1
│ │ TableID: 108
Expand Down Expand Up @@ -503,6 +508,9 @@ EXPLAIN (ddl, verbose) DROP DATABASE multi_region_test_db CASCADE;
│ │ DescriptorID: 108
│ │ User: root
│ │
│ ├── • AssertColumnFamilyIsRemoved
│ │ TableID: 108
│ │
│ ├── • MarkDescriptorAsDropped
│ │ DescriptorID: 104
│ │
Expand Down Expand Up @@ -997,8 +1005,17 @@ EXPLAIN (ddl, verbose) DROP DATABASE multi_region_test_db CASCADE;
│ │ ├── • ColumnFamily:{DescID: 108, Name: primary, ColumnFamilyID: 0}
│ │ │ │ PUBLIC → ABSENT
│ │ │ │
│ │ │ └── • Precedence dependency from DROPPED Table:{DescID: 108}
│ │ │ rule: "descriptor dropped before dependent element removal"
│ │ │ ├── • Precedence dependency from DROPPED Table:{DescID: 108}
│ │ │ │ rule: "descriptor dropped before dependent element removal"
│ │ │ │
│ │ │ ├── • Precedence dependency from ABSENT ColumnType:{DescID: 108, ColumnFamilyID: 0, ColumnID: 1}
│ │ │ │ rule: "column type removed before column family"
│ │ │ │
│ │ │ ├── • Precedence dependency from ABSENT ColumnType:{DescID: 108, ColumnFamilyID: 0, ColumnID: 4294967295}
│ │ │ │ rule: "column type removed before column family"
│ │ │ │
│ │ │ └── • Precedence dependency from ABSENT ColumnType:{DescID: 108, ColumnFamilyID: 0, ColumnID: 4294967294}
│ │ │ rule: "column type removed before column family"
│ │ │
│ │ ├── • Column:{DescID: 108, ColumnID: 1}
│ │ │ │ PUBLIC → ABSENT
Expand Down Expand Up @@ -1181,10 +1198,6 @@ EXPLAIN (ddl, verbose) DROP DATABASE multi_region_test_db CASCADE;
│ │ TypeIDs:
│ │ - 106
│ │
│ ├── • NotImplementedForPublicObjects
│ │ DescID: 108
│ │ ElementType: scpb.ColumnFamily
│ │
│ ├── • MakePublicColumnWriteOnly
│ │ ColumnID: 1
│ │ TableID: 108
Expand Down Expand Up @@ -1277,6 +1290,9 @@ EXPLAIN (ddl, verbose) DROP DATABASE multi_region_test_db CASCADE;
│ │ DescriptorID: 108
│ │ User: root
│ │
│ ├── • AssertColumnFamilyIsRemoved
│ │ TableID: 108
│ │
│ ├── • RemoveColumnNotNull
│ │ ColumnID: 1
│ │ TableID: 108
Expand Down
56 changes: 39 additions & 17 deletions pkg/ccl/schemachangerccl/testdata/explain_verbose/drop_table_multiregion
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,20 @@ EXPLAIN (ddl, verbose) DROP TABLE multi_region_test_db.public.table_regional_by_
│ │ ├── • ColumnFamily:{DescID: 108, Name: primary, ColumnFamilyID: 0}
│ │ │ │ PUBLIC → ABSENT
│ │ │ │
│ │ │ └── • Precedence dependency from DROPPED Table:{DescID: 108}
│ │ │ rule: "descriptor dropped before dependent element removal"
│ │ │ ├── • Precedence dependency from DROPPED Table:{DescID: 108}
│ │ │ │ rule: "descriptor dropped before dependent element removal"
│ │ │ │
│ │ │ ├── • Precedence dependency from ABSENT ColumnType:{DescID: 108, ColumnFamilyID: 0, ColumnID: 1}
│ │ │ │ rule: "column type removed before column family"
│ │ │ │
│ │ │ ├── • Precedence dependency from ABSENT ColumnType:{DescID: 108, ReferencedTypeIDs: [106 107], ColumnFamilyID: 0, ColumnID: 2}
│ │ │ │ rule: "column type removed before column family"
│ │ │ │
│ │ │ ├── • Precedence dependency from ABSENT ColumnType:{DescID: 108, ColumnFamilyID: 0, ColumnID: 4294967295}
│ │ │ │ rule: "column type removed before column family"
│ │ │ │
│ │ │ └── • Precedence dependency from ABSENT ColumnType:{DescID: 108, ColumnFamilyID: 0, ColumnID: 4294967294}
│ │ │ rule: "column type removed before column family"
│ │ │
│ │ ├── • Column:{DescID: 108, ColumnID: 1}
│ │ │ │ PUBLIC → WRITE_ONLY
Expand Down Expand Up @@ -224,10 +236,6 @@ EXPLAIN (ddl, verbose) DROP TABLE multi_region_test_db.public.table_regional_by_
│ │ DescID: 108
│ │ ElementType: scpb.TablePartitioning
│ │
│ ├── • NotImplementedForPublicObjects
│ │ DescID: 108
│ │ ElementType: scpb.ColumnFamily
│ │
│ ├── • MakePublicColumnWriteOnly
│ │ ColumnID: 1
│ │ TableID: 108
Expand Down Expand Up @@ -314,11 +322,14 @@ EXPLAIN (ddl, verbose) DROP TABLE multi_region_test_db.public.table_regional_by_
│ │ ColumnID: 2
│ │ TableID: 108
│ │
│ └── • UpdateTableBackReferencesInTypes
│ BackReferencedTableID: 108
│ TypeIDs:
│ - 106
│ - 107
│ ├── • UpdateTableBackReferencesInTypes
│ │ BackReferencedTableID: 108
│ │ TypeIDs:
│ │ - 106
│ │ - 107
│ │
│ └── • AssertColumnFamilyIsRemoved
│ TableID: 108
│
├── • PreCommitPhase
│ │
Expand Down Expand Up @@ -465,8 +476,20 @@ EXPLAIN (ddl, verbose) DROP TABLE multi_region_test_db.public.table_regional_by_
│ │ ├── • ColumnFamily:{DescID: 108, Name: primary, ColumnFamilyID: 0}
│ │ │ │ PUBLIC → ABSENT
│ │ │ │
│ │ │ └── • Precedence dependency from DROPPED Table:{DescID: 108}
│ │ │ rule: "descriptor dropped before dependent element removal"
│ │ │ ├── • Precedence dependency from DROPPED Table:{DescID: 108}
│ │ │ │ rule: "descriptor dropped before dependent element removal"
│ │ │ │
│ │ │ ├── • Precedence dependency from ABSENT ColumnType:{DescID: 108, ColumnFamilyID: 0, ColumnID: 1}
│ │ │ │ rule: "column type removed before column family"
│ │ │ │
│ │ │ ├── • Precedence dependency from ABSENT ColumnType:{DescID: 108, ReferencedTypeIDs: [106 107], ColumnFamilyID: 0, ColumnID: 2}
│ │ │ │ rule: "column type removed before column family"
│ │ │ │
│ │ │ ├── • Precedence dependency from ABSENT ColumnType:{DescID: 108, ColumnFamilyID: 0, ColumnID: 4294967295}
│ │ │ │ rule: "column type removed before column family"
│ │ │ │
│ │ │ └── • Precedence dependency from ABSENT ColumnType:{DescID: 108, ColumnFamilyID: 0, ColumnID: 4294967294}
│ │ │ rule: "column type removed before column family"
│ │ │
│ │ ├── • Column:{DescID: 108, ColumnID: 1}
│ │ │ │ PUBLIC → ABSENT
Expand Down Expand Up @@ -712,10 +735,6 @@ EXPLAIN (ddl, verbose) DROP TABLE multi_region_test_db.public.table_regional_by_
│ │ DescID: 108
│ │ ElementType: scpb.TablePartitioning
│ │
│ ├── • NotImplementedForPublicObjects
│ │ DescID: 108
│ │ ElementType: scpb.ColumnFamily
│ │
│ ├── • MakePublicColumnWriteOnly
│ │ ColumnID: 1
│ │ TableID: 108
Expand Down Expand Up @@ -824,6 +843,9 @@ EXPLAIN (ddl, verbose) DROP TABLE multi_region_test_db.public.table_regional_by_
│ │ ColumnID: 4294967294
│ │ TableID: 108
│ │
│ ├── • AssertColumnFamilyIsRemoved
│ │ TableID: 108
│ │
│ ├── • MakeWriteOnlyColumnDeleteOnly
│ │ ColumnID: 1
│ │ TableID: 108
Expand Down
Loading

0 comments on commit 65de860

Please sign in to comment.