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

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.