altering a column's collation leaves an invalid foreign key

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 ~{:-)
pj@illuminatedcomputing.com

On Mon, Mar 25, 2024 at 2:47 PM Paul Jungwirth
<pj@illuminatedcomputing.com> wrote:

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.

+ /* 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));

I don't understand the "ri_FetchConstraintInfo" comment.

+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;
+}

do we need to check if `collid` is a valid collation?
like:

if (!OidIsValid(collid))
{
lc = list_nth_cell(tab->changedCollationOids, attnum - 1);
lfirst_oid(lc) = collid;
}

On Tue, Mar 26, 2024 at 1:00 PM jian he <jian.universality@gmail.com> wrote:

On Mon, Mar 25, 2024 at 2:47 PM Paul Jungwirth
<pj@illuminatedcomputing.com> wrote:

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.

+ /* 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));

I don't understand the "ri_FetchConstraintInfo" comment.

I still don't understand this comment.

+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;
+}

do we need to check if `collid` is a valid collation?
like:

if (!OidIsValid(collid))
{
lc = list_nth_cell(tab->changedCollationOids, attnum - 1);
lfirst_oid(lc) = collid;
}

now I think RememberCollationForRebuilding is fine. no need further change.

in ATAddForeignKeyConstraint.
if (old_check_ok)
{
/*
* When a pfeqop changes, revalidate the constraint. We could
* permit intra-opfamily changes, but that adds subtle complexity
* without any concrete benefit for core types. We need not
* assess ppeqop or ffeqop, which RI_Initial_Check() does not use.
*/
old_check_ok = (pfeqop == lfirst_oid(old_pfeqop_item));
old_pfeqop_item = lnext(fkconstraint->old_conpfeqop,
old_pfeqop_item);
}
maybe we can do the logic right below it. like:

if (old_check_ok)
{

* 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.
old_check_ok = (old_collation == InvalidOid ||
get_collation_isdeterministic(old_collation));
}
then we don't need to cram it with the other `if (old_check_ok){}`.

do we need to add an entry in
https://wiki.postgresql.org/wiki/PostgreSQL_17_Open_Items#Open_Issues
?

On Fri, Apr 12, 2024 at 5:06 PM jian he <jian.universality@gmail.com> wrote:

On Tue, Mar 26, 2024 at 1:00 PM jian he <jian.universality@gmail.com> wrote:

On Mon, Mar 25, 2024 at 2:47 PM Paul Jungwirth
<pj@illuminatedcomputing.com> wrote:

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.

I think I found a simple way.

the logic is:
* ATExecAlterColumnType changes one column once at a time.
* one column can only have one collation. so we don't need to store a
list of collation oid.
* ATExecAlterColumnType we can get the new collation (targetcollid)
and original collation info.
* RememberAllDependentForRebuilding will check the column dependent,
whether this column is referenced by a foreign key or not information
is recorded.
so AlteredTableInfo->changedConstraintOids have the primary key and
foreign key oids.
* ATRewriteCatalogs will call ATPostAlterTypeCleanup (see the comments
in ATRewriteCatalogs)
* for tab->changedConstraintOids (foreign key, primary key) will call
ATPostAlterTypeParse, so
for foreign key (con->contype == CONSTR_FOREIGN) will call TryReuseForeignKey.
* in TryReuseForeignKey, we can pass the information that our primary
key old collation is nondeterministic
and old collation != new collation to the foreign key constraint.
so foreign key can act accordingly at ATAddForeignKeyConstraint (old_check_ok).

based on the above logic, I add one bool in struct AlteredTableInfo,
one bool in struct Constraint.
bool in AlteredTableInfo is for storing it, later passing it to struct
Constraint.
we need bool in Constraint because ATAddForeignKeyConstraint needs it.

On Sat, Apr 13, 2024 at 9:13 PM jian he <jian.universality@gmail.com> wrote:

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

I think I found a simple way.

the logic is:
* ATExecAlterColumnType changes one column once at a time.
* one column can only have one collation. so we don't need to store a
list of collation oid.
* ATExecAlterColumnType we can get the new collation (targetcollid)
and original collation info.
* RememberAllDependentForRebuilding will check the column dependent,
whether this column is referenced by a foreign key or not information
is recorded.
so AlteredTableInfo->changedConstraintOids have the primary key and
foreign key oids.
* ATRewriteCatalogs will call ATPostAlterTypeCleanup (see the comments
in ATRewriteCatalogs)
* for tab->changedConstraintOids (foreign key, primary key) will call
ATPostAlterTypeParse, so
for foreign key (con->contype == CONSTR_FOREIGN) will call TryReuseForeignKey.
* in TryReuseForeignKey, we can pass the information that our primary
key old collation is nondeterministic
and old collation != new collation to the foreign key constraint.
so foreign key can act accordingly at ATAddForeignKeyConstraint (old_check_ok).

based on the above logic, I add one bool in struct AlteredTableInfo,
one bool in struct Constraint.
bool in AlteredTableInfo is for storing it, later passing it to struct
Constraint.
we need bool in Constraint because ATAddForeignKeyConstraint needs it.

I refactored the comments.
also added some extra tests hoping to make it more bullet proof, maybe
it's redundant.

jian he <jian.universality@gmail.com> writes:

* in TryReuseForeignKey, we can pass the information that our primary
key old collation is nondeterministic
and old collation != new collation to the foreign key constraint.

I have a basic question about this: why are we allowing FKs to be
based on nondeterministic collations at all? ISTM that that breaks
the assumption that there is exactly one referenced row for any
referencing row.

regards, tom lane

On Sat, Jun 8, 2024 at 4:12 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

jian he <jian.universality@gmail.com> writes:

* in TryReuseForeignKey, we can pass the information that our primary
key old collation is nondeterministic
and old collation != new collation to the foreign key constraint.

I have a basic question about this: why are we allowing FKs to be
based on nondeterministic collations at all? ISTM that that breaks
the assumption that there is exactly one referenced row for any
referencing row.

for FKs nondeterministic,
I think that would require the PRIMARY KEY collation to not be
indeterministic also.

for example:
CREATE COLLATION ignore_accent_case (provider = icu, deterministic =
false, locale = 'und-u-ks-level1');
DROP TABLE IF EXISTS fktable, pktable;
CREATE TABLE pktable (x text COLLATE ignore_accent_case PRIMARY KEY);
CREATE TABLE fktable (x text REFERENCES pktable on update cascade on
delete cascade);
INSERT INTO pktable VALUES ('A');
INSERT INTO fktable VALUES ('a');
INSERT INTO fktable VALUES ('A');
update pktable set x = 'Å';
table fktable;

if FK is nondeterministic, then it looks PK more like FK.
the following example, one FK row is referenced by two PK rows.

DROP TABLE IF EXISTS fktable, pktable;
CREATE TABLE pktable (x text COLLATE "C" PRIMARY KEY);
CREATE TABLE fktable (x text COLLATE ignore_accent_case REFERENCES
pktable on update cascade on delete cascade);
INSERT INTO pktable VALUES ('A'), ('Å');
INSERT INTO fktable VALUES ('A');

begin; delete from pktable where x = 'Å'; TABLE fktable; rollback;
begin; delete from pktable where x = 'A'; TABLE fktable; rollback;

On 08.06.24 06:14, jian he wrote:

if FK is nondeterministic, then it looks PK more like FK.
the following example, one FK row is referenced by two PK rows.

DROP TABLE IF EXISTS fktable, pktable;
CREATE TABLE pktable (x text COLLATE "C" PRIMARY KEY);
CREATE TABLE fktable (x text COLLATE ignore_accent_case REFERENCES
pktable on update cascade on delete cascade);
INSERT INTO pktable VALUES ('A'), ('Å');
INSERT INTO fktable VALUES ('A');

Yes, this is a problem. The RI checks are done with the collation of
the primary key.

The comment at ri_GenerateQualCollation() says "the SQL standard
specifies that RI comparisons should use the referenced column's
collation". But this is not what it says in my current copy.

... [ digs around ISO archives ] ...

Yes, this was changed in SQL:2016 to require the collation on the PK
side and the FK side to match at constraint creation time. The argument
made is exactly the same we have here. This was actually already the
rule in SQL:1999 but was then relaxed in SQL:2003 and then changed back
because it was a mistake.

We probably don't need to enforce this for deterministic collations,
which would preserve some backward compatibility.

I'll think some more about what steps to take to solve this and what to
do about back branches etc.

On Tue, Jun 18, 2024 at 4:50 PM Peter Eisentraut <peter@eisentraut.org> wrote:

On 08.06.24 06:14, jian he wrote:

if FK is nondeterministic, then it looks PK more like FK.
the following example, one FK row is referenced by two PK rows.

DROP TABLE IF EXISTS fktable, pktable;
CREATE TABLE pktable (x text COLLATE "C" PRIMARY KEY);
CREATE TABLE fktable (x text COLLATE ignore_accent_case REFERENCES
pktable on update cascade on delete cascade);
INSERT INTO pktable VALUES ('A'), ('Å');
INSERT INTO fktable VALUES ('A');

Yes, this is a problem. The RI checks are done with the collation of
the primary key.

The comment at ri_GenerateQualCollation() says "the SQL standard
specifies that RI comparisons should use the referenced column's
collation". But this is not what it says in my current copy.

... [ digs around ISO archives ] ...

Yes, this was changed in SQL:2016 to require the collation on the PK
side and the FK side to match at constraint creation time. The argument
made is exactly the same we have here. This was actually already the
rule in SQL:1999 but was then relaxed in SQL:2003 and then changed back
because it was a mistake.

We probably don't need to enforce this for deterministic collations,
which would preserve some backward compatibility.

I'll think some more about what steps to take to solve this and what to
do about back branches etc.

I have come up with 3 corner cases.

---case1. not ok. PK indeterministic, FK default
DROP TABLE IF EXISTS fktable, pktable;
CREATE TABLE pktable (x text COLLATE ignore_accent_case PRIMARY KEY);
CREATE TABLE fktable (x text REFERENCES pktable on update cascade on
delete cascade);
INSERT INTO pktable VALUES ('A');
INSERT INTO fktable VALUES ('a');
INSERT INTO fktable VALUES ('A');

RI_FKey_check (Check foreign key existence ) querybuf.data is
SELECT 1 FROM ONLY "public"."pktable" x WHERE "x"
OPERATOR(pg_catalog.=) $1 FOR KEY SHARE OF x
in here, fktable doesn't have collation.
we cannot rewrite to
SELECT 1 FROM ONLY "public"."pktable" x WHERE "x"
OPERATOR(pg_catalog.=) $1 collate "C" FOR KEY SHARE OF x
so assumption (only one referenced row for any referencing row) will
break when inserting values to fktable.
RI_FKey_check already allows invalidate values to happen, not sure how
ri_GenerateQualCollation can help.
overall i don't know how to stop invalid value while inserting value
to fktable in this case.

---case2. not ok case PK deterministic, FK indeterministic
DROP TABLE IF EXISTS fktable, pktable;
CREATE TABLE pktable (x text COLLATE "C" PRIMARY KEY);
CREATE TABLE fktable (x text COLLATE ignore_accent_case REFERENCES
pktable on update cascade on delete cascade);
INSERT INTO pktable VALUES ('A'), ('Å');
INSERT INTO fktable VALUES ('A');
begin; update pktable set x = 'B' where x = 'Å'; table fktable; rollback;
begin; update pktable set x = 'B' where x = 'A'; table fktable; rollback;

when cascade update fktable, in RI_FKey_cascade_upd
we can use pktable's collation. but again, a query updating fktable
only, using pktable collation seems strange.

---case3. ---not ok case PK indeterministic, FK deterministic
DROP TABLE IF EXISTS fktable, pktable;
CREATE TABLE pktable (x text COLLATE ignore_accent_case PRIMARY KEY);
CREATE TABLE fktable (x text collate "C" REFERENCES pktable on update
cascade on delete cascade);
INSERT INTO pktable VALUES ('A');
INSERT INTO fktable VALUES ('A');
INSERT INTO fktable VALUES ('a');
begin; update pktable set x = 'Å'; table fktable; rollback;

when we "INSERT INTO fktable VALUES ('a');"
we can disallow this invalid query in RI_FKey_check by constructing
the following stop query
SELECT 1 FROM ONLY public.pktable x WHERE x OPERATOR(pg_catalog.=) 'a'
collate "C" FOR KEY SHARE OF x;
but this query associated PK table with fktable collation seems weird?

summary:
case1 seems hard to resolve
case2 can be resolved in ri_GenerateQualCollation, not 100% sure.
case3 can be resolved in RI_FKey_check
because case1 is hard to resolve;
so overall I feel like erroring out PK indeterministic and FK
indeterministic while creating foreign keys is easier.

We can mandate foreign keys and primary key columns be deterministic
in ATAddForeignKeyConstraint.
The attached patch does that.

That means src/test/regress/sql/collate.icu.utf8.sql table test10pk,
table test11pk will have a big change.
so only attach src/backend/commands/tablecmds.c changes.

On 07.06.24 08:39, jian he wrote:

On Sat, Apr 13, 2024 at 9:13 PM jian he <jian.universality@gmail.com> wrote:

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

I think I found a simple way.

the logic is:
* ATExecAlterColumnType changes one column once at a time.
* one column can only have one collation. so we don't need to store a
list of collation oid.
* ATExecAlterColumnType we can get the new collation (targetcollid)
and original collation info.
* RememberAllDependentForRebuilding will check the column dependent,
whether this column is referenced by a foreign key or not information
is recorded.
so AlteredTableInfo->changedConstraintOids have the primary key and
foreign key oids.
* ATRewriteCatalogs will call ATPostAlterTypeCleanup (see the comments
in ATRewriteCatalogs)
* for tab->changedConstraintOids (foreign key, primary key) will call
ATPostAlterTypeParse, so
for foreign key (con->contype == CONSTR_FOREIGN) will call TryReuseForeignKey.
* in TryReuseForeignKey, we can pass the information that our primary
key old collation is nondeterministic
and old collation != new collation to the foreign key constraint.
so foreign key can act accordingly at ATAddForeignKeyConstraint (old_check_ok).

based on the above logic, I add one bool in struct AlteredTableInfo,
one bool in struct Constraint.
bool in AlteredTableInfo is for storing it, later passing it to struct
Constraint.
we need bool in Constraint because ATAddForeignKeyConstraint needs it.

I refactored the comments.
also added some extra tests hoping to make it more bullet proof, maybe
it's redundant.

I like this patch version (v4). It's the simplest, probably also
easiest to backpatch.

It has a flaw: It will also trigger a FK recheck if you alter the
collation of the referencing column (foreign key column), even though
that is not necessary. (Note that your tests and the examples in this
thread only discuss altering the PK column collation, because that is
what is actually used during the foreign key checks.) Maybe there is an
easy way to avoid that, but I couldn't see one in that patch structure.

Maybe that is ok as a compromise. If, in the future, we make it a
requirement that the collations on the PK and FK side have to be the
same if either collation is nondeterministic, then this case can no
longer happen anyway. And so building more infrastructure for this
check might be wasted.

On Tue, Sep 3, 2024 at 5:41 PM Peter Eisentraut <peter@eisentraut.org> wrote:

I like this patch version (v4). It's the simplest, probably also
easiest to backpatch.

I am actually confused.
In this email thread [1]/messages/by-id/CACJufxEW6OMBqt8cbr=3Jt++Zd_SL-4YDjfk7Q7DhGKiSLcu4g@mail.gmail.com, I listed 3 corn cases.
I thought all these 3 corner cases should not be allowed.
but V4 didn't solve these corner case issues.
what do you think of their corner case, should it be allowed?

Anyway, I thought these corner cases should not be allowed to happen,
so I made sure PK, FK ties related collation were deterministic.
PK can have indeterministic collation as long as it does not interact with FK.

[1]: /messages/by-id/CACJufxEW6OMBqt8cbr=3Jt++Zd_SL-4YDjfk7Q7DhGKiSLcu4g@mail.gmail.com

On 04.09.24 08:54, jian he wrote:

On Tue, Sep 3, 2024 at 5:41 PM Peter Eisentraut <peter@eisentraut.org> wrote:

I like this patch version (v4). It's the simplest, probably also
easiest to backpatch.

I am actually confused.
In this email thread [1], I listed 3 corn cases.
I thought all these 3 corner cases should not be allowed.
but V4 didn't solve these corner case issues.
what do you think of their corner case, should it be allowed?

Anyway, I thought these corner cases should not be allowed to happen,
so I made sure PK, FK ties related collation were deterministic.
PK can have indeterministic collation as long as it does not interact with FK.

I had thought we could at first do a limited patch that just prevents
the problematic collation changes that Paul pointed out initially, and
then backpatch that, and then develop a more complete solution for
master. But after thinking about this a bit more, such a limited patch
might just be some partial whack-a-mole that would still leave open
problems (as you have pointed out), and it also looks like such a patch
wouldn't be any simpler than the complete solution.

So I took the v5 patch you had posted and started working from there.
The rule that you had picked isn't quite what we want, I think. It's
okay to have nondeterministic collations on foreign keys, as long as the
collation is the same on both sides. That's what I have implemented.
See attached.

This approach also allows cleaning up a bunch of hackiness in
ri_triggers.c, which feels satisfying.

I don't know what to do about backpatching though. The patch itself
appears to backpatch ok. (There are some cosmetic issues that need
manual intervention, but the code structure is pretty consistent going
back.) But that kind of behavior change, I don't know. Also, for the
most part, the existing setup works, it's only if you do some particular
table alterations that you can construct problems.

We could make the error a warning instead, so people know that what they
are building is problematic and deprecated.

But in either case, or even with some of the other approaches discussed
in previous patch versions, such as v4, you'd only get that warning or
error if you do table DDL. If you'd just upgrade the minor release, you
don't get any info. So we'd also have to construct some query to check
for this and create some release note guidance.

So for the moment this is a master-only patch. I think once we have
tied down the behavior we want for the future, we can then see how we
can nudge older versions in that direction.

On Thu, Oct 17, 2024 at 7:14 PM Peter Eisentraut <peter@eisentraut.org> wrote:

So I took the v5 patch you had posted and started working from there.
The rule that you had picked isn't quite what we want, I think. It's
okay to have nondeterministic collations on foreign keys, as long as the
collation is the same on both sides. That's what I have implemented.
See attached.

This approach also allows cleaning up a bunch of hackiness in
ri_triggers.c, which feels satisfying.

yech, i missed FK, PK both are nondeterministic but with same collation OID.
your work is more neat.
However FK, PK same nondeterministic collation OID have implications
for ri_KeysEqual.

ri_KeysEqual definitely deserves some comments.
for rel_is_pk, the equality is collation agnostic;
for rel_is_pk is false, the equality is collation aware.

for example:
DROP TABLE IF EXISTS fktable, pktable;
CREATE TABLE pktable (x text COLLATE case_insensitive PRIMARY KEY);
CREATE TABLE fktable (x text collate case_insensitive REFERENCES
pktable on update restrict on delete restrict);
INSERT INTO pktable VALUES ('A'), ('Å');
INSERT INTO fktable VALUES ('a');
update pktable set x = 'a' where x = 'A';
ERROR: update or delete on table "pktable" violates foreign key
constraint "fktable_x_fkey" on table "fktable"
DETAIL: Key (x)=(A) is still referenced from table "fktable".
this should not happen?
If so, the below change can solve the problem.

@@ -2930,6 +2915,16 @@ ri_KeysEqual(Relation rel, TupleTableSlot
*oldslot, TupleTableSlot *newslot,
              */
             Form_pg_attribute att =
TupleDescAttr(oldslot->tts_tupleDescriptor, attnums[i] - 1);
+            Oid        collation =  RIAttCollation(rel, attnums[i]);
+            if (OidIsValid(collation) &&
!get_collation_isdeterministic(collation))
+            {
+                Oid            eq_opr;
+                bool        result;
+                eq_opr = riinfo->pp_eq_oprs[i];
+                result = ri_CompareWithCast(eq_opr, RIAttType(rel, attnums[i]),
+                                        collation, newvalue, oldvalue);
+                return result;
+            }
             if (!datum_image_eq(oldvalue, newvalue, att->attbyval,
att->attlen))
                 return false;

The above change will make the ri_KeysEqual equality coalition aware
regardless rel_is_pk's value.
to see the effect, we can test it BEFORE and AFTER applying the above
ri_KeysEqual changes
with the attached sql script.

So for the moment this is a master-only patch. I think once we have
tied down the behavior we want for the future

 /*
  * ri_GenerateQualCollation --- add a COLLATE spec to a WHERE clause
  *
- * At present, we intentionally do not use this function for RI queries that
- * compare a variable to a $n parameter.  Since parameter symbols always have
- * default collation, the effect will be to use the variable's collation.
- * Now that is only strictly correct when testing the referenced column, since
- * the SQL standard specifies that RI comparisons should use the referenced
- * column's collation.  However, so long as all collations have the same
- * notion of equality (which they do, because texteq reduces to bitwise
- * equality), there's no visible semantic impact from using the referencing
- * column's collation when testing it, and this is a good thing to do because
- * it lets us use a normal index on the referencing column.  However, we do
- * have to use this function when directly comparing the referencing and
- * referenced columns, if they are of different collations; else the parser
- * will fail to resolve the collation to use.
+ * We only have to use this function when directly comparing the referencing
+ * and referenced columns, if they are of different collations; else the
+ * parser will fail to resolve the collation to use.  We don't need to use
+ * this function for RI queries that compare a variable to a $n parameter.
+ * Since parameter symbols always have default collation, the effect will be
+ * to use the variable's collation.
+ *
+ * Note that we require that the collations of the referencing and the
+ * referenced column have the some notion of equality: Either they have to
+ * both be deterministic or else they both have to be the same.
  */
" some notion of equality" should it be "same notion of equality"?
maybe we can add "see ATAddForeignKeyConstraint also"
at the end of the whole comment.

/*
* transformColumnNameList - transform list of column names
*
* Lookup each name and return its attnum and, optionally, type OID
*
* Note: the name of this function suggests that it's general-purpose,
* but actually it's only used to look up names appearing in foreign-key
* clauses. The error messages would need work to use it in other cases,
* and perhaps the validity checks as well.
*/
static int
transformColumnNameList(Oid relId, List *colList,
int16 *attnums, Oid *atttypids, Oid *attcollids)
comments need slight adjustment?
comments in transformFkeyGetPrimaryKey also need slight change?
ri_CompareWithCast, we can aslo add comments saying the output is
collation aware.

+ /*
+ * This shouldn't be possible, but better check to make sure we have a
+ * consistent state for the check below.
+ */
+ if ((pkcoll && !fkcoll) || (!pkcoll && fkcoll))
+ elog(ERROR, "key columns are not both collatable");
+
+ if (pkcoll && fkcoll)

cosmetic. pkcoll can change to OidIsValid(pkcoll)
fkcoll change to OidIsValid(fkcoll).

ereport(ERROR,
(errcode(ERRCODE_COLLATION_MISMATCH),
errmsg("foreign key constraint \"%s\" cannot be implemented",
fkconstraint->conname),
errdetail("Key columns \"%s\" and \"%s\" have incompatible
collations: \"%s\" and \"%s\"."
"If either collation is nondeterministic, then both collations
have to be the same.",
strVal(list_nth(fkconstraint->fk_attrs, i)),
strVal(list_nth(fkconstraint->pk_attrs, i)),
get_collation_name(fkcoll),
get_collation_name(pkcoll))));

ERROR: foreign key constraint "a_fk_col1_fkey" cannot be implemented
DETAIL: Key columns "col1" and "col1" have incompatible collations:
"default" and "case_insensitive". If either collation is
nondeterministic, then both collations have to be the same.

"DETAIL: Key columns "col1" and "col1"" may be confusing.
we can be more verbose. like
DETAIL: Key columns "col1" in relation "X" and "col1" in relation "Y"
have incompatible collations......
(just a idea, don't have much preference)

old_check_ok = (new_pathtype == old_pathtype &&
new_castfunc == old_castfunc &&
(!IsPolymorphicType(pfeqop_right) ||
new_fktype == old_fktype) &&
(new_fkcoll == old_fkcoll ||
(get_collation_isdeterministic(old_fkcoll) &&
get_collation_isdeterministic(new_fkcoll))));

I am wondering if one of the new_fkcoll, old_fkcoll is zero, the other is not.
turns out that would n't happen.

before "if (old_check_ok)" we already did extensionsive check that
new_fkcoll is ok
for the job.
in ATAddForeignKeyConstraint, we can leave the old_check_ok part as is.
what do you think of the following:

old_check_ok = (new_pathtype == old_pathtype &&
new_castfunc == old_castfunc &&
(!IsPolymorphicType(pfeqop_right) ||
new_fktype == old_fktype));
if (OidIsValid(new_fkcoll) && OidIsValid(old_fkcoll) &&
new_fkcoll != old_fkcoll)
{
if(!get_collation_isdeterministic(new_fkcoll))
elog(ERROR,"new_fkcoll should be deterministic");
}

On Fri, Oct 25, 2024 at 2:27 PM jian he <jian.universality@gmail.com> wrote:

/*
* ri_GenerateQualCollation --- add a COLLATE spec to a WHERE clause
*
- * At present, we intentionally do not use this function for RI queries that
- * compare a variable to a $n parameter.  Since parameter symbols always have
- * default collation, the effect will be to use the variable's collation.
- * Now that is only strictly correct when testing the referenced column, since
- * the SQL standard specifies that RI comparisons should use the referenced
- * column's collation.  However, so long as all collations have the same
- * notion of equality (which they do, because texteq reduces to bitwise
- * equality), there's no visible semantic impact from using the referencing
- * column's collation when testing it, and this is a good thing to do because
- * it lets us use a normal index on the referencing column.  However, we do
- * have to use this function when directly comparing the referencing and
- * referenced columns, if they are of different collations; else the parser
- * will fail to resolve the collation to use.
+ * We only have to use this function when directly comparing the referencing
+ * and referenced columns, if they are of different collations; else the
+ * parser will fail to resolve the collation to use.  We don't need to use
+ * this function for RI queries that compare a variable to a $n parameter.
+ * Since parameter symbols always have default collation, the effect will be
+ * to use the variable's collation.
+ *
+ * Note that we require that the collations of the referencing and the
+ * referenced column have the some notion of equality: Either they have to
+ * both be deterministic or else they both have to be the same.
*/

drop table if exists pktable, fktable;
CREATE TABLE pktable (x text COLLATE "POSIX" PRIMARY KEY);
CREATE TABLE fktable (x text COLLATE "C" REFERENCES pktable on update
cascade on delete cascade);
INSERT INTO pktable VALUES ('A'), ('Å');
INSERT INTO fktable VALUES ('A');

update pktable set x = 'a' collate "C" where x = 'A' collate "POSIX";

the cascade update fktable query string is:
UPDATE ONLY "public"."fktable" SET "x" = $1 WHERE $2 OPERATOR(pg_catalog.=) "x"
ideally it should be
UPDATE ONLY "public"."fktable" SET "x" = $1 collate "C" WHERE $2
collate "POSIX" OPERATOR(pg_catalog.=) "x"

as we already mentioned in several places: PK-FK tie either they have to
both be deterministic or else they both have to be the same collation
oid.
so the reduction to
UPDATE ONLY "public"."fktable" SET "x" = $1 WHERE $2 OPERATOR(pg_catalog.=) "x"
is safe.
but you look at SPI_execute_snapshot, _SPI_convert_params. then we can see
the collation metadata is not keeped.

+ We don't need to use
+ * this function for RI queries that compare a variable to a $n parameter.
+ * Since parameter symbols always have default collation, the effect will be
+ * to use the variable's collation.

so I think a better description is

+ We don't need to use
+ * this function for RI queries that compare a variable to a $n parameter.
+ * Since parameter symbols don't have collation information, the effect will be
+ * to use the variable's collation.

you can see related discovery in
/messages/by-id/CACJufxEtPBWAk7nEn69ww2LKi9w1i4dLwd5gnjD1DQ2vaYoi2g@mail.gmail.com

Here is a new patch that addresses all of your review comments. I also
added an example of a problem in the commit message.

On 25.10.24 08:27, jian he wrote:

+ * Note that we require that the collations of the referencing and the
+ * referenced column have the some notion of equality: Either they have to
+ * both be deterministic or else they both have to be the same.
*/
" some notion of equality" should it be "same notion of equality"?
maybe we can add "see ATAddForeignKeyConstraint also"
at the end of the whole comment.

both fixed

/*
* transformColumnNameList - transform list of column names
*
* Lookup each name and return its attnum and, optionally, type OID
*
* Note: the name of this function suggests that it's general-purpose,
* but actually it's only used to look up names appearing in foreign-key
* clauses. The error messages would need work to use it in other cases,
* and perhaps the validity checks as well.
*/
static int
transformColumnNameList(Oid relId, List *colList,
int16 *attnums, Oid *atttypids, Oid *attcollids)
comments need slight adjustment?
comments in transformFkeyGetPrimaryKey also need slight change?
ri_CompareWithCast, we can aslo add comments saying the output is
collation aware.

all fixed

+ /*
+ * This shouldn't be possible, but better check to make sure we have a
+ * consistent state for the check below.
+ */
+ if ((pkcoll && !fkcoll) || (!pkcoll && fkcoll))
+ elog(ERROR, "key columns are not both collatable");
+
+ if (pkcoll && fkcoll)

cosmetic. pkcoll can change to OidIsValid(pkcoll)
fkcoll change to OidIsValid(fkcoll).

done

ERROR: foreign key constraint "a_fk_col1_fkey" cannot be implemented
DETAIL: Key columns "col1" and "col1" have incompatible collations:
"default" and "case_insensitive". If either collation is
nondeterministic, then both collations have to be the same.

"DETAIL: Key columns "col1" and "col1"" may be confusing.
we can be more verbose. like
DETAIL: Key columns "col1" in relation "X" and "col1" in relation "Y"
have incompatible collations......
(just a idea, don't have much preference)

Done. I also changed the error message about incompatible types in the
same way, in a separate commit.

old_check_ok = (new_pathtype == old_pathtype &&
new_castfunc == old_castfunc &&
(!IsPolymorphicType(pfeqop_right) ||
new_fktype == old_fktype) &&
(new_fkcoll == old_fkcoll ||
(get_collation_isdeterministic(old_fkcoll) &&
get_collation_isdeterministic(new_fkcoll))));

I am wondering if one of the new_fkcoll, old_fkcoll is zero, the other is not.
turns out that would n't happen.

before "if (old_check_ok)" we already did extensionsive check that
new_fkcoll is ok
for the job.
in ATAddForeignKeyConstraint, we can leave the old_check_ok part as is.
what do you think of the following:

old_check_ok = (new_pathtype == old_pathtype &&
new_castfunc == old_castfunc &&
(!IsPolymorphicType(pfeqop_right) ||
new_fktype == old_fktype));
if (OidIsValid(new_fkcoll) && OidIsValid(old_fkcoll) &&
new_fkcoll != old_fkcoll)
{
if(!get_collation_isdeterministic(new_fkcoll))
elog(ERROR,"new_fkcoll should be deterministic");
}

I don't think this is right. The "old_check_ok" business is only used
when changing an existing foreign key, to see if the check needs to be
run again. The earlier code I add already ensures that if the
collations are nondeterministic, then they have to be the same between
PK and FK. So if this is the case, the only way to change the foreign
key collation is if you have a foreign key that references the same
table and you change the primary key column types in the same ALTER
TABLE statement. Then this conditional ensures that the recheck is run
under appropriate circumstances. But we don't want to error here; the
new situation is valid, we just need to determine whether we have to run
the check again.

On 25.10.24 16:26, jian he wrote:

drop table if exists pktable, fktable;
CREATE TABLE pktable (x text COLLATE "POSIX" PRIMARY KEY);
CREATE TABLE fktable (x text COLLATE "C" REFERENCES pktable on update
cascade on delete cascade);
INSERT INTO pktable VALUES ('A'), ('Å');
INSERT INTO fktable VALUES ('A');

update pktable set x = 'a' collate "C" where x = 'A' collate "POSIX";

the cascade update fktable query string is:
UPDATE ONLY "public"."fktable" SET "x" = $1 WHERE $2 OPERATOR(pg_catalog.=) "x"
ideally it should be
UPDATE ONLY "public"."fktable" SET "x" = $1 collate "C" WHERE $2
collate "POSIX" OPERATOR(pg_catalog.=) "x"

as we already mentioned in several places: PK-FK tie either they have to
both be deterministic or else they both have to be the same collation
oid.
so the reduction to
UPDATE ONLY "public"."fktable" SET "x" = $1 WHERE $2 OPERATOR(pg_catalog.=) "x"
is safe.
but you look at SPI_execute_snapshot, _SPI_convert_params. then we can see
the collation metadata is not keeped.

+ We don't need to use
+ * this function for RI queries that compare a variable to a $n parameter.
+ * Since parameter symbols always have default collation, the effect will be
+ * to use the variable's collation.

so I think a better description is

+ We don't need to use
+ * this function for RI queries that compare a variable to a $n parameter.
+ * Since parameter symbols don't have collation information, the effect will be
+ * to use the variable's collation.

you can see related discovery in
/messages/by-id/CACJufxEtPBWAk7nEn69ww2LKi9w1i4dLwd5gnjD1DQ2vaYoi2g@mail.gmail.com

I don't know. I don't think there is anything wrong with the existing
code in this respect. Notably a parameter gets assigned its type's
default collation (see src/backend/parser/parse_param.c), so the change
of the comment as you suggest it is not correct.

Also, I don't think this is actually related to the patch discussed in
this thread.

On 25.10.24 08:23, jian he wrote:

ri_KeysEqual definitely deserves some comments.
for rel_is_pk, the equality is collation agnostic;
for rel_is_pk is false, the equality is collation aware.

for example:
DROP TABLE IF EXISTS fktable, pktable;
CREATE TABLE pktable (x text COLLATE case_insensitive PRIMARY KEY);
CREATE TABLE fktable (x text collate case_insensitive REFERENCES
pktable on update restrict on delete restrict);
INSERT INTO pktable VALUES ('A'), ('Å');
INSERT INTO fktable VALUES ('a');
update pktable set x = 'a' where x = 'A';
ERROR: update or delete on table "pktable" violates foreign key
constraint "fktable_x_fkey" on table "fktable"
DETAIL: Key (x)=(A) is still referenced from table "fktable".
this should not happen?

Apparently this is intentional. It's the difference between RESTRICT
and NO ACTION. In ri_restrict(), there is a comment:

/*
* If another PK row now exists providing the old key values, we should
* not do anything. However, this check should only be made in the NO
* ACTION case; in RESTRICT cases we don't wish to allow another
row to be
* substituted.
*/

In any case, this patch does not change this behavior. It exists in old
versions as well.

On Thu, Nov 7, 2024 at 8:15 PM Peter Eisentraut <peter@eisentraut.org> wrote:

Apparently this is intentional. It's the difference between RESTRICT
and NO ACTION. In ri_restrict(), there is a comment:

/*
* If another PK row now exists providing the old key values, we should
* not do anything. However, this check should only be made in the NO
* ACTION case; in RESTRICT cases we don't wish to allow another
row to be
* substituted.
*/

In any case, this patch does not change this behavior. It exists in old
versions as well.

https://stackoverflow.com/questions/14921668/difference-between-restrict-and-no-action
mentioned about the difference between "no action" and "restrict".

RI_FKey_restrict_upd comments also says:

* The SQL standard intends that this referential action occur exactly when
* the update is performed, rather than after. This appears to be
* the only difference between "NO ACTION" and "RESTRICT". In Postgres
* we still implement this as an AFTER trigger, but it's non-deferrable.

DROP TABLE IF EXISTS fktable, pktable;
CREATE TABLE pktable (x text COLLATE case_insensitive PRIMARY KEY);
CREATE TABLE fktable (x text collate case_insensitive REFERENCES
pktable on update restrict on delete restrict);
INSERT INTO pktable VALUES ('A'), ('Å');
INSERT INTO fktable VALUES ('a');
update pktable set x = 'a' where x = 'A';
----------------------------------------------------------
DROP TABLE IF EXISTS fktable, pktable;
CREATE TABLE pktable (x text COLLATE case_insensitive PRIMARY KEY);
CREATE TABLE fktable (x text collate case_insensitive REFERENCES
pktable on update no action on delete no action);
INSERT INTO pktable VALUES ('A'), ('Å');
INSERT INTO fktable VALUES ('a');
update pktable set x = 'a' where x = 'A';

In the above two cases, the last queries behavior difference does not
look like "no action" vs "restrict" mentioned in the doc (create_table.sgml),
or the above stackoverflow link.
so this part, i am still confused.

-----------------------------<<>>>-----------------------------
CREATE TABLE pktable (x text COLLATE case_insensitive PRIMARY KEY);
INSERT INTO pktable VALUES ('A'), ('Å'), ('H');
INSERT INTO fktable VALUES ('a');

fktable foreign key variants:
collate case_insensitive REFERENCES pktable on update set default on
delete set default
collate case_insensitive REFERENCES pktable on update set null on
delete set null
collate case_insensitive REFERENCES pktable on update cascade on delete cascade

`update pktable set x = 'a' where x = 'A';`
will act as if the column "x" value has changed.
so it will do the action to fktable.

following the same logic,
maybe we should let "on update no action on delete no action"
fail for the following case:

DROP TABLE IF EXISTS fktable, pktable;
CREATE TABLE pktable (x text COLLATE case_insensitive PRIMARY KEY);
CREATE TABLE fktable (x text collate case_insensitive REFERENCES
pktable on update no action on delete no action);
INSERT INTO pktable VALUES ('A'), ('Å');
INSERT INTO fktable VALUES ('a');

---expect it to fail. since column "x " value changed and is still
being referenced.
update pktable set x = 'a' where x = 'A';
-----------------------------<<>>>-----------------------------

I added a "on update cascade, on delete cascade" tests on collate.icu.utf8.sql
for both foreign key and primary key are nondeterministic collation.