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>
 <CACJufxEE_dma4zLh2ydfAfRQPWmJ=M6iV2J-T20BpXm9V0Dzvw@mail.gmail.com>
 <CACJufxF5W9XUdFcYayyfuKBcxw830q=B0xuA-SYBo7XyMULZrg@mail.gmail.com>
 <CACJufxE3U650j=acBuksB+D9GoGVwU0Q2q+7b-BiF6muyp8_qA@mail.gmail.com>
 <216627.1717791128@sss.pgh.pa.us> <CACJufxHNvXvC+2SgqNN2Y8QEfkD3VjR_47KiUGKyKcJcqeeAnw@mail.gmail.com>
 <a876bf93-61c6-4715-9faa-cda85b78c1f9@eisentraut.org>
In-Reply-To: <a876bf93-61c6-4715-9faa-cda85b78c1f9@eisentraut.org>
From: jian he <jian.universality@gmail.com>
Date: Tue, 16 Jul 2024 16:29:00 +0800
Message-ID: <CACJufxEW6OMBqt8cbr=3Jt++Zd_SL-4YDjfk7Q7DhGKiSLcu4g@mail.gmail.com>
Subject: Re: altering a column's collation leaves an invalid foreign key
To: Peter Eisentraut <peter@eisentraut.org>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Paul Jungwirth <pj@illuminatedcomputing.com>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/mixed; boundary="0000000000001b5998061d59257c"
Archived-At: <https://www.postgresql.org/message-id/CACJufxEW6OMBqt8cbr%3D3Jt%2B%2BZd_SL-4YDjfk7Q7DhGKiSLcu4g%40mail.gmail.com>
Precedence: bulk

--0000000000001b5998061d59257c
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Jun 18, 2024 at 4:50=E2=80=AFPM Peter Eisentraut <peter@eisentraut.=
org> wrote:
>
> On 08.06.24 06:14, jian he wrote:
> > if FK is nondeterministic, then it looks PK more like FK.
> > the following example, one FK row is referenced by two PK rows.
> >
> > DROP TABLE IF EXISTS fktable, pktable;
> > CREATE TABLE pktable (x text COLLATE "C" PRIMARY KEY);
> > CREATE TABLE fktable (x text COLLATE ignore_accent_case REFERENCES
> > pktable on update cascade on delete cascade);
> > INSERT INTO pktable VALUES ('A'), ('=C3=85');
> > INSERT INTO fktable VALUES ('A');
>
> Yes, this is a problem.  The RI checks are done with the collation of
> the primary key.
>
> The comment at ri_GenerateQualCollation() says "the SQL standard
> specifies that RI comparisons should use the referenced column's
> collation".  But this is not what it says in my current copy.
>
> ... [ digs around ISO archives ] ...
>
> Yes, this was changed in SQL:2016 to require the collation on the PK
> side and the FK side to match at constraint creation time.  The argument
> made is exactly the same we have here.  This was actually already the
> rule in SQL:1999 but was then relaxed in SQL:2003 and then changed back
> because it was a mistake.
>
> We probably don't need to enforce this for deterministic collations,
> which would preserve some backward compatibility.
>
> I'll think some more about what steps to take to solve this and what to
> do about back branches etc.
>

I have come up with 3 corner cases.

---case1. not ok. PK indeterministic, FK default
DROP TABLE IF EXISTS fktable, pktable;
CREATE TABLE pktable (x text COLLATE ignore_accent_case PRIMARY KEY);
CREATE TABLE fktable (x text REFERENCES pktable on update cascade on
delete cascade);
INSERT INTO pktable VALUES ('A');
INSERT INTO fktable VALUES ('a');
INSERT INTO fktable VALUES ('A');

RI_FKey_check (Check foreign key existence ) querybuf.data is
SELECT 1 FROM ONLY "public"."pktable" x WHERE "x"
OPERATOR(pg_catalog.=3D) $1 FOR KEY SHARE OF x
in here, fktable doesn't have collation.
we cannot rewrite to
SELECT 1 FROM ONLY "public"."pktable" x WHERE "x"
OPERATOR(pg_catalog.=3D) $1 collate "C" FOR KEY SHARE OF x
so assumption (only one referenced row for any referencing row) will
break when inserting values to fktable.
RI_FKey_check already allows invalidate values to happen, not sure how
ri_GenerateQualCollation can help.
overall i don't know how to stop invalid value  while inserting value
to fktable in this case.


---case2. not ok case PK deterministic, FK indeterministic
DROP TABLE IF EXISTS fktable, pktable;
CREATE TABLE pktable (x text COLLATE "C" PRIMARY KEY);
CREATE TABLE fktable (x text COLLATE ignore_accent_case REFERENCES
pktable on update cascade on delete cascade);
INSERT INTO pktable VALUES ('A'), ('=C3=85');
INSERT INTO fktable VALUES ('A');
begin; update pktable set x  =3D 'B' where x =3D '=C3=85'; table fktable; r=
ollback;
begin; update pktable set x  =3D 'B' where x =3D 'A'; table fktable; rollba=
ck;

when cascade update fktable, in RI_FKey_cascade_upd
we can use pktable's collation. but again, a query updating fktable
only, using pktable collation seems strange.


---case3. ---not ok case PK indeterministic, FK deterministic
DROP TABLE IF EXISTS fktable, pktable;
CREATE TABLE pktable (x text COLLATE ignore_accent_case PRIMARY KEY);
CREATE TABLE fktable (x text collate "C" REFERENCES pktable on update
cascade on delete cascade);
INSERT INTO pktable VALUES ('A');
INSERT INTO fktable VALUES ('A');
INSERT INTO fktable VALUES ('a');
begin; update pktable set x  =3D '=C3=85'; table fktable; rollback;

when we "INSERT INTO fktable VALUES ('a');"
we can disallow this invalid query in RI_FKey_check by constructing
the following stop query
SELECT 1 FROM ONLY public.pktable x WHERE x OPERATOR(pg_catalog.=3D) 'a'
collate "C" FOR KEY SHARE OF x;
but this query associated PK table with fktable collation seems weird?


summary:
case1 seems hard to resolve
case2 can be resolved in ri_GenerateQualCollation, not 100% sure.
case3 can be resolved in RI_FKey_check
because case1 is hard to resolve;
so overall I feel like erroring out PK indeterministic and FK
indeterministic while creating foreign keys is easier.

We can mandate foreign keys and primary key columns be deterministic
in ATAddForeignKeyConstraint.
The attached patch does that.

That means src/test/regress/sql/collate.icu.utf8.sql table test10pk,
table test11pk will have a big change.
so only attach src/backend/commands/tablecmds.c changes.

--0000000000001b5998061d59257c
Content-Type: text/x-patch; charset="US-ASCII"; 
	name="make_pk_fk_tie_collation_deterministic.diff"
Content-Disposition: attachment; 
	filename="make_pk_fk_tie_collation_deterministic.diff"
Content-Transfer-Encoding: base64
Content-ID: <f_lyo56x100>
X-Attachment-Id: f_lyo56x100

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2NvbW1hbmRzL3RhYmxlY21kcy5jIGIvc3JjL2JhY2tl
bmQvY29tbWFuZHMvdGFibGVjbWRzLmMKaW5kZXggNzIxZDI0NzguLjQ5MWJhODdjIDEwMDY0NAot
LS0gYS9zcmMvYmFja2VuZC9jb21tYW5kcy90YWJsZWNtZHMuYworKysgYi9zcmMvYmFja2VuZC9j
b21tYW5kcy90YWJsZWNtZHMuYwpAQCAtOTc1Nyw2ICs5NzU3LDM5IEBAIEFUQWRkRm9yZWlnbktl
eUNvbnN0cmFpbnQoTGlzdCAqKndxdWV1ZSwgQWx0ZXJlZFRhYmxlSW5mbyAqdGFiLCBSZWxhdGlv
biByZWwsCiAJCQkJKGVycmNvZGUoRVJSQ09ERV9JTlZBTElEX0ZPUkVJR05fS0VZKSwKIAkJCQkg
ZXJybXNnKCJudW1iZXIgb2YgcmVmZXJlbmNpbmcgYW5kIHJlZmVyZW5jZWQgY29sdW1ucyBmb3Ig
Zm9yZWlnbiBrZXkgZGlzYWdyZWUiKSkpOwogCisJZm9yIChpID0gMDsgaSA8IG51bXBrczsgaSsr
KQorCXsKKwkJaW50MzIJCWtleWNvbHR5cG1vZDsKKwkJT2lkCQkJa2V5Y29sdHlwZTsKKwkJT2lk
CQkJa2V5Y29sY29sbGF0aW9uOworCisJCWdldF9hdHR0eXBldHlwbW9kY29sbChSZWxhdGlvbkdl
dFJlbGlkKHBrcmVsKSwgcGthdHRudW1baV0sCisJCQkJCQkJCSZrZXljb2x0eXBlLCAma2V5Y29s
dHlwbW9kLAorCQkJCQkJCQkma2V5Y29sY29sbGF0aW9uKTsKKwkJaWYgKE9pZElzVmFsaWQoa2V5
Y29sY29sbGF0aW9uKSAmJgorCQkJCSFnZXRfY29sbGF0aW9uX2lzZGV0ZXJtaW5pc3RpYyhrZXlj
b2xjb2xsYXRpb24pKQorCQkJZXJlcG9ydChFUlJPUiwKKwkJCQkJKGVycmNvZGUoRVJSQ09ERV9E
QVRBVFlQRV9NSVNNQVRDSCksCisJCQkJCQkgZXJybXNnKCJmb3JlaWduIGtleSBjb25zdHJhaW50
IFwiJXNcIiBjYW5ub3QgYmUgaW1wbGVtZW50ZWQiLAorCQkJCQkJCQlma2NvbnN0cmFpbnQtPmNv
bm5hbWUpLAorCQkJCQkJIGVycmRldGFpbCgiQ2Fubm90IGhhdmUgaW5kZXRlcm1pbmlzdGljIGNv
bGxhdGlvbnMgaW4gcHJpbWFyeSBrZXkgZm9yIHJlZmVyZW5jZWQgdGFibGUgXCIlc1wiIiwKKwkJ
CQkJCQkJUmVsYXRpb25HZXRSZWxhdGlvbk5hbWUocGtyZWwpKSwKKwkJCQkJCSBlcnJoaW50KCJZ
b3UgbWF5IG1ha2UgdGhlIHByaW1hcnkga2V5IGNvbGxhdGlvbnMgZGV0ZXJtaW5pc3RpYy4iKSkp
OworCisJCWdldF9hdHR0eXBldHlwbW9kY29sbChSZWxhdGlvbkdldFJlbGlkKHJlbCksIGZrYXR0
bnVtW2ldLAorCQkJCQkJCQkma2V5Y29sdHlwZSwgJmtleWNvbHR5cG1vZCwKKwkJCQkJCQkJJmtl
eWNvbGNvbGxhdGlvbik7CisJCWlmIChPaWRJc1ZhbGlkKGtleWNvbGNvbGxhdGlvbikgJiYKKwkJ
CQkhZ2V0X2NvbGxhdGlvbl9pc2RldGVybWluaXN0aWMoa2V5Y29sY29sbGF0aW9uKSkKKwkJCWVy
ZXBvcnQoRVJST1IsCisJCQkJCShlcnJjb2RlKEVSUkNPREVfREFUQVRZUEVfTUlTTUFUQ0gpLAor
CQkJCQkgZXJybXNnKCJmb3JlaWduIGtleSBjb25zdHJhaW50IFwiJXNcIiBjYW5ub3QgYmUgaW1w
bGVtZW50ZWQiLAorCQkJCQkJCWZrY29uc3RyYWludC0+Y29ubmFtZSksCisJCQkJCSBlcnJkZXRh
aWwoIkZvcmVpZ24ga2V5IGNhbm5vdCB1c2UgaW5kZXRlcm1pbmlzdGljIGNvbGxhdGlvbnMgZm9y
IHJlZmVyZW5jaW5nIHRhYmxlIFwiJXNcIiIsCisJCQkJCQlSZWxhdGlvbkdldFJlbGF0aW9uTmFt
ZShyZWwpKSwKKwkJCQkJIGVycmhpbnQoIllvdSBtYXkgbWFrZSB0aGUgZm9yZWlnbiBrZXkgY29s
bGF0aW9ucyBkZXRlcm1pbmlzdGljLiIpKSk7CisJfQorCiAJLyoKIAkgKiBPbiB0aGUgc3RyZW5n
dGggb2YgYSBwcmV2aW91cyBjb25zdHJhaW50LCB3ZSBtaWdodCBhdm9pZCBzY2FubmluZwogCSAq
IHRhYmxlcyB0byB2YWxpZGF0ZSB0aGlzIG9uZS4gIFNlZSBiZWxvdy4K
--0000000000001b5998061d59257c--