Skip to content

Commit be40e3b

Browse files
craig[bot]spilchen
craig[bot]
and
spilchen
committed
Merge #158133
158133: workload/schemachange: fix FK dependency check for drop column logic r=spilchen a=spilchen 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 Co-authored-by: Matt Spilchen <matt.spilchen@cockroachlabs.com>
2 parents 29d8485 + b148ad2 commit be40e3b

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
@@ -4839,3 +4839,257 @@ DROP TABLE t2_fk;
48394839
DROP TABLE t1_fk;
48404840

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