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 ~{:-)
p...@illuminatedcomputing.comFrom a8078f85859400f4e4cffb57d8ec1b069e46bfb8 Mon Sep 17 00:00:00 2001
From: "Paul A. Jungwirth" <p...@illuminatedcomputing.com>
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
