Skip to content

Commit b148ad2

Browse files
spilchenspilchen
committed
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
1 parent 2e5f8a0 commit b148ad2

File tree

3 files changed

+264
-7
lines changed

3 files changed

+264
-7
lines changed

‎pkg/sql/logictest/testdata/logic_test/fk‎

Lines changed: 254 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4841,3 +4841,257 @@ DROP TABLE t2_fk;
48414841
DROP TABLE t1_fk;
48424842

48434843
subtest end
4844+
4845+
subtest drop_col_dep_non_self_fk_stored
4846+
4847+
# The logic in this UDF is equivalent to and originated from
4848+
# columnDropViolatesFKIndexRequirements() in pkg/workload/schemachange/error_screening.go.
4849+
statement ok
4850+
CREATE FUNCTION column_drop_violates_fk_index_requirements(table_name STRING, column_name STRING) RETURNS BOOL LANGUAGE SQL AS $$
4851+
WITH fk AS (
4852+
SELECT
4853+
oid,
4854+
(SELECT r.relname FROM pg_class AS r WHERE r.oid = c.confrelid) AS base_table,
4855+
a.attname AS base_col,
4856+
array_position(c.confkey, a.attnum) AS base_ordinal,
4857+
(SELECT r.relname FROM pg_class AS r WHERE r.oid = c.conrelid) AS referencing_table,
4858+
unnest(
4859+
(SELECT array_agg(attname)
4860+
FROM pg_attribute
4861+
WHERE attrelid = c.conrelid
4862+
AND ARRAY[attnum] <@ c.conkey
4863+
AND array_position(c.confkey, a.attnum) = array_position(c.conkey, attnum))
4864+
) AS referencing_col
4865+
FROM pg_constraint AS c
4866+
JOIN pg_attribute AS a ON
4867+
c.confrelid = a.attrelid
4868+
AND ARRAY[attnum] <@ c.conkey
4869+
WHERE c.confrelid = table_name::REGCLASS::OID
4870+
),
4871+
all_index_columns AS (
4872+
SELECT
4873+
i.indexrelid::REGCLASS::STRING AS index_name,
4874+
a.attname AS col_name,
4875+
NOT i.indisunique AS non_unique,
4876+
a.attnum > i.indnkeyatts AS storing
4877+
FROM pg_index i
4878+
JOIN pg_attribute a ON a.attrelid = i.indexrelid AND a.attnum > 0
4879+
WHERE i.indrelid = table_name::REGCLASS::OID
4880+
),
4881+
valid_indexes AS (
4882+
SELECT *
4883+
FROM all_index_columns
4884+
WHERE index_name NOT IN (
4885+
SELECT DISTINCT index_name
4886+
FROM all_index_columns
4887+
WHERE col_name = column_name
4888+
AND storing = false
4889+
AND index_name NOT LIKE '%_pkey'
4890+
)
4891+
),
4892+
fk_col_counts AS (
4893+
SELECT oid, count(*) AS num_cols
4894+
FROM fk
4895+
GROUP BY oid
4896+
),
4897+
index_col_counts AS (
4898+
SELECT index_name, count(*) AS num_cols
4899+
FROM valid_indexes
4900+
WHERE storing = false
4901+
GROUP BY index_name
4902+
),
4903+
matching_fks AS (
4904+
SELECT fk.oid
4905+
FROM fk
4906+
JOIN valid_indexes
4907+
ON fk.base_col = valid_indexes.col_name
4908+
JOIN fk_col_counts
4909+
ON fk.oid = fk_col_counts.oid
4910+
JOIN index_col_counts
4911+
ON valid_indexes.index_name = index_col_counts.index_name
4912+
WHERE valid_indexes.storing = false
4913+
AND valid_indexes.non_unique = false
4914+
AND fk_col_counts.num_cols = index_col_counts.num_cols
4915+
GROUP BY fk.oid, valid_indexes.index_name, fk_col_counts.num_cols
4916+
HAVING count(*) = fk_col_counts.num_cols
4917+
)
4918+
SELECT EXISTS (
4919+
SELECT *
4920+
FROM fk
4921+
WHERE oid NOT IN (
4922+
SELECT DISTINCT oid FROM matching_fks
4923+
)
4924+
)
4925+
$$;
4926+
4927+
statement ok
4928+
CREATE TABLE parent_dep_store (
4929+
id INT PRIMARY KEY,
4930+
key_col INT UNIQUE,
4931+
stored STRING,
4932+
FAMILY f1 (id, key_col, stored)
4933+
) WITH (schema_locked = false);
4934+
4935+
statement ok
4936+
CREATE UNIQUE INDEX idx_parent_dep_store ON parent_dep_store (key_col) STORING (stored);
4937+
4938+
statement ok
4939+
CREATE TABLE child_dep_store (
4940+
id INT PRIMARY KEY,
4941+
parent_key INT REFERENCES parent_dep_store (key_col),
4942+
FAMILY f1 (id, parent_key)
4943+
) WITH (schema_locked = false);
4944+
4945+
query B
4946+
SELECT column_drop_violates_fk_index_requirements('parent_dep_store', 'stored');
4947+
----
4948+
false
4949+
4950+
statement ok
4951+
ALTER TABLE parent_dep_store DROP COLUMN stored;
4952+
4953+
query TT
4954+
SHOW CREATE TABLE parent_dep_store
4955+
----
4956+
parent_dep_store CREATE TABLE public.parent_dep_store (
4957+
id INT8 NOT NULL,
4958+
key_col INT8 NULL,
4959+
CONSTRAINT parent_dep_store_pkey PRIMARY KEY (id ASC),
4960+
UNIQUE INDEX parent_dep_store_key_col_key (key_col ASC),
4961+
FAMILY f1 (id, key_col)
4962+
);
4963+
4964+
statement ok
4965+
DROP TABLE child_dep_store;
4966+
4967+
statement ok
4968+
DROP TABLE parent_dep_store;
4969+
4970+
subtest end
4971+
4972+
subtest drop_col_dep_non_self_fk_key
4973+
4974+
statement ok
4975+
CREATE TABLE parent_dep_key (
4976+
id INT PRIMARY KEY,
4977+
key_col INT,
4978+
stored STRING,
4979+
FAMILY f1 (id, key_col, stored)
4980+
) WITH (schema_locked = false);
4981+
4982+
statement ok
4983+
CREATE UNIQUE INDEX idx_parent_dep_key ON parent_dep_key (key_col) STORING (stored);
4984+
4985+
statement ok
4986+
CREATE TABLE child_dep_key (
4987+
id INT PRIMARY KEY,
4988+
parent_key INT REFERENCES parent_dep_key (key_col),
4989+
FAMILY f1 (id, parent_key)
4990+
) WITH (schema_locked = false);
4991+
4992+
query B
4993+
SELECT column_drop_violates_fk_index_requirements('parent_dep_key', 'key_col');
4994+
----
4995+
true
4996+
4997+
statement error pq: "idx_parent_dep_key" is referenced by foreign key from table "child_dep_key"
4998+
ALTER TABLE parent_dep_key DROP COLUMN key_col;
4999+
5000+
query TT
5001+
SHOW CREATE TABLE parent_dep_key
5002+
----
5003+
parent_dep_key CREATE TABLE public.parent_dep_key (
5004+
id INT8 NOT NULL,
5005+
key_col INT8 NULL,
5006+
stored STRING NULL,
5007+
CONSTRAINT parent_dep_key_pkey PRIMARY KEY (id ASC),
5008+
UNIQUE INDEX idx_parent_dep_key (key_col ASC) STORING (stored),
5009+
FAMILY f1 (id, key_col, stored)
5010+
);
5011+
5012+
statement ok
5013+
DROP TABLE child_dep_key;
5014+
5015+
statement ok
5016+
DROP TABLE parent_dep_key;
5017+
5018+
subtest end
5019+
5020+
subtest drop_col_dep_self_ref_stored
5021+
5022+
statement ok
5023+
CREATE TABLE self_dep_store (
5024+
id INT PRIMARY KEY,
5025+
ref INT,
5026+
stored STRING,
5027+
FAMILY f1 (id, ref, stored)
5028+
) WITH (schema_locked = false);
5029+
5030+
statement ok
5031+
CREATE UNIQUE INDEX idx_self_dep_store ON self_dep_store (ref) STORING (stored);
5032+
5033+
statement ok
5034+
ALTER TABLE self_dep_store ADD CONSTRAINT fk_self_dep_store FOREIGN KEY (ref) REFERENCES self_dep_store (id);
5035+
5036+
query B
5037+
SELECT column_drop_violates_fk_index_requirements('self_dep_store', 'stored');
5038+
----
5039+
false
5040+
5041+
statement ok
5042+
ALTER TABLE self_dep_store DROP COLUMN stored;
5043+
5044+
query TT
5045+
SHOW CREATE TABLE self_dep_store
5046+
----
5047+
self_dep_store CREATE TABLE public.self_dep_store (
5048+
id INT8 NOT NULL,
5049+
ref INT8 NULL,
5050+
CONSTRAINT self_dep_store_pkey PRIMARY KEY (id ASC),
5051+
CONSTRAINT fk_self_dep_store FOREIGN KEY (ref) REFERENCES public.self_dep_store(id),
5052+
FAMILY f1 (id, ref)
5053+
);
5054+
5055+
statement ok
5056+
DROP TABLE self_dep_store;
5057+
5058+
subtest end
5059+
5060+
subtest drop_col_dep_self_ref_key
5061+
5062+
statement ok
5063+
CREATE TABLE self_dep_key (
5064+
id INT PRIMARY KEY,
5065+
ref INT,
5066+
stored STRING,
5067+
FAMILY f1 (id, ref, stored)
5068+
) WITH (schema_locked = false);
5069+
5070+
statement ok
5071+
CREATE UNIQUE INDEX idx_self_dep_key ON self_dep_key (ref) STORING (stored);
5072+
5073+
statement ok
5074+
ALTER TABLE self_dep_key ADD CONSTRAINT fk_self_dep_key FOREIGN KEY (ref) REFERENCES self_dep_key (id);
5075+
5076+
query B
5077+
SELECT column_drop_violates_fk_index_requirements('self_dep_key', 'ref');
5078+
----
5079+
false
5080+
5081+
statement ok
5082+
ALTER TABLE self_dep_key DROP COLUMN ref;
5083+
5084+
query TT
5085+
SHOW CREATE TABLE self_dep_key
5086+
----
5087+
self_dep_key CREATE TABLE public.self_dep_key (
5088+
id INT8 NOT NULL,
5089+
stored STRING NULL,
5090+
CONSTRAINT self_dep_key_pkey PRIMARY KEY (id ASC),
5091+
FAMILY f1 (id, stored)
5092+
);
5093+
5094+
statement ok
5095+
DROP TABLE self_dep_key;
5096+
5097+
subtest end

‎pkg/workload/schemachange/error_screening.go‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -149,11 +149,13 @@ func (og *operationGenerator) tableHasDependencies(
149149
return og.scanBool(ctx, tx, q, tableName.Object(), tableName.Schema(), skipSelfRef)
150150
}
151151

152-
// columnRemovalWillDropFKBackingIndexes determines if dropping this column
153-
// will lead to no indexes backing a foreign key. CockroachDB will consider
154-
// alternative indexes backing a foreign key if they have the same number of
155-
// key columns as the FK in any order.
156-
func (og *operationGenerator) columnRemovalWillDropFKBackingIndexes(
152+
// columnDropViolatesFKIndexRequirements determines if dropping this column
153+
// will violate foreign key index requirements. This occurs when dropping the
154+
// column would leave a foreign key without a suitable backing index.
155+
// CockroachDB requires backing indexes to have the same number of key columns
156+
// as the FK in any order. Only key columns (not stored columns) count toward
157+
// FK backing index requirements.
158+
func (og *operationGenerator) columnDropViolatesFKIndexRequirements(
157159
ctx context.Context, tx pgx.Tx, tableName *tree.TableName, columName tree.Name,
158160
) (bool, error) {
159161
return og.scanBool(ctx, tx, fmt.Sprintf(`
@@ -226,6 +228,7 @@ WITH
226228
[SHOW INDEXES FROM %s]
227229
WHERE
228230
column_name = $2
231+
AND storing = 'f'
229232
AND index_name
230233
NOT LIKE '%%_pkey' -- renames would keep the old table name
231234
)

‎pkg/workload/schemachange/operation_generator.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1709,7 +1709,7 @@ func (og *operationGenerator) dropColumn(ctx context.Context, tx pgx.Tx) (*opStm
17091709
if err != nil {
17101710
return nil, err
17111711
}
1712-
columnRemovalWillDropFKBackingIndexes, err := og.columnRemovalWillDropFKBackingIndexes(ctx, tx, tableName, columnName)
1712+
columnDropViolatesFKIndexRequirements, err := og.columnDropViolatesFKIndexRequirements(ctx, tx, tableName, columnName)
17131713
if err != nil {
17141714
return nil, err
17151715
}
@@ -1731,7 +1731,7 @@ func (og *operationGenerator) dropColumn(ctx context.Context, tx pgx.Tx) (*opStm
17311731
{code: pgcode.ObjectNotInPrerequisiteState, condition: columnIsInAddingOrDroppingIndex},
17321732
{code: pgcode.UndefinedColumn, condition: !columnExists},
17331733
{code: pgcode.InvalidColumnReference, condition: colIsPrimaryKey || colIsRefByComputed},
1734-
{code: pgcode.DependentObjectsStillExist, condition: columnIsDependedOn || columnRemovalWillDropFKBackingIndexes},
1734+
{code: pgcode.DependentObjectsStillExist, condition: columnIsDependedOn || columnDropViolatesFKIndexRequirements},
17351735
{code: pgcode.FeatureNotSupported, condition: hasAlterPKSchemaChange && !og.useDeclarativeSchemaChanger},
17361736
})
17371737
stmt.potentialExecErrors.addAll(codesWithConditions{

0 commit comments

Comments
 (0)