From b148ad2121ada4e719d3f59eb8ecae4f78cdb608 Mon Sep 17 00:00:00 2001 From: Matt Spilchen Date: Thu, 20 Nov 2025 08:39:21 -0400 Subject: [PATCH] workload/schemachange: fix FK dependency check for drop column logic This is a fix for the schemachanger workload. Previously, `columnRemovalWillDropFKBackingIndexes` incorrectly identified foreign key dependencies when dropping columns. It included stored columns from indexes, even though only key columns are relevant to determining foreign key dependencies. This led to false positives where a drop column operation was expected to fail due to a foreign key dependency, even when it wouldn't in practice. To address this: - The function (now renamed to `columnDropViolatesFKIndexRequirements`) now filters out stored columns, considering only key columns when determining if an index would be invalidated by the column drop. - A new logic test was added to model the SQL logic under various scenarios. The test uses a UDF with pg_catalog queries instead of `SHOW INDEXES` (which cannot be used inside SQL functions), but the behavior is equivalent. Fixes #157886 Release note: none --- pkg/sql/logictest/testdata/logic_test/fk | 254 ++++++++++++++++++ pkg/workload/schemachange/error_screening.go | 13 +- .../schemachange/operation_generator.go | 4 +- 3 files changed, 264 insertions(+), 7 deletions(-) diff --git a/pkg/sql/logictest/testdata/logic_test/fk b/pkg/sql/logictest/testdata/logic_test/fk index 20f1fcc0b75a..c19a61d65031 100644 --- a/pkg/sql/logictest/testdata/logic_test/fk +++ b/pkg/sql/logictest/testdata/logic_test/fk @@ -4841,3 +4841,257 @@ DROP TABLE t2_fk; DROP TABLE t1_fk; subtest end + +subtest drop_col_dep_non_self_fk_stored + +# The logic in this UDF is equivalent to and originated from +# columnDropViolatesFKIndexRequirements() in pkg/workload/schemachange/error_screening.go. +statement ok +CREATE FUNCTION column_drop_violates_fk_index_requirements(table_name STRING, column_name STRING) RETURNS BOOL LANGUAGE SQL AS $$ + WITH fk AS ( + SELECT + oid, + (SELECT r.relname FROM pg_class AS r WHERE r.oid = c.confrelid) AS base_table, + a.attname AS base_col, + array_position(c.confkey, a.attnum) AS base_ordinal, + (SELECT r.relname FROM pg_class AS r WHERE r.oid = c.conrelid) AS referencing_table, + unnest( + (SELECT array_agg(attname) + FROM pg_attribute + WHERE attrelid = c.conrelid + AND ARRAY[attnum] <@ c.conkey + AND array_position(c.confkey, a.attnum) = array_position(c.conkey, attnum)) + ) AS referencing_col + FROM pg_constraint AS c + JOIN pg_attribute AS a ON + c.confrelid = a.attrelid + AND ARRAY[attnum] <@ c.conkey + WHERE c.confrelid = table_name::REGCLASS::OID + ), + all_index_columns AS ( + SELECT + i.indexrelid::REGCLASS::STRING AS index_name, + a.attname AS col_name, + NOT i.indisunique AS non_unique, + a.attnum > i.indnkeyatts AS storing + FROM pg_index i + JOIN pg_attribute a ON a.attrelid = i.indexrelid AND a.attnum > 0 + WHERE i.indrelid = table_name::REGCLASS::OID + ), + valid_indexes AS ( + SELECT * + FROM all_index_columns + WHERE index_name NOT IN ( + SELECT DISTINCT index_name + FROM all_index_columns + WHERE col_name = column_name + AND storing = false + AND index_name NOT LIKE '%_pkey' + ) + ), + fk_col_counts AS ( + SELECT oid, count(*) AS num_cols + FROM fk + GROUP BY oid + ), + index_col_counts AS ( + SELECT index_name, count(*) AS num_cols + FROM valid_indexes + WHERE storing = false + GROUP BY index_name + ), + matching_fks AS ( + SELECT fk.oid + FROM fk + JOIN valid_indexes + ON fk.base_col = valid_indexes.col_name + JOIN fk_col_counts + ON fk.oid = fk_col_counts.oid + JOIN index_col_counts + ON valid_indexes.index_name = index_col_counts.index_name + WHERE valid_indexes.storing = false + AND valid_indexes.non_unique = false + AND fk_col_counts.num_cols = index_col_counts.num_cols + GROUP BY fk.oid, valid_indexes.index_name, fk_col_counts.num_cols + HAVING count(*) = fk_col_counts.num_cols + ) +SELECT EXISTS ( + SELECT * + FROM fk + WHERE oid NOT IN ( + SELECT DISTINCT oid FROM matching_fks + ) +) +$$; + +statement ok +CREATE TABLE parent_dep_store ( + id INT PRIMARY KEY, + key_col INT UNIQUE, + stored STRING, + FAMILY f1 (id, key_col, stored) +) WITH (schema_locked = false); + +statement ok +CREATE UNIQUE INDEX idx_parent_dep_store ON parent_dep_store (key_col) STORING (stored); + +statement ok +CREATE TABLE child_dep_store ( + id INT PRIMARY KEY, + parent_key INT REFERENCES parent_dep_store (key_col), + FAMILY f1 (id, parent_key) +) WITH (schema_locked = false); + +query B +SELECT column_drop_violates_fk_index_requirements('parent_dep_store', 'stored'); +---- +false + +statement ok +ALTER TABLE parent_dep_store DROP COLUMN stored; + +query TT +SHOW CREATE TABLE parent_dep_store +---- +parent_dep_store CREATE TABLE public.parent_dep_store ( + id INT8 NOT NULL, + key_col INT8 NULL, + CONSTRAINT parent_dep_store_pkey PRIMARY KEY (id ASC), + UNIQUE INDEX parent_dep_store_key_col_key (key_col ASC), + FAMILY f1 (id, key_col) + ); + +statement ok +DROP TABLE child_dep_store; + +statement ok +DROP TABLE parent_dep_store; + +subtest end + +subtest drop_col_dep_non_self_fk_key + +statement ok +CREATE TABLE parent_dep_key ( + id INT PRIMARY KEY, + key_col INT, + stored STRING, + FAMILY f1 (id, key_col, stored) +) WITH (schema_locked = false); + +statement ok +CREATE UNIQUE INDEX idx_parent_dep_key ON parent_dep_key (key_col) STORING (stored); + +statement ok +CREATE TABLE child_dep_key ( + id INT PRIMARY KEY, + parent_key INT REFERENCES parent_dep_key (key_col), + FAMILY f1 (id, parent_key) +) WITH (schema_locked = false); + +query B +SELECT column_drop_violates_fk_index_requirements('parent_dep_key', 'key_col'); +---- +true + +statement error pq: "idx_parent_dep_key" is referenced by foreign key from table "child_dep_key" +ALTER TABLE parent_dep_key DROP COLUMN key_col; + +query TT +SHOW CREATE TABLE parent_dep_key +---- +parent_dep_key CREATE TABLE public.parent_dep_key ( + id INT8 NOT NULL, + key_col INT8 NULL, + stored STRING NULL, + CONSTRAINT parent_dep_key_pkey PRIMARY KEY (id ASC), + UNIQUE INDEX idx_parent_dep_key (key_col ASC) STORING (stored), + FAMILY f1 (id, key_col, stored) + ); + +statement ok +DROP TABLE child_dep_key; + +statement ok +DROP TABLE parent_dep_key; + +subtest end + +subtest drop_col_dep_self_ref_stored + +statement ok +CREATE TABLE self_dep_store ( + id INT PRIMARY KEY, + ref INT, + stored STRING, + FAMILY f1 (id, ref, stored) +) WITH (schema_locked = false); + +statement ok +CREATE UNIQUE INDEX idx_self_dep_store ON self_dep_store (ref) STORING (stored); + +statement ok +ALTER TABLE self_dep_store ADD CONSTRAINT fk_self_dep_store FOREIGN KEY (ref) REFERENCES self_dep_store (id); + +query B +SELECT column_drop_violates_fk_index_requirements('self_dep_store', 'stored'); +---- +false + +statement ok +ALTER TABLE self_dep_store DROP COLUMN stored; + +query TT +SHOW CREATE TABLE self_dep_store +---- +self_dep_store CREATE TABLE public.self_dep_store ( + id INT8 NOT NULL, + ref INT8 NULL, + CONSTRAINT self_dep_store_pkey PRIMARY KEY (id ASC), + CONSTRAINT fk_self_dep_store FOREIGN KEY (ref) REFERENCES public.self_dep_store(id), + FAMILY f1 (id, ref) + ); + +statement ok +DROP TABLE self_dep_store; + +subtest end + +subtest drop_col_dep_self_ref_key + +statement ok +CREATE TABLE self_dep_key ( + id INT PRIMARY KEY, + ref INT, + stored STRING, + FAMILY f1 (id, ref, stored) +) WITH (schema_locked = false); + +statement ok +CREATE UNIQUE INDEX idx_self_dep_key ON self_dep_key (ref) STORING (stored); + +statement ok +ALTER TABLE self_dep_key ADD CONSTRAINT fk_self_dep_key FOREIGN KEY (ref) REFERENCES self_dep_key (id); + +query B +SELECT column_drop_violates_fk_index_requirements('self_dep_key', 'ref'); +---- +false + +statement ok +ALTER TABLE self_dep_key DROP COLUMN ref; + +query TT +SHOW CREATE TABLE self_dep_key +---- +self_dep_key CREATE TABLE public.self_dep_key ( + id INT8 NOT NULL, + stored STRING NULL, + CONSTRAINT self_dep_key_pkey PRIMARY KEY (id ASC), + FAMILY f1 (id, stored) + ); + +statement ok +DROP TABLE self_dep_key; + +subtest end diff --git a/pkg/workload/schemachange/error_screening.go b/pkg/workload/schemachange/error_screening.go index 8fa2d054b219..53c85180da64 100644 --- a/pkg/workload/schemachange/error_screening.go +++ b/pkg/workload/schemachange/error_screening.go @@ -149,11 +149,13 @@ func (og *operationGenerator) tableHasDependencies( return og.scanBool(ctx, tx, q, tableName.Object(), tableName.Schema(), skipSelfRef) } -// columnRemovalWillDropFKBackingIndexes determines if dropping this column -// will lead to no indexes backing a foreign key. CockroachDB will consider -// alternative indexes backing a foreign key if they have the same number of -// key columns as the FK in any order. -func (og *operationGenerator) columnRemovalWillDropFKBackingIndexes( +// columnDropViolatesFKIndexRequirements determines if dropping this column +// will violate foreign key index requirements. This occurs when dropping the +// column would leave a foreign key without a suitable backing index. +// CockroachDB requires backing indexes to have the same number of key columns +// as the FK in any order. Only key columns (not stored columns) count toward +// FK backing index requirements. +func (og *operationGenerator) columnDropViolatesFKIndexRequirements( ctx context.Context, tx pgx.Tx, tableName *tree.TableName, columName tree.Name, ) (bool, error) { return og.scanBool(ctx, tx, fmt.Sprintf(` @@ -226,6 +228,7 @@ WITH [SHOW INDEXES FROM %s] WHERE column_name = $2 + AND storing = 'f' AND index_name NOT LIKE '%%_pkey' -- renames would keep the old table name ) diff --git a/pkg/workload/schemachange/operation_generator.go b/pkg/workload/schemachange/operation_generator.go index 1829ef9e3650..f0f6e195bbaa 100644 --- a/pkg/workload/schemachange/operation_generator.go +++ b/pkg/workload/schemachange/operation_generator.go @@ -1709,7 +1709,7 @@ func (og *operationGenerator) dropColumn(ctx context.Context, tx pgx.Tx) (*opStm if err != nil { return nil, err } - columnRemovalWillDropFKBackingIndexes, err := og.columnRemovalWillDropFKBackingIndexes(ctx, tx, tableName, columnName) + columnDropViolatesFKIndexRequirements, err := og.columnDropViolatesFKIndexRequirements(ctx, tx, tableName, columnName) if err != nil { return nil, err } @@ -1731,7 +1731,7 @@ func (og *operationGenerator) dropColumn(ctx context.Context, tx pgx.Tx) (*opStm {code: pgcode.ObjectNotInPrerequisiteState, condition: columnIsInAddingOrDroppingIndex}, {code: pgcode.UndefinedColumn, condition: !columnExists}, {code: pgcode.InvalidColumnReference, condition: colIsPrimaryKey || colIsRefByComputed}, - {code: pgcode.DependentObjectsStillExist, condition: columnIsDependedOn || columnRemovalWillDropFKBackingIndexes}, + {code: pgcode.DependentObjectsStillExist, condition: columnIsDependedOn || columnDropViolatesFKIndexRequirements}, {code: pgcode.FeatureNotSupported, condition: hasAlterPKSchemaChange && !og.useDeclarativeSchemaChanger}, }) stmt.potentialExecErrors.addAll(codesWithConditions{