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(at)eisentraut(dot)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.