MIME-Version: 1.0
References: <78d824e0-b21e-480d-a252-e4b84bc2c24b@illuminatedcomputing.com>
 <69f0a887-a4ed-49cf-9d63-923891006658@illuminatedcomputing.com> <CACJufxE8BePAtzoedMo5Fyznif_6vWy0-aq7WjaDd09nd6k4YQ@mail.gmail.com>
In-Reply-To: <CACJufxE8BePAtzoedMo5Fyznif_6vWy0-aq7WjaDd09nd6k4YQ@mail.gmail.com>
From: jian he <jian.universality@gmail.com>
Date: Fri, 12 Apr 2024 17:06:04 +0800
Message-ID: <CACJufxEE_dma4zLh2ydfAfRQPWmJ=M6iV2J-T20BpXm9V0Dzvw@mail.gmail.com>
Subject: Re: altering a column's collation leaves an invalid foreign key
To: Paul Jungwirth <pj@illuminatedcomputing.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CACJufxEE_dma4zLh2ydfAfRQPWmJ%3DM6iV2J-T20BpXm9V0Dzvw%40mail.gmail.com>
Precedence: bulk

On Tue, Mar 26, 2024 at 1:00=E2=80=AFPM jian he <jian.universality@gmail.co=
m> wrote:
>
> On Mon, Mar 25, 2024 at 2:47=E2=80=AFPM Paul Jungwirth
> <pj@illuminatedcomputing.com> wrote:
> >
> > On 3/23/24 10:04, Paul Jungwirth wrote:
> > > Perhaps if the previous collation was nondeterministic we should forc=
e a re-check.
> >
> > Here is a patch implementing this. It was a bit more fuss than I expect=
ed, so maybe someone has a
> > better way.
> >
>
> + /* test follows the one in ri_FetchConstraintInfo() */
> + if (ARR_NDIM(arr) !=3D 1 ||
> + ARR_HASNULL(arr) ||
> + ARR_ELEMTYPE(arr) !=3D INT2OID)
> + elog(ERROR, "conkey is not a 1-D smallint array");
> + attarr =3D (AttrNumber *) ARR_DATA_PTR(arr);
> +
> + /* stash a List of the collation Oids in our Constraint node */
> + for (i =3D 0; i < numkeys; i++)
> + con->old_collations =3D lappend_oid(con->old_collations,
> +  list_nth_oid(changedCollationOids, attarr[i] - 1));
>
> I don't understand the "ri_FetchConstraintInfo" comment.

I still don't understand this comment.

>
> +static void
> +RememberCollationForRebuilding(AttrNumber attnum, AlteredTableInfo *tab)
> +{
> + Oid typid;
> + int32 typmod;
> + Oid collid;
> + ListCell   *lc;
> +
> + /* Fill in the list with InvalidOid if this is our first visit */
> + if (tab->changedCollationOids =3D=3D NIL)
> + {
> + int len =3D RelationGetNumberOfAttributes(tab->rel);
> + int i;
> +
> + for (i =3D 0; i < len; i++)
> + tab->changedCollationOids =3D lappend_oid(tab->changedCollationOids,
> + InvalidOid);
> + }
> +
> + get_atttypetypmodcoll(RelationGetRelid(tab->rel), attnum,
> +  &typid, &typmod, &collid);
> +
> + lc =3D list_nth_cell(tab->changedCollationOids, attnum - 1);
> + lfirst_oid(lc) =3D collid;
> +}
>
> do we need to check if `collid` is a valid collation?
> like:
>
> if (!OidIsValid(collid))
> {
> lc =3D list_nth_cell(tab->changedCollationOids, attnum - 1);
> lfirst_oid(lc) =3D collid;
> }
now I think RememberCollationForRebuilding is fine. no need further change.


in ATAddForeignKeyConstraint.
if (old_check_ok)
{
/*
* When a pfeqop changes, revalidate the constraint.  We could
* permit intra-opfamily changes, but that adds subtle complexity
* without any concrete benefit for core types.  We need not
* assess ppeqop or ffeqop, which RI_Initial_Check() does not use.
*/
old_check_ok =3D (pfeqop =3D=3D lfirst_oid(old_pfeqop_item));
old_pfeqop_item =3D lnext(fkconstraint->old_conpfeqop,
old_pfeqop_item);
}
maybe we can do the logic right below it. like:

if (old_check_ok)
{

* All deterministic collations use bitwise equality to resolve
* ties, but if the previous collation was nondeterministic,
* we must re-check the foreign key, because some references
* that use to be "equal" may not be anymore. If we have
* InvalidOid here, either there was no collation or the
* attribute didn't change.
old_check_ok =3D (old_collation =3D=3D InvalidOid ||
get_collation_isdeterministic(old_collation));
}
then we don't need to cram it with the other `if (old_check_ok){}`.


do we need to add an entry in
https://wiki.postgresql.org/wiki/PostgreSQL_17_Open_Items#Open_Issues
?