From a8078f85859400f4e4cffb57d8ec1b069e46bfb8 Mon Sep 17 00:00:00 2001 From: "Paul A. Jungwirth" Date: Sun, 24 Mar 2024 23:37:48 -0700 Subject: [PATCH v1] Re-check foreign key if referenced collation was nondeterministic With deterministic collations, we break ties by looking at binary equality. But nondeterministic collations can allow non-identical values to be considered equal, e.g. 'a' and 'A' when case-insensitive. So some references that used to be valid may not be anymore. --- src/backend/commands/tablecmds.c | 96 ++++++++++++++++--- src/include/nodes/parsenodes.h | 2 + .../regress/expected/collate.icu.utf8.out | 9 ++ src/test/regress/sql/collate.icu.utf8.sql | 8 ++ 4 files changed, 103 insertions(+), 12 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 71740984f33..940b1845edb 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -194,6 +194,7 @@ typedef struct AlteredTableInfo /* Objects to rebuild after completing ALTER TYPE operations */ List *changedConstraintOids; /* OIDs of constraints to rebuild */ List *changedConstraintDefs; /* string definitions of same */ + List *changedCollationOids; /* collation of each attnum */ List *changedIndexOids; /* OIDs of indexes to rebuild */ List *changedIndexDefs; /* string definitions of same */ char *replicaIdentityIndex; /* index to reset as REPLICA IDENTITY */ @@ -572,6 +573,7 @@ static ObjectAddress ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel, AlterTableCmd *cmd, LOCKMODE lockmode); static void RememberAllDependentForRebuilding(AlteredTableInfo *tab, AlterTableType subtype, Relation rel, AttrNumber attnum, const char *colName); +static void RememberCollationForRebuilding(AttrNumber attnum, AlteredTableInfo *tab); static void RememberConstraintForRebuilding(Oid conoid, AlteredTableInfo *tab); static void RememberIndexForRebuilding(Oid indoid, AlteredTableInfo *tab); static void RememberStatisticsForRebuilding(Oid stxoid, AlteredTableInfo *tab); @@ -579,12 +581,12 @@ static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode); static void ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd, List **wqueue, LOCKMODE lockmode, - bool rewrite); + bool rewrite, List *collationOids); static void RebuildConstraintComment(AlteredTableInfo *tab, AlterTablePass pass, Oid objid, Relation rel, List *domname, const char *conname); static void TryReuseIndex(Oid oldId, IndexStmt *stmt); -static void TryReuseForeignKey(Oid oldId, Constraint *con); +static void TryReuseForeignKey(Oid oldId, Constraint *con, List *changedCollationOids); static ObjectAddress ATExecAlterColumnGenericOptions(Relation rel, const char *colName, List *options, LOCKMODE lockmode); static void change_owner_fix_column_acls(Oid relationOid, @@ -9826,6 +9828,7 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, bool old_check_ok; ObjectAddress address; ListCell *old_pfeqop_item = list_head(fkconstraint->old_conpfeqop); + ListCell *old_collation_item = list_head(fkconstraint->old_collations); /* * Grab ShareRowExclusiveLock on the pk table, so that someone doesn't @@ -10212,9 +10215,15 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, CoercionPathType new_pathtype; Oid old_castfunc; Oid new_castfunc; + Oid old_collation; Form_pg_attribute attr = TupleDescAttr(tab->oldDesc, fkattnum[i] - 1); + /* Get the collation for this key part. */ + old_collation = lfirst_oid(old_collation_item); + old_collation_item = lnext(fkconstraint->old_collations, + old_collation_item); + /* * Identify coercion pathways from each of the old and new FK-side * column types to the right (foreign) operand type of the pfeqop. @@ -10250,9 +10259,12 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, * turn conform to the domain. Consequently, we need not treat * domains specially here. * - * Since we require that all collations share the same notion of - * equality (which they do, because texteq reduces to bitwise - * equality), we don't compare collation here. + * All deterministic collations use bitwise equality to resolve + * ties, but if the previous collation was nondeterministic, + * we must re-check the foreign key, because some references + * that use to be "equal" may not be anymore. If we have + * InvalidOid here, either there was no collation or the + * attribute didn't change. * * We need not directly consider the PK type. It's necessarily * binary coercible to the opcintype of the unique index column, @@ -10261,6 +10273,8 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, */ old_check_ok = (new_pathtype == old_pathtype && new_castfunc == old_castfunc && + (old_collation == InvalidOid || + get_collation_isdeterministic(old_collation)) && (!IsPolymorphicType(pfeqop_right) || new_fktype == old_fktype)); } @@ -13985,6 +13999,7 @@ RememberAllDependentForRebuilding(AlteredTableInfo *tab, AlterTableType subtype, relKind == RELKIND_PARTITIONED_INDEX) { Assert(foundObject.objectSubId == 0); + RememberCollationForRebuilding(attnum, tab); RememberIndexForRebuilding(foundObject.objectId, tab); } else if (relKind == RELKIND_SEQUENCE) @@ -14006,6 +14021,7 @@ RememberAllDependentForRebuilding(AlteredTableInfo *tab, AlterTableType subtype, case OCLASS_CONSTRAINT: Assert(foundObject.objectSubId == 0); + RememberCollationForRebuilding(attnum, tab); RememberConstraintForRebuilding(foundObject.objectId, tab); break; @@ -14188,6 +14204,40 @@ RememberClusterOnForRebuilding(Oid indoid, AlteredTableInfo *tab) tab->clusterOnIndex = get_rel_name(indoid); } +/* + * Subroutine for ATExecAlterColumnType: remember what the collations were + * for each attribute of the table. This is because if one of them used to be + * nondeterministic, foreign keys that referenced that attribute must be + * re-checked. We make a list with one entry for each attribute in the table, + * so that FKs can easily look them up by attribute number. But only attributes + * that are changing get a non-zero value. + */ +static void +RememberCollationForRebuilding(AttrNumber attnum, AlteredTableInfo *tab) +{ + Oid typid; + int32 typmod; + Oid collid; + ListCell *lc; + + /* Fill in the list with InvalidOid if this is our first visit */ + if (tab->changedCollationOids == NIL) + { + int len = RelationGetNumberOfAttributes(tab->rel); + int i; + + for (i = 0; i < len; i++) + tab->changedCollationOids = lappend_oid(tab->changedCollationOids, + InvalidOid); + } + + get_atttypetypmodcoll(RelationGetRelid(tab->rel), attnum, + &typid, &typmod, &collid); + + lc = list_nth_cell(tab->changedCollationOids, attnum - 1); + lfirst_oid(lc) = collid; +} + /* * Subroutine for ATExecAlterColumnType: remember that a constraint needs * to be rebuilt (which we might already know). @@ -14393,7 +14443,8 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode) ATPostAlterTypeParse(oldId, relid, confrelid, (char *) lfirst(def_item), - wqueue, lockmode, tab->rewrite); + wqueue, lockmode, tab->rewrite, + tab->changedCollationOids); } forboth(oid_item, tab->changedIndexOids, def_item, tab->changedIndexDefs) @@ -14404,7 +14455,8 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode) relid = IndexGetRelation(oldId, false); ATPostAlterTypeParse(oldId, relid, InvalidOid, (char *) lfirst(def_item), - wqueue, lockmode, tab->rewrite); + wqueue, lockmode, tab->rewrite, + tab->changedCollationOids); ObjectAddressSet(obj, RelationRelationId, oldId); add_exact_object_address(&obj, objects); @@ -14420,7 +14472,8 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode) relid = StatisticsGetRelation(oldId, false); ATPostAlterTypeParse(oldId, relid, InvalidOid, (char *) lfirst(def_item), - wqueue, lockmode, tab->rewrite); + wqueue, lockmode, tab->rewrite, + tab->changedCollationOids); ObjectAddressSet(obj, StatisticExtRelationId, oldId); add_exact_object_address(&obj, objects); @@ -14483,7 +14536,8 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode) */ static void ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd, - List **wqueue, LOCKMODE lockmode, bool rewrite) + List **wqueue, LOCKMODE lockmode, bool rewrite, + List *changedCollationOids) { List *raw_parsetree_list; List *querytree_list; @@ -14610,7 +14664,7 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd, /* rewriting neither side of a FK */ if (con->contype == CONSTR_FOREIGN && !rewrite && tab->rewrite == 0) - TryReuseForeignKey(oldId, con); + TryReuseForeignKey(oldId, con, changedCollationOids); con->reset_default_tblspc = true; cmd->subtype = AT_ReAddConstraint; tab->subcmds[AT_PASS_OLD_CONSTR] = @@ -14768,20 +14822,22 @@ TryReuseIndex(Oid oldId, IndexStmt *stmt) * * Stash the old P-F equality operator into the Constraint node, for possible * use by ATAddForeignKeyConstraint() in determining whether revalidation of - * this constraint can be skipped. + * this constraint can be skipped. Also stash the collations. */ static void -TryReuseForeignKey(Oid oldId, Constraint *con) +TryReuseForeignKey(Oid oldId, Constraint *con, List *changedCollationOids) { HeapTuple tup; Datum adatum; ArrayType *arr; Oid *rawarr; + AttrNumber *attarr; int numkeys; int i; Assert(con->contype == CONSTR_FOREIGN); Assert(con->old_conpfeqop == NIL); /* already prepared this node */ + Assert(con->old_collations == NIL); /* already prepared this node */ tup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(oldId)); if (!HeapTupleIsValid(tup)) /* should not happen */ @@ -14802,6 +14858,22 @@ TryReuseForeignKey(Oid oldId, Constraint *con) for (i = 0; i < numkeys; i++) con->old_conpfeqop = lappend_oid(con->old_conpfeqop, rawarr[i]); + adatum = SysCacheGetAttrNotNull(CONSTROID, tup, + Anum_pg_constraint_conkey); + arr = DatumGetArrayTypeP(adatum); /* ensure not toasted */ + numkeys = ARR_DIMS(arr)[0]; + /* test follows the one in ri_FetchConstraintInfo() */ + if (ARR_NDIM(arr) != 1 || + ARR_HASNULL(arr) || + ARR_ELEMTYPE(arr) != INT2OID) + elog(ERROR, "conkey is not a 1-D smallint array"); + attarr = (AttrNumber *) ARR_DATA_PTR(arr); + + /* stash a List of the collation Oids in our Constraint node */ + for (i = 0; i < numkeys; i++) + con->old_collations = lappend_oid(con->old_collations, + list_nth_oid(changedCollationOids, attarr[i] - 1)); + ReleaseSysCache(tup); } diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index b89baef95d3..96c71c75115 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -2691,6 +2691,8 @@ typedef struct Constraint char fk_del_action; /* ON DELETE action */ List *fk_del_set_cols; /* ON DELETE SET NULL/DEFAULT (col1, col2) */ List *old_conpfeqop; /* pg_constraint.conpfeqop of my former self */ + List *old_collations; /* collations of referenced columns, + before change */ Oid old_pktable_oid; /* pg_constraint.confrelid of my former * self */ diff --git a/src/test/regress/expected/collate.icu.utf8.out b/src/test/regress/expected/collate.icu.utf8.out index 4b8c8f143f3..4119776f0d8 100644 --- a/src/test/regress/expected/collate.icu.utf8.out +++ b/src/test/regress/expected/collate.icu.utf8.out @@ -1933,6 +1933,15 @@ SELECT * FROM test11fk; --- (0 rows) +-- Test altering the collation of the referenced column. +CREATE TABLE pktable (x text COLLATE case_insensitive PRIMARY KEY); +CREATE TABLE fktable (x text REFERENCES pktable); +INSERT INTO pktable VALUES ('a'); +INSERT INTO fktable VALUES ('A'); +-- should fail: +ALTER TABLE pktable ALTER COLUMN x TYPE text COLLATE "C"; +ERROR: insert or update on table "fktable" violates foreign key constraint "fktable_x_fkey" +DETAIL: Key (x)=(A) is not present in table "pktable". -- partitioning CREATE TABLE test20 (a int, b text COLLATE case_insensitive) PARTITION BY LIST (b); CREATE TABLE test20_1 PARTITION OF test20 FOR VALUES IN ('abc'); diff --git a/src/test/regress/sql/collate.icu.utf8.sql b/src/test/regress/sql/collate.icu.utf8.sql index 80f28a97d78..08a6964950b 100644 --- a/src/test/regress/sql/collate.icu.utf8.sql +++ b/src/test/regress/sql/collate.icu.utf8.sql @@ -721,6 +721,14 @@ DELETE FROM test11pk WHERE x = 'abc'; SELECT * FROM test11pk; SELECT * FROM test11fk; +-- Test altering the collation of the referenced column. +CREATE TABLE pktable (x text COLLATE case_insensitive PRIMARY KEY); +CREATE TABLE fktable (x text REFERENCES pktable); +INSERT INTO pktable VALUES ('a'); +INSERT INTO fktable VALUES ('A'); +-- should fail: +ALTER TABLE pktable ALTER COLUMN x TYPE text COLLATE "C"; + -- partitioning CREATE TABLE test20 (a int, b text COLLATE case_insensitive) PARTITION BY LIST (b); CREATE TABLE test20_1 PARTITION OF test20 FOR VALUES IN ('abc'); -- 2.42.0