From 23a55b62a59223fef723faef0045e9c3b66fcd5d Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Thu, 29 Jun 2023 16:34:59 -0400 Subject: [PATCH] catalog: add a PostDeserializationChange for fixing secondary index encoding type Release note (bug fix): Fixed a bug where some secondary indexes would incorrectly be treated internally as primary indexes, which could cause some schema change operations to fail. The bug could occur if ALTER PRIMARY KEY was used on v21.1 or earlier, and the cluster was upgraded. --- .../catalog/post_deserialization_changes.go | 4 + pkg/sql/catalog/tabledesc/BUILD.bazel | 1 + pkg/sql/catalog/tabledesc/structured_test.go | 106 ++++++++++++++++++ .../catalog/tabledesc/table_desc_builder.go | 16 +++ 4 files changed, 127 insertions(+) diff --git a/pkg/sql/catalog/post_deserialization_changes.go b/pkg/sql/catalog/post_deserialization_changes.go index e6369550d307..ff896a30483a 100644 --- a/pkg/sql/catalog/post_deserialization_changes.go +++ b/pkg/sql/catalog/post_deserialization_changes.go @@ -98,4 +98,8 @@ const ( // SetCreateAsOfTimeUsingModTime indicates that a table's CreateAsOfTime field // was unset and the ModificationTime value was assigned to it. SetCreateAsOfTimeUsingModTime + + // FixSecondaryIndexEncodingType indicates that a secondary index had its + // encoding type fixed, so it is not incorrectly marked as a primary index. + FixSecondaryIndexEncodingType ) diff --git a/pkg/sql/catalog/tabledesc/BUILD.bazel b/pkg/sql/catalog/tabledesc/BUILD.bazel index 841155855294..353b9f9c9969 100644 --- a/pkg/sql/catalog/tabledesc/BUILD.bazel +++ b/pkg/sql/catalog/tabledesc/BUILD.bazel @@ -104,6 +104,7 @@ go_test( "//pkg/sql/catalog/internal/validate", "//pkg/sql/catalog/nstree", "//pkg/sql/privilege", + "//pkg/sql/schemachanger/scpb", "//pkg/sql/sem/catconstants", "//pkg/sql/sem/catid", "//pkg/sql/types", diff --git a/pkg/sql/catalog/tabledesc/structured_test.go b/pkg/sql/catalog/tabledesc/structured_test.go index 80cf89414d1a..c3e6dde530f3 100644 --- a/pkg/sql/catalog/tabledesc/structured_test.go +++ b/pkg/sql/catalog/tabledesc/structured_test.go @@ -28,6 +28,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils" "github.com/cockroachdb/cockroach/pkg/sql/catalog/internal/validate" . "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" + "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb" "github.com/cockroachdb/cockroach/pkg/sql/types" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" @@ -681,6 +682,111 @@ func TestUnvalidateConstraints(t *testing.T) { } } +func TestMaybeFixSecondaryIndexEncodingType(t *testing.T) { + tests := []struct { + desc descpb.TableDescriptor + expUpgrade bool + verify func(*testing.T, int, catalog.TableDescriptor) // nil means no extra verification. + }{ + { // 1 + desc: descpb.TableDescriptor{ + ID: 2, + ParentID: 1, + Name: "foo", + FormatVersion: descpb.InterleavedFormatVersion, + Columns: []descpb.ColumnDescriptor{ + {ID: 1, Name: "bar"}, + {ID: 2, Name: "baz"}, + }, + Families: []descpb.ColumnFamilyDescriptor{ + {ID: 0, Name: "primary", ColumnIDs: []descpb.ColumnID{1, 2}, ColumnNames: []string{"bar", "baz"}}, + }, + Privileges: catpb.NewBasePrivilegeDescriptor(username.RootUserName()), + PrimaryIndex: descpb.IndexDescriptor{ID: 1, Name: "primary", + KeyColumnIDs: []descpb.ColumnID{1, 2}, + KeyColumnNames: []string{"bar", "baz"}, + KeyColumnDirections: []catpb.IndexColumn_Direction{catpb.IndexColumn_ASC, catpb.IndexColumn_ASC}, + EncodingType: descpb.PrimaryIndexEncoding, + Version: descpb.LatestIndexDescriptorVersion, + ConstraintID: 1, + }, + Indexes: []descpb.IndexDescriptor{{ + ID: 2, + Name: "secondary", + KeyColumnIDs: []descpb.ColumnID{2}, + KeyColumnNames: []string{"baz"}, + KeyColumnDirections: []catpb.IndexColumn_Direction{catpb.IndexColumn_ASC}, + EncodingType: descpb.PrimaryIndexEncoding, + }}, + NextColumnID: 3, + NextFamilyID: 1, + NextIndexID: 3, + NextConstraintID: 2, + }, + expUpgrade: true, + verify: func(t *testing.T, _ int, newDesc catalog.TableDescriptor) { + require.Equal(t, descpb.SecondaryIndexEncoding, newDesc.TableDesc().Indexes[0].EncodingType) + }, + }, + { // 2 + desc: descpb.TableDescriptor{ + ID: 2, + ParentID: 1, + Name: "foo", + FormatVersion: descpb.InterleavedFormatVersion, + Columns: []descpb.ColumnDescriptor{ + {ID: 1, Name: "bar"}, + {ID: 2, Name: "baz"}, + }, + Families: []descpb.ColumnFamilyDescriptor{ + {ID: 0, Name: "primary", ColumnIDs: []descpb.ColumnID{1, 2}, ColumnNames: []string{"bar", "baz"}}, + }, + Privileges: catpb.NewBasePrivilegeDescriptor(username.RootUserName()), + PrimaryIndex: descpb.IndexDescriptor{ID: 1, Name: "primary", + KeyColumnIDs: []descpb.ColumnID{1, 2}, + KeyColumnNames: []string{"bar", "baz"}, + KeyColumnDirections: []catpb.IndexColumn_Direction{catpb.IndexColumn_ASC, catpb.IndexColumn_ASC}, + EncodingType: descpb.PrimaryIndexEncoding, + Version: descpb.LatestIndexDescriptorVersion, + ConstraintID: 1, + }, + Indexes: []descpb.IndexDescriptor{{ + ID: 2, + Name: "secondary", + KeyColumnIDs: []descpb.ColumnID{2}, + KeyColumnNames: []string{"baz"}, + KeyColumnDirections: []catpb.IndexColumn_Direction{catpb.IndexColumn_ASC}, + EncodingType: descpb.PrimaryIndexEncoding, + }}, + DeclarativeSchemaChangerState: &scpb.DescriptorState{ + JobID: catpb.JobID(1), + }, + NextColumnID: 3, + NextFamilyID: 1, + NextIndexID: 3, + NextConstraintID: 2, + }, + expUpgrade: false, + }, + } + for i, test := range tests { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + b := NewBuilder(&test.desc) + require.NoError(t, b.RunPostDeserializationChanges()) + desc := b.BuildImmutableTable() + changes, err := GetPostDeserializationChanges(desc) + require.NoError(t, err) + upgraded := changes.Contains(catalog.FixSecondaryIndexEncodingType) + if upgraded != test.expUpgrade { + t.Fatalf("%d: expected upgraded=%t, but got upgraded=%t", i, test.expUpgrade, upgraded) + } + if test.verify != nil { + test.verify(t, i, desc) + } + }) + } +} + func TestKeysPerRow(t *testing.T) { defer leaktest.AfterTest(t)() diff --git a/pkg/sql/catalog/tabledesc/table_desc_builder.go b/pkg/sql/catalog/tabledesc/table_desc_builder.go index 20bf13a24914..06e750f3633f 100644 --- a/pkg/sql/catalog/tabledesc/table_desc_builder.go +++ b/pkg/sql/catalog/tabledesc/table_desc_builder.go @@ -304,6 +304,7 @@ func maybeFillInDescriptor( set(catalog.UpgradedPrivileges, fixedPrivileges) set(catalog.RemovedDuplicateIDsInRefs, maybeRemoveDuplicateIDsInRefs(desc)) set(catalog.AddedConstraintIDs, maybeAddConstraintIDs(desc)) + set(catalog.FixSecondaryIndexEncodingType, maybeFixSecondaryIndexEncodingType(desc)) return changes, nil } @@ -831,6 +832,21 @@ func maybeAddConstraintIDs(desc *descpb.TableDescriptor) (hasChanged bool) { return desc.NextConstraintID != initialConstraintID } +func maybeFixSecondaryIndexEncodingType(desc *descpb.TableDescriptor) (hasChanged bool) { + if desc.DeclarativeSchemaChangerState != nil || len(desc.Mutations) > 0 { + // Don't try to fix the encoding type if a schema change is in progress. + return false + } + for i := range desc.Indexes { + idx := &desc.Indexes[i] + if idx.EncodingType != descpb.SecondaryIndexEncoding { + idx.EncodingType = descpb.SecondaryIndexEncoding + hasChanged = true + } + } + return hasChanged +} + // maybeSetCreateAsOfTime ensures that the CreateAsOfTime field is set. // // CreateAsOfTime is used for CREATE TABLE ... AS ... and was introduced in