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

On 07.06.24 08:39, jian he wrote:
> On Sat, Apr 13, 2024 at 9:13 PM jian he <jian(dot)universality(at)gmail(dot)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.