public inbox for [email protected]  

help / color / mirror / Atom feed

From: Peter Eisentraut <[email protected]>
To: jian he <[email protected]>
To: Paul Jungwirth <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: altering a column's collation leaves an invalid foreign key
Date: Tue, 3 Sep 2024 11:41:20 +0200
Message-ID: <[email protected]> (raw)
In-Reply-To: <CACJufxE3U650j=acBuksB+D9GoGVwU0Q2q+7b-BiF6muyp8_qA@mail.gmail.com>
References: <[email protected]>
	<[email protected]>
	<CACJufxE8BePAtzoedMo5Fyznif_6vWy0-aq7WjaDd09nd6k4YQ@mail.gmail.com>
	<CACJufxEE_dma4zLh2ydfAfRQPWmJ=M6iV2J-T20BpXm9V0Dzvw@mail.gmail.com>
	<CACJufxF5W9XUdFcYayyfuKBcxw830q=B0xuA-SYBo7XyMULZrg@mail.gmail.com>
	<CACJufxE3U650j=acBuksB+D9GoGVwU0Q2q+7b-BiF6muyp8_qA@mail.gmail.com>

On 07.06.24 08:39, jian he wrote:
> On Sat, Apr 13, 2024 at 9:13 PM jian he <[email protected]> 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.

view thread (9+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: altering a column's collation leaves an invalid foreign key
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox