sql: remove explicit index IDs in foreign key descriptors

docs/generated/settings/settings.html

@@ -61,6 +61,6 @@
 <tr><td><code>trace.debug.enable</code></td><td>boolean</td><td><code>false</code></td><td>if set, traces for recent requests can be seen in the /debug page</td></tr>
 <tr><td><code>trace.lightstep.token</code></td><td>string</td><td><code></code></td><td>if set, traces go to Lightstep using this token</td></tr>
 <tr><td><code>trace.zipkin.collector</code></td><td>string</td><td><code></code></td><td>if set, traces go to the given Zipkin instance (example: '127.0.0.1:9411'); ignored if trace.lightstep.token is set</td></tr>
-<tr><td><code>version</code></td><td>custom validation</td><td><code>19.2-10</code></td><td>set the active cluster version in the format '<major>.<minor>'</td></tr>
+<tr><td><code>version</code></td><td>custom validation</td><td><code>19.2-11</code></td><td>set the active cluster version in the format '<major>.<minor>'</td></tr>
 </tbody>
 </table>

pkg/settings/cluster/cockroach_versions.go

@@ -55,6 +55,7 @@ const (
 	VersionAuthLocalAndTrustRejectMethods
 	VersionPrimaryKeyColumnsOutOfFamilyZero
 	VersionRootPassword
+	VersionNoExplicitForeignKeyIndexIDs
 
 	// Add new versions here (step one of two).
 )
@@ -388,6 +389,15 @@ var versionsSingleton = keyedVersions([]keyedVersion{
 		Key:     VersionRootPassword,
 		Version: roachpb.Version{Major: 19, Minor: 2, Unstable: 10},
 	},
+	{
+		// VersionNoExplicitForeignKeyIndexIDs is https://github.com/cockroachdb/cockroach/pull/43332.
+		//
+		// It represents the migration away from using explicit index IDs in foreign
+		// key constraints, and instead allows all places that need these IDs to select
+		// an appropriate index to uphold the foreign key relationship.
+		Key:     VersionNoExplicitForeignKeyIndexIDs,
+		Version: roachpb.Version{Major: 19, Minor: 2, Unstable: 11},
+	},
 
 	// Add new versions here (step two of two).
 

pkg/sql/crdb_internal.go

@@ -1686,13 +1686,21 @@ CREATE TABLE crdb_internal.backward_dependencies (
 
 				for i := range table.OutboundFKs {
 					fk := &table.OutboundFKs[i]
+					refTbl, err := tableLookup.getTableByID(fk.ReferencedTableID)
+					if err != nil {
+						return err
+					}
+					refIdx, err := sqlbase.FindFKReferencedIndex(refTbl, fk.ReferencedColumnIDs)
+					if err != nil {
+						return err
+					}
 					if err := addRow(
 						tableID, tableName,
 						tree.DNull,
 						tree.DNull,
 						tree.NewDInt(tree.DInt(fk.ReferencedTableID)),
 						fkDep,
-						tree.NewDInt(tree.DInt(fk.LegacyReferencedIndex)),
+						tree.NewDInt(tree.DInt(refIdx.ID)),
 						tree.NewDString(fk.Name),
 						tree.DNull,
 					); err != nil {

pkg/sql/create_table.go

@@ -442,6 +442,43 @@ const (
 	NonEmptyTable
 )
 
+// MaybeUpgradeDependentOldForeignKeyVersionTables upgrades the on-disk foreign key descriptor
+// version of all table descriptors that have foreign key relationships with desc. This is intended
+// to catch upgrade 19.1 version table descriptors that haven't been upgraded yet before an operation
+// like drop index which could cause them to lose FK information in the old representation.
+func (p *planner) MaybeUpgradeDependentOldForeignKeyVersionTables(
+	ctx context.Context, desc *sqlbase.MutableTableDescriptor,
+) error {
+	// In order to avoid having old version foreign key descriptors that depend on this
+	// index lose information when this index is dropped, ensure that they get updated.
+	maybeUpgradeFKRepresentation := func(id sqlbase.ID) error {
+		// Read the referenced table and see if the foreign key representation has changed. If it has, write
+		// the upgraded descriptor back to disk.
+		tbl, didUpgrade, err := sqlbase.GetTableDescFromIDWithFKsChanged(ctx, p.txn, id)
+		if err != nil {
+			return err
+		}
+		if didUpgrade {
+			err := p.writeSchemaChange(ctx, sqlbase.NewMutableExistingTableDescriptor(*tbl), sqlbase.InvalidMutationID)
+			if err != nil {
+				return err
+			}
+		}
+		return nil
+	}
+	for i := range desc.OutboundFKs {
+		if err := maybeUpgradeFKRepresentation(desc.OutboundFKs[i].ReferencedTableID); err != nil {
+			return err
+		}
+	}
+	for i := range desc.InboundFKs {
+		if err := maybeUpgradeFKRepresentation(desc.InboundFKs[i].OriginTableID); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 // ResolveFK looks up the tables and columns mentioned in a `REFERENCES`
 // constraint and adds metadata representing that constraint to the descriptor.
 // It may, in doing so, add to or alter descriptors in the passed in `backrefs`

pkg/sql/drop_index.go

@@ -16,6 +16,7 @@ import (
 	"strings"
 
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
@@ -165,29 +166,84 @@ func (p *planner) dropIndexByName(
 	// state consistent with the removal of the reference on the other table
 	// involved in the FK, in case of rollbacks (#38733).
 
+	// TODO (rohany): switching all the checks from checking the legacy ID's to
+	//  checking if the index has a prefix of the columns needed for the foreign
+	//  key might result in some false positives for this index while it is in
+	//  a mixed version cluster, but we have to remove all reads of the legacy
+	//  explicit index fields.
+
+	// Construct a list of all the remaining indexes, so that we can see if there
+	// is another index that could replace the one we are deleting for a given
+	// foreign key constraint.
+	remainingIndexes := make([]*sqlbase.IndexDescriptor, 0, len(tableDesc.Indexes)+1)
+	remainingIndexes = append(remainingIndexes, &tableDesc.PrimaryIndex)
+	for i := range tableDesc.Indexes {
+		index := &tableDesc.Indexes[i]
+		if index.ID != idx.ID {
+			remainingIndexes = append(remainingIndexes, index)
+		}
+	}
+
+	// indexHasReplacementCandidate runs isValidIndex on each index in remainingIndexes and returns
+	// true if at least one index satisfies isValidIndex.
+	indexHasReplacementCandidate := func(isValidIndex func(*sqlbase.IndexDescriptor) bool) bool {
+		foundReplacement := false
+		for _, index := range remainingIndexes {
+			if isValidIndex(index) {
+				foundReplacement = true
+				break
+			}
+		}
+		return foundReplacement
+	}
+	// If we aren't at the cluster version where we have removed explicit foreign key IDs
+	// from the foreign key descriptors, fall back to the existing drop index logic.
+	// That means we pretend that we can never find replacements for any indexes.
+	if !cluster.Version.IsActive(ctx, p.ExecCfg().Settings, cluster.VersionNoExplicitForeignKeyIndexIDs) {
+		indexHasReplacementCandidate = func(func(*sqlbase.IndexDescriptor) bool) bool {
+			return false
+		}
+	}
+
 	// Check for foreign key mutations referencing this index.
 	for _, m := range tableDesc.Mutations {
 		if c := m.GetConstraint(); c != nil &&
 			c.ConstraintType == sqlbase.ConstraintToUpdate_FOREIGN_KEY &&
-			c.ForeignKey.LegacyOriginIndex == idx.ID {
+			// If the index being deleted could be used as a index for this outbound
+			// foreign key mutation, then make sure that we have another index that
+			// could be used for this mutation.
+			idx.IsValidOriginIndex(c.ForeignKey.OriginColumnIDs) &&
+			!indexHasReplacementCandidate(func(idx *sqlbase.IndexDescriptor) bool {
+				return idx.IsValidOriginIndex(c.ForeignKey.OriginColumnIDs)
+			}) {
 			return pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
 				"referencing constraint %q in the middle of being added, try again later", c.ForeignKey.Name)
 		}
 	}
 
+	if err := p.MaybeUpgradeDependentOldForeignKeyVersionTables(ctx, tableDesc); err != nil {
+		return err
+	}
+
 	// Index for updating the FK slices in place when removing FKs.
 	sliceIdx := 0
 	for i := range tableDesc.OutboundFKs {
 		tableDesc.OutboundFKs[sliceIdx] = tableDesc.OutboundFKs[i]
 		sliceIdx++
 		fk := &tableDesc.OutboundFKs[i]
-		if fk.LegacyOriginIndex == idx.ID {
-			if behavior != tree.DropCascade && constraintBehavior != ignoreIdxConstraint {
-				return errors.Errorf("index %q is in use as a foreign key constraint", idx.Name)
-			}
-			sliceIdx--
-			if err := p.removeFKBackReference(ctx, tableDesc, fk); err != nil {
-				return err
+		canReplace := func(idx *sqlbase.IndexDescriptor) bool {
+			return idx.IsValidOriginIndex(fk.OriginColumnIDs)
+		}
+		// The index being deleted could be used as the origin index for this foreign key.
+		if idx.IsValidOriginIndex(fk.OriginColumnIDs) {
+			if !indexHasReplacementCandidate(canReplace) {
+				if behavior != tree.DropCascade && constraintBehavior != ignoreIdxConstraint {
+					return errors.Errorf("index %q is in use as a foreign key constraint", idx.Name)
+				}
+				sliceIdx--
+				if err := p.removeFKBackReference(ctx, tableDesc, fk); err != nil {
+					return err
+				}
 			}
 		}
 	}
@@ -198,14 +254,22 @@ func (p *planner) dropIndexByName(
 		tableDesc.InboundFKs[sliceIdx] = tableDesc.InboundFKs[i]
 		sliceIdx++
 		fk := &tableDesc.InboundFKs[i]
-		if fk.LegacyReferencedIndex == idx.ID {
-			err := p.canRemoveFKBackreference(ctx, idx.Name, fk, behavior)
-			if err != nil {
-				return err
-			}
-			sliceIdx--
-			if err := p.removeFKForBackReference(ctx, tableDesc, fk); err != nil {
-				return err
+		canReplace := func(idx *sqlbase.IndexDescriptor) bool {
+			return idx.IsValidReferencedIndex(fk.ReferencedColumnIDs)
+		}
+		// The index being deleted could potentially be the referenced index for this fk.
+		if idx.IsValidReferencedIndex(fk.ReferencedColumnIDs) {
+			// If we haven't found a replacement candidate for this foreign key, then
+			// we need a cascade to delete this index.
+			if !indexHasReplacementCandidate(canReplace) {
+				// If we found haven't found a replacement, then we check that the drop behavior is cascade.
+				if err := p.canRemoveFKBackreference(ctx, idx.Name, fk, behavior); err != nil {
+					return err
+				}
+				sliceIdx--
+				if err := p.removeFKForBackReference(ctx, tableDesc, fk); err != nil {
+					return err
+				}
 			}
 		}
 	}

pkg/sql/information_schema.go

@@ -815,7 +815,7 @@ CREATE TABLE information_schema.referential_constraints (
 				if r, ok := matchOptionMap[fk.Match]; ok {
 					matchType = r
 				}
-				referencedIdx, err := refTable.FindIndexByID(fk.LegacyReferencedIndex)
+				referencedIdx, err := sqlbase.FindFKReferencedIndex(refTable, fk.ReferencedColumnIDs)
 				if err != nil {
 					return err
 				}

pkg/sql/logictest/logic.go

@@ -448,6 +448,15 @@ var logicTestConfigs = []testClusterConfig{
 		overrideDistSQLMode: "on",
 		overrideAutoStats:   "false",
 	},
+	{
+		name:                "local-mixed-19.2-20.1",
+		numNodes:            1,
+		overrideDistSQLMode: "off",
+		overrideAutoStats:   "false",
+		bootstrapVersion:    roachpb.Version{Major: 19, Minor: 2},
+		serverVersion:       roachpb.Version{Major: 20, Minor: 1},
+		disableUpgrade:      true,
+	},
 	{
 		name:                "fakedist-vec-off",
 		numNodes:            3,

pkg/sql/logictest/testdata/logic_test/drop_index

@@ -265,3 +265,52 @@ DROP INDEX tu_a_key
 
 statement ok
 DROP INDEX tu_a
+
+# Test that we have more relaxed restrictions on dropping indexes referenced by fks.
+subtest fk_drop
+
+# Ensure that we can drop an index used by a foreign key if there is another
+# index that can take its place.
+statement ok
+CREATE TABLE fk1 (x INT);
+CREATE TABLE fk2 (x INT PRIMARY KEY);
+ALTER TABLE fk1 ADD CONSTRAINT fk1 FOREIGN KEY (x) REFERENCES fk2 (x);
+CREATE INDEX i ON fk1 (x);
+DROP INDEX fk1_auto_index_fk1
+
+statement error pq: index "i" is in use as a foreign key constraint
+DROP INDEX fk1@i
+
+# Ensure that DROP INDEX CASCADE does not delete the foreign key when
+# there is another index that can satisfy the foreign key constraint.
+statement ok
+DROP TABLE fk1;
+DROP TABLE fk2;
+CREATE TABLE fk1 (x int);
+CREATE TABLE fk2 (x int PRIMARY KEY);
+CREATE INDEX i ON fk1 (x);
+CREATE INDEX i2 ON fk1 (x);
+ALTER TABLE fk1 ADD CONSTRAINT fk1 FOREIGN KEY (x) REFERENCES fk2 (x);
+DROP INDEX fk1@i CASCADE
+
+query TT
+SHOW CREATE fk1
+----
+fk1  CREATE TABLE fk1 (
+     x INT8 NULL,
+     CONSTRAINT fk1 FOREIGN KEY (x) REFERENCES fk2(x),
+     INDEX i2 (x ASC),
+     FAMILY "primary" (x, rowid)
+)
+
+# Ensure that now the cascade deletes the foreign key constraint.
+statement ok
+DROP INDEX fk1@i2 CASCADE
+
+query TT
+SHOW CREATE fk1
+----
+fk1  CREATE TABLE fk1 (
+     x INT8 NULL,
+     FAMILY "primary" (x, rowid)
+)