catalog: add a PostDeserializationChange for fixing secondary index e…

…ncoding 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.

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
 )

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",

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)()
 

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