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