Feedback-ID: ie0a040ee:Fastmail
Message-ID: <a3837e08-70a7-4c4c-a581-eb4570dbdcf1@eisentraut.org>
Date: Tue, 3 Sep 2024 11:41:20 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: altering a column's collation leaves an invalid foreign key
To: jian he <jian.universality@gmail.com>,
 Paul Jungwirth <pj@illuminatedcomputing.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <78d824e0-b21e-480d-a252-e4b84bc2c24b@illuminatedcomputing.com>
 <69f0a887-a4ed-49cf-9d63-923891006658@illuminatedcomputing.com>
 <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>
Content-Language: en-US
From: Peter Eisentraut <peter@eisentraut.org>
In-Reply-To: <CACJufxE3U650j=acBuksB+D9GoGVwU0Q2q+7b-BiF6muyp8_qA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/a3837e08-70a7-4c4c-a581-eb4570dbdcf1%40eisentraut.org>
Precedence: bulk

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.