Subject: Re: altering a column's collation leaves an invalid foreign key

From: Paul Jungwirth <[email-masked]>
Date: Sun, 24 Mar 2024 23:47:26 -0700

On 3/23/24 10:04, Paul Jungwirth wrote:
> Perhaps if the previous collation was nondeterministic we should force a re-check.

Here is a patch implementing this. It was a bit more fuss than I expected, so maybe someone has a 
better way.

We have had nondeterministic collations since v12, so perhaps it is something to back-patch?

Yours,

-- 
Paul              ~{:-)
[email-masked]

From a8078f85859400f4e4cffb57d8ec1b069e46bfb8 Mon Sep 17 00:00:00 2001
From: "Paul A. Jungwirth" <[email-masked]>
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