public inbox for [email protected]
help / color / mirror / Atom feedRe: altering a column's collation leaves an invalid foreign key
9+ messages / 4 participants
[nested] [flat]
* Re: altering a column's collation leaves an invalid foreign key
@ 2024-04-13 13:13 jian he <[email protected]>
2024-06-07 06:39 ` Re: altering a column's collation leaves an invalid foreign key jian he <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: jian he @ 2024-04-13 13:13 UTC (permalink / raw)
To: Paul Jungwirth <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Fri, Apr 12, 2024 at 5:06 PM jian he <[email protected]> wrote:
>
> On Tue, Mar 26, 2024 at 1:00 PM jian he <[email protected]> wrote:
> >
> > On Mon, Mar 25, 2024 at 2:47 PM Paul Jungwirth
> > <[email protected]> wrote:
> > >
> > > On 3/23/24 10:04, Paul Jungwirth wrote:
> > > > Perhaps if the previous collation was nondeterministic we should force a re-check.
> > >
> > > 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.
Attachments:
[text/x-patch] v3-0001-ALTER-COLUMN-SET-DATA-TYPE-Re-check-foreign-key-t.patch (10.1K, ../../CACJufxF5W9XUdFcYayyfuKBcxw830q=B0xuA-SYBo7XyMULZrg@mail.gmail.com/2-v3-0001-ALTER-COLUMN-SET-DATA-TYPE-Re-check-foreign-key-t.patch)
download | inline diff:
From 0eb0846450cb33996d6fb35c19290d297c26fd3c Mon Sep 17 00:00:00 2001
From: jian he <[email protected]>
Date: Sat, 13 Apr 2024 20:28:25 +0800
Subject: [PATCH v3 1/1] ALTER COLUMN SET DATA TYPE Re-check foreign key ties
under certain condition
with deterministic collations, we check ties by looking at binary
equality. But nondeterministic collation can allow non-identical values
to be considered equal, e.g. 'a' and 'A' when case-insensitive.
ALTER COLUMN .. SET DATA TYPE make
some references that used to be valid may not be anymore.
for `ALTER COLUMN .. SET DATA TYPE`:
If the changed key columns is part of the primary key columns,
then under the following condition
we need to revalidate the foreign key constraint:
* the primary key is referenced by a foreign key constraint
* the new collation is not the same as the old
* the old collation is nondeterministic.
discussion: https://postgr.es/m/78d824e0-b21e-480d-a252-e4b84bc2c24b%40illuminatedcomputing.com
---
src/backend/commands/tablecmds.c | 62 +++++++++++++++----
src/include/nodes/parsenodes.h | 3 +-
.../regress/expected/collate.icu.utf8.out | 9 +++
src/test/regress/sql/collate.icu.utf8.sql | 8 +++
4 files changed, 69 insertions(+), 13 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 000212f2..d5bd61f1 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -198,6 +198,7 @@ typedef struct AlteredTableInfo
/* Objects to rebuild after completing ALTER TYPE operations */
List *changedConstraintOids; /* OIDs of constraints to rebuild */
List *changedConstraintDefs; /* string definitions of same */
+ bool verify_new_collation; /* T if we should recheck new collation for foreign key */
List *changedIndexOids; /* OIDs of indexes to rebuild */
List *changedIndexDefs; /* string definitions of same */
char *replicaIdentityIndex; /* index to reset as REPLICA IDENTITY */
@@ -583,12 +584,13 @@ static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab,
LOCKMODE lockmode);
static void ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId,
char *cmd, List **wqueue, LOCKMODE lockmode,
- bool rewrite);
+ bool rewrite,
+ bool verify_new_collation);
static void RebuildConstraintComment(AlteredTableInfo *tab, AlterTablePass pass,
Oid objid, Relation rel, List *domname,
const char *conname);
static void TryReuseIndex(Oid oldId, IndexStmt *stmt);
-static void TryReuseForeignKey(Oid oldId, Constraint *con);
+static void TryReuseForeignKey(Oid oldId, Constraint *con, bool verify_new_collation);
static ObjectAddress ATExecAlterColumnGenericOptions(Relation rel, const char *colName,
List *options, LOCKMODE lockmode);
static void change_owner_fix_column_acls(Oid relationOid,
@@ -10256,6 +10258,15 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
old_pfeqop_item);
}
if (old_check_ok)
+ {
+ /* also see TryReuseForeignKey.
+ * collation_recheck is true means that, we need to
+ * recheck the foreign key side value is ok with the
+ * new primey key collation.
+ */
+ old_check_ok = !fkconstraint->collation_recheck;
+ }
+ if (old_check_ok)
{
Oid old_fktype;
Oid new_fktype;
@@ -10301,10 +10312,6 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
* turn conform to the domain. Consequently, we need not treat
* domains specially here.
*
- * Since we require that all collations share the same notion of
- * equality (which they do, because texteq reduces to bitwise
- * equality), we don't compare collation here.
- *
* We need not directly consider the PK type. It's necessarily
* binary coercible to the opcintype of the unique index column,
* and ri_triggers.c will only deal with PK datums in terms of
@@ -13738,6 +13745,30 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
/* And the collation */
targetcollid = GetColumnDefCollation(NULL, def, targettype);
+ if (OidIsValid(targetcollid))
+ {
+ Oid typid;
+ int32 typmod;
+ Oid collid;
+
+ get_atttypetypmodcoll(RelationGetRelid(tab->rel), attnum,
+ &typid, &typmod, &collid);
+ /*
+ * All deterministic collations use bitwise equality to resolve
+ * ties, but if the previous(source) collation was indeterminstic,
+ * and previous collation is not the same as the target collation then
+ * we must re-check the foreign key, because some references
+ * that use to be "equal" may not be anymore. we first store this
+ * information in AlteredTableInfo, later we pass it to foreign key
+ * Constraint.
+ * also see ATAddForeignKeyConstraint.
+ */
+ if (OidIsValid(collid))
+ {
+ if (!get_collation_isdeterministic(collid) && targetcollid != collid)
+ tab->verify_new_collation = true;
+ }
+ }
/*
* If there is a default expression for the column, get it and ensure we
* can coerce it to the new datatype. (We must do this before changing
@@ -14406,7 +14437,8 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
ATPostAlterTypeParse(oldId, relid, confrelid,
(char *) lfirst(def_item),
- wqueue, lockmode, tab->rewrite);
+ wqueue, lockmode, tab->rewrite,
+ tab->verify_new_collation);
}
forboth(oid_item, tab->changedIndexOids,
def_item, tab->changedIndexDefs)
@@ -14417,7 +14449,8 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
relid = IndexGetRelation(oldId, false);
ATPostAlterTypeParse(oldId, relid, InvalidOid,
(char *) lfirst(def_item),
- wqueue, lockmode, tab->rewrite);
+ wqueue, lockmode, tab->rewrite,
+ tab->verify_new_collation);
ObjectAddressSet(obj, RelationRelationId, oldId);
add_exact_object_address(&obj, objects);
@@ -14433,7 +14466,8 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
relid = StatisticsGetRelation(oldId, false);
ATPostAlterTypeParse(oldId, relid, InvalidOid,
(char *) lfirst(def_item),
- wqueue, lockmode, tab->rewrite);
+ wqueue, lockmode, tab->rewrite,
+ tab->verify_new_collation);
ObjectAddressSet(obj, StatisticExtRelationId, oldId);
add_exact_object_address(&obj, objects);
@@ -14496,7 +14530,8 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
*/
static void
ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
- List **wqueue, LOCKMODE lockmode, bool rewrite)
+ List **wqueue, LOCKMODE lockmode, bool rewrite,
+ bool verify_new_collation)
{
List *raw_parsetree_list;
List *querytree_list;
@@ -14623,7 +14658,7 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
/* rewriting neither side of a FK */
if (con->contype == CONSTR_FOREIGN &&
!rewrite && tab->rewrite == 0)
- TryReuseForeignKey(oldId, con);
+ TryReuseForeignKey(oldId, con, verify_new_collation);
con->reset_default_tblspc = true;
cmd->subtype = AT_ReAddConstraint;
tab->subcmds[AT_PASS_OLD_CONSTR] =
@@ -14784,7 +14819,7 @@ TryReuseIndex(Oid oldId, IndexStmt *stmt)
* this constraint can be skipped.
*/
static void
-TryReuseForeignKey(Oid oldId, Constraint *con)
+TryReuseForeignKey(Oid oldId, Constraint *con, bool verify_new_collation)
{
HeapTuple tup;
Datum adatum;
@@ -14816,6 +14851,9 @@ TryReuseForeignKey(Oid oldId, Constraint *con)
con->old_conpfeqop = lappend_oid(con->old_conpfeqop, rawarr[i]);
ReleaseSysCache(tup);
+
+ /* verify_new_collation is computed at ATExecAlterColumnType */
+ con->collation_recheck = verify_new_collation;
}
/*
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index f763f790..0349c61c 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2777,7 +2777,8 @@ typedef struct Constraint
List *old_conpfeqop; /* pg_constraint.conpfeqop of my former self */
Oid old_pktable_oid; /* pg_constraint.confrelid of my former
* self */
-
+ bool collation_recheck; /* does primary key columns collation change need
+ * foreign key constraint recheck */
ParseLoc location; /* token location, or -1 if unknown */
} Constraint;
diff --git a/src/test/regress/expected/collate.icu.utf8.out b/src/test/regress/expected/collate.icu.utf8.out
index 4b8c8f14..4119776f 100644
--- a/src/test/regress/expected/collate.icu.utf8.out
+++ b/src/test/regress/expected/collate.icu.utf8.out
@@ -1933,6 +1933,15 @@ SELECT * FROM test11fk;
---
(0 rows)
+-- Test altering the collation of the referenced column.
+CREATE TABLE pktable (x text COLLATE case_insensitive PRIMARY KEY);
+CREATE TABLE fktable (x text REFERENCES pktable);
+INSERT INTO pktable VALUES ('a');
+INSERT INTO fktable VALUES ('A');
+-- should fail:
+ALTER TABLE pktable ALTER COLUMN x TYPE text COLLATE "C";
+ERROR: insert or update on table "fktable" violates foreign key constraint "fktable_x_fkey"
+DETAIL: Key (x)=(A) is not present in table "pktable".
-- partitioning
CREATE TABLE test20 (a int, b text COLLATE case_insensitive) PARTITION BY LIST (b);
CREATE TABLE test20_1 PARTITION OF test20 FOR VALUES IN ('abc');
diff --git a/src/test/regress/sql/collate.icu.utf8.sql b/src/test/regress/sql/collate.icu.utf8.sql
index 80f28a97..08a69649 100644
--- a/src/test/regress/sql/collate.icu.utf8.sql
+++ b/src/test/regress/sql/collate.icu.utf8.sql
@@ -721,6 +721,14 @@ DELETE FROM test11pk WHERE x = 'abc';
SELECT * FROM test11pk;
SELECT * FROM test11fk;
+-- Test altering the collation of the referenced column.
+CREATE TABLE pktable (x text COLLATE case_insensitive PRIMARY KEY);
+CREATE TABLE fktable (x text REFERENCES pktable);
+INSERT INTO pktable VALUES ('a');
+INSERT INTO fktable VALUES ('A');
+-- should fail:
+ALTER TABLE pktable ALTER COLUMN x TYPE text COLLATE "C";
+
-- partitioning
CREATE TABLE test20 (a int, b text COLLATE case_insensitive) PARTITION BY LIST (b);
CREATE TABLE test20_1 PARTITION OF test20 FOR VALUES IN ('abc');
base-commit: 4cc1c76fe9f13aa96bae14f4fcfdf6d508af72a4
--
2.34.1
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: altering a column's collation leaves an invalid foreign key
2024-04-13 13:13 Re: altering a column's collation leaves an invalid foreign key jian he <[email protected]>
@ 2024-06-07 06:39 ` jian he <[email protected]>
2024-06-07 20:12 ` Re: altering a column's collation leaves an invalid foreign key Tom Lane <[email protected]>
2024-09-03 09:41 ` Re: altering a column's collation leaves an invalid foreign key Peter Eisentraut <[email protected]>
0 siblings, 2 replies; 9+ messages in thread
From: jian he @ 2024-06-07 06:39 UTC (permalink / raw)
To: Paul Jungwirth <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
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.
Attachments:
[text/x-patch] v4-0001-Revalidate-PK-FK-ties-when-PK-collation-is-change.patch (10.9K, ../../CACJufxE3U650j=acBuksB+D9GoGVwU0Q2q+7b-BiF6muyp8_qA@mail.gmail.com/2-v4-0001-Revalidate-PK-FK-ties-when-PK-collation-is-change.patch)
download | inline diff:
From 9a2e73231e037c7555449d8790885c1feb4f3ed0 Mon Sep 17 00:00:00 2001
From: jian he <[email protected]>
Date: Fri, 7 Jun 2024 14:32:55 +0800
Subject: [PATCH v4 1/1] Revalidate PK-FK ties when PK collation is changed
With deterministic collations, we check the PK-FK ties by looking at binary
equality. But nondeterministic collation can allow non-identical values
to be considered equal, e.g. 'a' and 'A' when case-insensitive.
ALTER COLUMN .. SET DATA TYPE make
some references that used to be valid may not be anymore.
for `ALTER COLUMN .. SET DATA TYPE`:
If the changed key column is part of the primary key columns,
then under the following conditions we need to revalidate the PK-FK ties
* the PK new collation is different from the old
* the old collation is nondeterministic.
we need to revalidate the PK-FK ties.
discussion: https://postgr.es/m/78d824e0-b21e-480d-a252-e4b84bc2c24b%40illuminatedcomputing.com
---
src/backend/commands/tablecmds.c | 61 +++++++++++++++----
src/include/nodes/parsenodes.h | 3 +-
.../regress/expected/collate.icu.utf8.out | 18 ++++++
src/test/regress/sql/collate.icu.utf8.sql | 16 +++++
4 files changed, 85 insertions(+), 13 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7b6c69b7..77337b6f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -199,6 +199,7 @@ typedef struct AlteredTableInfo
/* Objects to rebuild after completing ALTER TYPE operations */
List *changedConstraintOids; /* OIDs of constraints to rebuild */
List *changedConstraintDefs; /* string definitions of same */
+ bool verify_new_collation; /* T if we need recheck the new collation */
List *changedIndexOids; /* OIDs of indexes to rebuild */
List *changedIndexDefs; /* string definitions of same */
char *replicaIdentityIndex; /* index to reset as REPLICA IDENTITY */
@@ -572,12 +573,13 @@ static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab,
LOCKMODE lockmode);
static void ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId,
char *cmd, List **wqueue, LOCKMODE lockmode,
- bool rewrite);
+ bool rewrite,
+ bool verify_new_collation);
static void RebuildConstraintComment(AlteredTableInfo *tab, AlterTablePass pass,
Oid objid, Relation rel, List *domname,
const char *conname);
static void TryReuseIndex(Oid oldId, IndexStmt *stmt);
-static void TryReuseForeignKey(Oid oldId, Constraint *con);
+static void TryReuseForeignKey(Oid oldId, Constraint *con, bool verify_new_collation);
static ObjectAddress ATExecAlterColumnGenericOptions(Relation rel, const char *colName,
List *options, LOCKMODE lockmode);
static void change_owner_fix_column_acls(Oid relationOid,
@@ -9857,6 +9859,15 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
old_pfeqop_item);
}
if (old_check_ok)
+ {
+ /*
+ * collation_recheck is true then we need to
+ * revalidate the foreign key and primary key value ties.
+ * see also TryReuseForeignKey.
+ */
+ old_check_ok = !fkconstraint->collation_recheck;
+ }
+ if (old_check_ok)
{
Oid old_fktype;
Oid new_fktype;
@@ -9902,10 +9913,6 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
* turn conform to the domain. Consequently, we need not treat
* domains specially here.
*
- * Since we require that all collations share the same notion of
- * equality (which they do, because texteq reduces to bitwise
- * equality), we don't compare collation here.
- *
* We need not directly consider the PK type. It's necessarily
* binary coercible to the opcintype of the unique index column,
* and ri_triggers.c will only deal with PK datums in terms of
@@ -13035,6 +13042,29 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
/* And the collation */
targetcollid = GetColumnDefCollation(NULL, def, targettype);
+ if (OidIsValid(targetcollid))
+ {
+ Oid typid;
+ int32 typmod;
+ Oid source_collid;
+
+ get_atttypetypmodcoll(RelationGetRelid(tab->rel), attnum,
+ &typid, &typmod, &source_collid);
+ /*
+ * All deterministic collations use bitwise equality to resolve
+ * PK-FK ties. But if the primary key (source) collation was indeterministic,
+ * and ALTER COLUMN .. SET DATA TYPE changes the primary key collation, then
+ * we must re-check the PK-FK ties, because some references
+ * that used to be "equal" may not be anymore.
+ * We add a flag in AlteredTableInfo to signify
+ * that we need to verify the new collation for PK-FK ties.
+ * see ATAddForeignKeyConstraint also.
+ */
+ if (OidIsValid(source_collid) &&
+ !get_collation_isdeterministic(source_collid) &&
+ (targetcollid != source_collid))
+ tab->verify_new_collation = true;
+ }
/*
* If there is a default expression for the column, get it and ensure we
* can coerce it to the new datatype. (We must do this before changing
@@ -13742,7 +13772,8 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
ATPostAlterTypeParse(oldId, relid, confrelid,
(char *) lfirst(def_item),
- wqueue, lockmode, tab->rewrite);
+ wqueue, lockmode, tab->rewrite,
+ tab->verify_new_collation);
}
forboth(oid_item, tab->changedIndexOids,
def_item, tab->changedIndexDefs)
@@ -13753,7 +13784,8 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
relid = IndexGetRelation(oldId, false);
ATPostAlterTypeParse(oldId, relid, InvalidOid,
(char *) lfirst(def_item),
- wqueue, lockmode, tab->rewrite);
+ wqueue, lockmode, tab->rewrite,
+ tab->verify_new_collation);
ObjectAddressSet(obj, RelationRelationId, oldId);
add_exact_object_address(&obj, objects);
@@ -13769,7 +13801,8 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
relid = StatisticsGetRelation(oldId, false);
ATPostAlterTypeParse(oldId, relid, InvalidOid,
(char *) lfirst(def_item),
- wqueue, lockmode, tab->rewrite);
+ wqueue, lockmode, tab->rewrite,
+ tab->verify_new_collation);
ObjectAddressSet(obj, StatisticExtRelationId, oldId);
add_exact_object_address(&obj, objects);
@@ -13832,7 +13865,8 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
*/
static void
ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
- List **wqueue, LOCKMODE lockmode, bool rewrite)
+ List **wqueue, LOCKMODE lockmode, bool rewrite,
+ bool verify_new_collation)
{
List *raw_parsetree_list;
List *querytree_list;
@@ -13959,7 +13993,7 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
/* rewriting neither side of a FK */
if (con->contype == CONSTR_FOREIGN &&
!rewrite && tab->rewrite == 0)
- TryReuseForeignKey(oldId, con);
+ TryReuseForeignKey(oldId, con, verify_new_collation);
con->reset_default_tblspc = true;
cmd->subtype = AT_ReAddConstraint;
tab->subcmds[AT_PASS_OLD_CONSTR] =
@@ -14119,7 +14153,7 @@ TryReuseIndex(Oid oldId, IndexStmt *stmt)
* this constraint can be skipped.
*/
static void
-TryReuseForeignKey(Oid oldId, Constraint *con)
+TryReuseForeignKey(Oid oldId, Constraint *con, bool verify_new_collation)
{
HeapTuple tup;
Datum adatum;
@@ -14151,6 +14185,9 @@ TryReuseForeignKey(Oid oldId, Constraint *con)
con->old_conpfeqop = lappend_oid(con->old_conpfeqop, rawarr[i]);
ReleaseSysCache(tup);
+
+ /* verify_new_collation is computed at ATExecAlterColumnType */
+ con->collation_recheck = verify_new_collation;
}
/*
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index ddfed02d..29b484f5 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2773,7 +2773,8 @@ typedef struct Constraint
List *old_conpfeqop; /* pg_constraint.conpfeqop of my former self */
Oid old_pktable_oid; /* pg_constraint.confrelid of my former
* self */
-
+ bool collation_recheck; /* does primary key columns collation changes need
+ * foreign key constraint recheck */
ParseLoc location; /* token location, or -1 if unknown */
} Constraint;
diff --git a/src/test/regress/expected/collate.icu.utf8.out b/src/test/regress/expected/collate.icu.utf8.out
index 7d59fb44..ea28031b 100644
--- a/src/test/regress/expected/collate.icu.utf8.out
+++ b/src/test/regress/expected/collate.icu.utf8.out
@@ -1933,6 +1933,24 @@ SELECT * FROM test11fk;
---
(0 rows)
+-- Test altering the collation of the referenced column.
+CREATE COLLATION ignore_accent_case (provider = icu, deterministic = false, locale = 'und-u-ks-level1');
+CREATE TABLE pktable (x text COLLATE case_insensitive PRIMARY KEY);
+CREATE TABLE fktable (x text REFERENCES pktable);
+INSERT INTO pktable VALUES ('a');
+INSERT INTO fktable VALUES ('A');
+-- should fail:
+ALTER TABLE pktable ALTER COLUMN x TYPE text COLLATE "C";
+ERROR: insert or update on table "fktable" violates foreign key constraint "fktable_x_fkey"
+DETAIL: Key (x)=(A) is not present in table "pktable".
+-- should fail:
+ALTER TABLE pktable ALTER COLUMN x TYPE text COLLATE ignore_accents;
+ERROR: insert or update on table "fktable" violates foreign key constraint "fktable_x_fkey"
+DETAIL: Key (x)=(A) is not present in table "pktable".
+-- should ok:
+ALTER TABLE pktable ALTER COLUMN x TYPE text COLLATE case_insensitive;
+ALTER TABLE pktable ALTER COLUMN x TYPE text COLLATE ignore_accent_case;
+DROP TABLE fktable, pktable;
-- partitioning
CREATE TABLE test20 (a int, b text COLLATE case_insensitive) PARTITION BY LIST (b);
CREATE TABLE test20_1 PARTITION OF test20 FOR VALUES IN ('abc');
diff --git a/src/test/regress/sql/collate.icu.utf8.sql b/src/test/regress/sql/collate.icu.utf8.sql
index 80f28a97..dcdbbc48 100644
--- a/src/test/regress/sql/collate.icu.utf8.sql
+++ b/src/test/regress/sql/collate.icu.utf8.sql
@@ -721,6 +721,22 @@ DELETE FROM test11pk WHERE x = 'abc';
SELECT * FROM test11pk;
SELECT * FROM test11fk;
+-- Test altering the collation of the referenced column.
+CREATE COLLATION ignore_accent_case (provider = icu, deterministic = false, locale = 'und-u-ks-level1');
+CREATE TABLE pktable (x text COLLATE case_insensitive PRIMARY KEY);
+CREATE TABLE fktable (x text REFERENCES pktable);
+INSERT INTO pktable VALUES ('a');
+INSERT INTO fktable VALUES ('A');
+-- should fail:
+ALTER TABLE pktable ALTER COLUMN x TYPE text COLLATE "C";
+
+-- should fail:
+ALTER TABLE pktable ALTER COLUMN x TYPE text COLLATE ignore_accents;
+
+-- should ok:
+ALTER TABLE pktable ALTER COLUMN x TYPE text COLLATE case_insensitive;
+ALTER TABLE pktable ALTER COLUMN x TYPE text COLLATE ignore_accent_case;
+DROP TABLE fktable, pktable;
-- partitioning
CREATE TABLE test20 (a int, b text COLLATE case_insensitive) PARTITION BY LIST (b);
CREATE TABLE test20_1 PARTITION OF test20 FOR VALUES IN ('abc');
--
2.34.1
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: altering a column's collation leaves an invalid foreign key
2024-04-13 13:13 Re: altering a column's collation leaves an invalid foreign key jian he <[email protected]>
2024-06-07 06:39 ` Re: altering a column's collation leaves an invalid foreign key jian he <[email protected]>
@ 2024-06-07 20:12 ` Tom Lane <[email protected]>
2024-06-08 04:14 ` Re: altering a column's collation leaves an invalid foreign key jian he <[email protected]>
1 sibling, 1 reply; 9+ messages in thread
From: Tom Lane @ 2024-06-07 20:12 UTC (permalink / raw)
To: jian he <[email protected]>; +Cc: Paul Jungwirth <[email protected]>; PostgreSQL Hackers <[email protected]>
jian he <[email protected]> writes:
>> * 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.
I have a basic question about this: why are we allowing FKs to be
based on nondeterministic collations at all? ISTM that that breaks
the assumption that there is exactly one referenced row for any
referencing row.
regards, tom lane
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: altering a column's collation leaves an invalid foreign key
2024-04-13 13:13 Re: altering a column's collation leaves an invalid foreign key jian he <[email protected]>
2024-06-07 06:39 ` Re: altering a column's collation leaves an invalid foreign key jian he <[email protected]>
2024-06-07 20:12 ` Re: altering a column's collation leaves an invalid foreign key Tom Lane <[email protected]>
@ 2024-06-08 04:14 ` jian he <[email protected]>
2024-06-18 08:50 ` Re: altering a column's collation leaves an invalid foreign key Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: jian he @ 2024-06-08 04:14 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Paul Jungwirth <[email protected]>; PostgreSQL Hackers <[email protected]>
On Sat, Jun 8, 2024 at 4:12 AM Tom Lane <[email protected]> wrote:
>
> jian he <[email protected]> writes:
> >> * 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.
>
> I have a basic question about this: why are we allowing FKs to be
> based on nondeterministic collations at all? ISTM that that breaks
> the assumption that there is exactly one referenced row for any
> referencing row.
>
for FKs nondeterministic,
I think that would require the PRIMARY KEY collation to not be
indeterministic also.
for example:
CREATE COLLATION ignore_accent_case (provider = icu, deterministic =
false, locale = 'und-u-ks-level1');
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');
update pktable set x = 'Å';
table fktable;
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'), ('Å');
INSERT INTO fktable VALUES ('A');
begin; delete from pktable where x = 'Å'; TABLE fktable; rollback;
begin; delete from pktable where x = 'A'; TABLE fktable; rollback;
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: altering a column's collation leaves an invalid foreign key
2024-04-13 13:13 Re: altering a column's collation leaves an invalid foreign key jian he <[email protected]>
2024-06-07 06:39 ` Re: altering a column's collation leaves an invalid foreign key jian he <[email protected]>
2024-06-07 20:12 ` Re: altering a column's collation leaves an invalid foreign key Tom Lane <[email protected]>
2024-06-08 04:14 ` Re: altering a column's collation leaves an invalid foreign key jian he <[email protected]>
@ 2024-06-18 08:50 ` Peter Eisentraut <[email protected]>
2024-07-16 08:29 ` Re: altering a column's collation leaves an invalid foreign key jian he <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Peter Eisentraut @ 2024-06-18 08:50 UTC (permalink / raw)
To: jian he <[email protected]>; Tom Lane <[email protected]>; +Cc: Paul Jungwirth <[email protected]>; PostgreSQL Hackers <[email protected]>
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'), ('Å');
> 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.
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: altering a column's collation leaves an invalid foreign key
2024-04-13 13:13 Re: altering a column's collation leaves an invalid foreign key jian he <[email protected]>
2024-06-07 06:39 ` Re: altering a column's collation leaves an invalid foreign key jian he <[email protected]>
2024-06-07 20:12 ` Re: altering a column's collation leaves an invalid foreign key Tom Lane <[email protected]>
2024-06-08 04:14 ` Re: altering a column's collation leaves an invalid foreign key jian he <[email protected]>
2024-06-18 08:50 ` Re: altering a column's collation leaves an invalid foreign key Peter Eisentraut <[email protected]>
@ 2024-07-16 08:29 ` jian he <[email protected]>
0 siblings, 0 replies; 9+ messages in thread
From: jian he @ 2024-07-16 08:29 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Tom Lane <[email protected]>; Paul Jungwirth <[email protected]>; PostgreSQL Hackers <[email protected]>
On Tue, Jun 18, 2024 at 4:50 PM Peter Eisentraut <[email protected]> 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'), ('Å');
> > 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.=) $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.=) $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'), ('Å');
INSERT INTO fktable VALUES ('A');
begin; update pktable set x = 'B' where x = 'Å'; table fktable; rollback;
begin; update pktable set x = 'B' where x = 'A'; table fktable; rollback;
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 = 'Å'; 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.=) '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.
Attachments:
[text/x-patch] make_pk_fk_tie_collation_deterministic.diff (1.8K, ../../CACJufxEW6OMBqt8cbr=3Jt++Zd_SL-4YDjfk7Q7DhGKiSLcu4g@mail.gmail.com/2-make_pk_fk_tie_collation_deterministic.diff)
download | inline diff:
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 721d2478..491ba87c 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -9757,6 +9757,39 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
(errcode(ERRCODE_INVALID_FOREIGN_KEY),
errmsg("number of referencing and referenced columns for foreign key disagree")));
+ for (i = 0; i < numpks; i++)
+ {
+ int32 keycoltypmod;
+ Oid keycoltype;
+ Oid keycolcollation;
+
+ get_atttypetypmodcoll(RelationGetRelid(pkrel), pkattnum[i],
+ &keycoltype, &keycoltypmod,
+ &keycolcollation);
+ if (OidIsValid(keycolcollation) &&
+ !get_collation_isdeterministic(keycolcollation))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("foreign key constraint \"%s\" cannot be implemented",
+ fkconstraint->conname),
+ errdetail("Cannot have indeterministic collations in primary key for referenced table \"%s\"",
+ RelationGetRelationName(pkrel)),
+ errhint("You may make the primary key collations deterministic.")));
+
+ get_atttypetypmodcoll(RelationGetRelid(rel), fkattnum[i],
+ &keycoltype, &keycoltypmod,
+ &keycolcollation);
+ if (OidIsValid(keycolcollation) &&
+ !get_collation_isdeterministic(keycolcollation))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("foreign key constraint \"%s\" cannot be implemented",
+ fkconstraint->conname),
+ errdetail("Foreign key cannot use indeterministic collations for referencing table \"%s\"",
+ RelationGetRelationName(rel)),
+ errhint("You may make the foreign key collations deterministic.")));
+ }
+
/*
* On the strength of a previous constraint, we might avoid scanning
* tables to validate this one. See below.
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: altering a column's collation leaves an invalid foreign key
2024-04-13 13:13 Re: altering a column's collation leaves an invalid foreign key jian he <[email protected]>
2024-06-07 06:39 ` Re: altering a column's collation leaves an invalid foreign key jian he <[email protected]>
@ 2024-09-03 09:41 ` Peter Eisentraut <[email protected]>
2024-09-04 06:54 ` Re: altering a column's collation leaves an invalid foreign key jian he <[email protected]>
1 sibling, 1 reply; 9+ messages in thread
From: Peter Eisentraut @ 2024-09-03 09:41 UTC (permalink / raw)
To: jian he <[email protected]>; Paul Jungwirth <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
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.
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: altering a column's collation leaves an invalid foreign key
2024-04-13 13:13 Re: altering a column's collation leaves an invalid foreign key jian he <[email protected]>
2024-06-07 06:39 ` Re: altering a column's collation leaves an invalid foreign key jian he <[email protected]>
2024-09-03 09:41 ` Re: altering a column's collation leaves an invalid foreign key Peter Eisentraut <[email protected]>
@ 2024-09-04 06:54 ` jian he <[email protected]>
0 siblings, 0 replies; 9+ messages in thread
From: jian he @ 2024-09-04 06:54 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Paul Jungwirth <[email protected]>; PostgreSQL Hackers <[email protected]>
On Tue, Sep 3, 2024 at 5:41 PM Peter Eisentraut <[email protected]> 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.
[1] https://www.postgresql.org/message-id/CACJufxEW6OMBqt8cbr%3D3Jt%2B%2BZd_SL-4YDjfk7Q7DhGKiSLcu4g%40ma...
Attachments:
[text/x-patch] v5-0001-ensure-PK-FK-ties-related-collation-indeterminist.patch (14.7K, ../../CACJufxFy9EEpqv3Q3XTM3aAQ2UbzbyavLzN=WT1yXC7JcZqzhA@mail.gmail.com/2-v5-0001-ensure-PK-FK-ties-related-collation-indeterminist.patch)
download | inline diff:
From 024d818e0e286af10b74649bd3156836d6f28447 Mon Sep 17 00:00:00 2001
From: jian he <[email protected]>
Date: Wed, 4 Sep 2024 14:31:43 +0800
Subject: [PATCH v5 1/1] ensure PK-FK ties related collation indeterministic
PK can have indeterministic collation as long as it not interact with FK.
To make the PK-FK tie consistent, we need to ensure both side's collation is
deterministic, otherwise, the following two some anomalitys can happen.
1. PK side collation is indeterministic, multiple form FK values equal to one PK
value.
2. FK side collation is indeterministic, multiple form PK values equal to one FK
value.
discussion: https://postgr.es/m/78d824e0-b21e-480d-a252-e4b84bc2c24b%40illuminatedcomputing.com
---
src/backend/commands/tablecmds.c | 65 ++++++++
.../regress/expected/collate.icu.utf8.out | 142 +++++++-----------
src/test/regress/sql/collate.icu.utf8.sql | 60 ++++----
3 files changed, 151 insertions(+), 116 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b3cc6f8f69..6c951ae36e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -9705,6 +9705,71 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
(errcode(ERRCODE_INVALID_FOREIGN_KEY),
errmsg("number of referencing and referenced columns for foreign key disagree")));
+ for (i = 0; i < numpks; i++)
+ {
+ int32 keycoltypmod;
+ Oid keycoltype;
+ Oid keycolcollation;
+ Oid typcollation;
+ bool col_coll_deterministic = true;
+
+ /*
+ * for both foreign key and primary key, we need make sure the specified
+ * keys collation (via COLLATE clause) is deterministic. we also need to
+ * ensure the key's typcollation is indeterministic (for domain type).
+ *
+ * PK and FK both are not indeterministic, then anomality can happen:
+ * 1. PK side collation is indeterministic, multiple form FK values
+ * equal to one PK value.
+ * 2. FK side collation is indeterministic, multiple form PK values
+ * equal to one FK value.
+ *
+ */
+
+ /* checking PK collation deterministic */
+ get_atttypetypmodcoll(RelationGetRelid(pkrel), pkattnum[i],
+ &keycoltype, &keycoltypmod,
+ &keycolcollation);
+
+ typcollation = get_typcollation(keycoltype);
+ if (OidIsValid(typcollation))
+ col_coll_deterministic = get_collation_isdeterministic(typcollation);
+
+ if (col_coll_deterministic && OidIsValid(keycolcollation))
+ col_coll_deterministic = get_collation_isdeterministic(keycolcollation);
+
+ if (!col_coll_deterministic)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("foreign key constraint \"%s\" cannot be implemented",
+ fkconstraint->conname),
+ errdetail("Cannot have indeterministic collations in primary key for referenced table \"%s\".",
+ RelationGetRelationName(pkrel)),
+ errhint("You may need to make primary key constraint's collation deterministic.")));
+
+
+ /* checking PK collation deterministic */
+ get_atttypetypmodcoll(RelationGetRelid(rel), fkattnum[i],
+ &keycoltype, &keycoltypmod,
+ &keycolcollation);
+
+ typcollation = get_typcollation(keycoltype);
+ if (OidIsValid(typcollation))
+ col_coll_deterministic = get_collation_isdeterministic(typcollation);
+
+ if (col_coll_deterministic && OidIsValid(keycolcollation))
+ col_coll_deterministic = get_collation_isdeterministic(keycolcollation);
+
+ if (!col_coll_deterministic)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("foreign key constraint \"%s\" cannot be implemented",
+ fkconstraint->conname),
+ errdetail("Foreign key constraint cannot use indeterministic collation for referencing table \"%s\".",
+ RelationGetRelationName(rel)),
+ errhint("You may need to make the foreign key constraint's collation deterministic.")));
+ }
+
/*
* On the strength of a previous constraint, we might avoid scanning
* tables to validate this one. See below.
diff --git a/src/test/regress/expected/collate.icu.utf8.out b/src/test/regress/expected/collate.icu.utf8.out
index 31345295c1..5074812099 100644
--- a/src/test/regress/expected/collate.icu.utf8.out
+++ b/src/test/regress/expected/collate.icu.utf8.out
@@ -1841,100 +1841,62 @@ SELECT * FROM test4 WHERE b = 'Cote' COLLATE case_insensitive;
(1 row)
-- foreign keys (should use collation of primary key)
--- PK is case-sensitive, FK is case-insensitive
+CREATE COLLATION ignore_accent_case (provider = icu, deterministic = false, locale = 'und-u-ks-level1');
+CREATE DOMAIN text_d_ignore_accent_case AS text COLLATE ignore_accent_case;
+-- PK is case-sensitive (deterministic), FK is case-insensitive (indeterministic).
+-- not OK, multiple PK-side value equal to one FK-side value not allowed
+-- e.g. not allowed case: PK values {'a', 'A"} both equal to FK value {'A'}
CREATE TABLE test10pk (x text COLLATE case_sensitive PRIMARY KEY);
-INSERT INTO test10pk VALUES ('abc'), ('def'), ('ghi');
CREATE TABLE test10fk (x text COLLATE case_insensitive REFERENCES test10pk (x) ON UPDATE CASCADE ON DELETE CASCADE);
-INSERT INTO test10fk VALUES ('abc'); -- ok
-INSERT INTO test10fk VALUES ('ABC'); -- error
-ERROR: insert or update on table "test10fk" violates foreign key constraint "test10fk_x_fkey"
-DETAIL: Key (x)=(ABC) is not present in table "test10pk".
-INSERT INTO test10fk VALUES ('xyz'); -- error
-ERROR: insert or update on table "test10fk" violates foreign key constraint "test10fk_x_fkey"
-DETAIL: Key (x)=(xyz) is not present in table "test10pk".
-SELECT * FROM test10pk;
- x
------
- abc
- def
- ghi
-(3 rows)
-
-SELECT * FROM test10fk;
- x
------
- abc
-(1 row)
-
--- restrict update even though the values are "equal" in the FK table
-UPDATE test10fk SET x = 'ABC' WHERE x = 'abc'; -- error
-ERROR: insert or update on table "test10fk" violates foreign key constraint "test10fk_x_fkey"
-DETAIL: Key (x)=(ABC) is not present in table "test10pk".
-SELECT * FROM test10fk;
- x
------
- abc
-(1 row)
-
-DELETE FROM test10pk WHERE x = 'abc';
-SELECT * FROM test10pk;
- x
------
- def
- ghi
-(2 rows)
-
-SELECT * FROM test10fk;
- x
----
-(0 rows)
-
--- PK is case-insensitive, FK is case-sensitive
+ERROR: foreign key constraint "test10fk_x_fkey" cannot be implemented
+DETAIL: Foreign key constraint cannot use indeterministic collation for referencing table "test10fk".
+HINT: You may need to make the foreign key constraint's collation deterministic.
+-- PK is case-insensitive (indeterministic), FK is case-sensitive (deterministic)
+-- more than one referencing (FK) row value equal to referenced (PK) row should not allowed
+-- e.g. not allowed case: FK values {'a', 'A"} both equal to PK value {'A'}
CREATE TABLE test11pk (x text COLLATE case_insensitive PRIMARY KEY);
-INSERT INTO test11pk VALUES ('abc'), ('def'), ('ghi');
CREATE TABLE test11fk (x text COLLATE case_sensitive REFERENCES test11pk (x) ON UPDATE CASCADE ON DELETE CASCADE);
-INSERT INTO test11fk VALUES ('abc'); -- ok
-INSERT INTO test11fk VALUES ('ABC'); -- ok
-INSERT INTO test11fk VALUES ('xyz'); -- error
-ERROR: insert or update on table "test11fk" violates foreign key constraint "test11fk_x_fkey"
-DETAIL: Key (x)=(xyz) is not present in table "test11pk".
-SELECT * FROM test11pk;
- x
------
- abc
- def
- ghi
-(3 rows)
-
-SELECT * FROM test11fk;
- x
------
- abc
- ABC
-(2 rows)
-
--- cascade update even though the values are "equal" in the PK table
-UPDATE test11pk SET x = 'ABC' WHERE x = 'abc';
-SELECT * FROM test11fk;
- x
------
- ABC
- ABC
-(2 rows)
-
-DELETE FROM test11pk WHERE x = 'abc';
-SELECT * FROM test11pk;
- x
------
- def
- ghi
-(2 rows)
-
-SELECT * FROM test11fk;
- x
----
-(0 rows)
-
+ERROR: foreign key constraint "test11fk_x_fkey" cannot be implemented
+DETAIL: Cannot have indeterministic collations in primary key for referenced table "test11pk".
+HINT: You may need to make primary key constraint's collation deterministic.
+drop table test11pk;
+CREATE TABLE test12pk (x text PRIMARY KEY);
+CREATE TABLE test12fk (x text REFERENCES test12pk on update cascade on delete cascade);
+--alter table change primary key's collation to indeterministic, should fail.
+alter table test12pk alter column x set data type text COLLATE ignore_accent_case;
+ERROR: foreign key constraint "test12fk_x_fkey" cannot be implemented
+DETAIL: Cannot have indeterministic collations in primary key for referenced table "test12pk".
+HINT: You may need to make primary key constraint's collation deterministic.
+alter table test12pk alter column x set data type text_d_ignore_accent_case;
+ERROR: foreign key constraint "test12fk_x_fkey" cannot be implemented
+DETAIL: Cannot have indeterministic collations in primary key for referenced table "test12pk".
+HINT: You may need to make primary key constraint's collation deterministic.
+---should also fail.
+alter table test12pk alter column x set data type text_d_ignore_accent_case COLLATE "C";
+ERROR: foreign key constraint "test12fk_x_fkey" cannot be implemented
+DETAIL: Cannot have indeterministic collations in primary key for referenced table "test12pk".
+HINT: You may need to make primary key constraint's collation deterministic.
+drop table test12pk,test12fk;
+--alter table change foreign key's collation, all these case should fail.
+CREATE TABLE test13pk (x text PRIMARY KEY);
+CREATE TABLE test13fk (x text REFERENCES test13pk on update cascade on delete cascade);
+alter table test13fk alter column x set data type text COLLATE ignore_accent_case;
+ERROR: foreign key constraint "test13fk_x_fkey" cannot be implemented
+DETAIL: Foreign key constraint cannot use indeterministic collation for referencing table "test13fk".
+HINT: You may need to make the foreign key constraint's collation deterministic.
+alter table test13fk alter column x set data type text_d_ignore_accent_case;
+ERROR: foreign key constraint "test13fk_x_fkey" cannot be implemented
+DETAIL: Foreign key constraint cannot use indeterministic collation for referencing table "test13fk".
+HINT: You may need to make the foreign key constraint's collation deterministic.
+alter table test13fk alter column x set data type text_d_ignore_accent_case COLLATE ignore_accent_case;
+ERROR: foreign key constraint "test13fk_x_fkey" cannot be implemented
+DETAIL: Foreign key constraint cannot use indeterministic collation for referencing table "test13fk".
+HINT: You may need to make the foreign key constraint's collation deterministic.
+alter table test13fk alter column x set data type text_d_ignore_accent_case COLLATE "C";
+ERROR: foreign key constraint "test13fk_x_fkey" cannot be implemented
+DETAIL: Foreign key constraint cannot use indeterministic collation for referencing table "test13fk".
+HINT: You may need to make the foreign key constraint's collation deterministic.
+drop table test13pk,test13fk;
-- partitioning
CREATE TABLE test20 (a int, b text COLLATE case_insensitive) PARTITION BY LIST (b);
CREATE TABLE test20_1 PARTITION OF test20 FOR VALUES IN ('abc');
diff --git a/src/test/regress/sql/collate.icu.utf8.sql b/src/test/regress/sql/collate.icu.utf8.sql
index 80f28a97d7..ad418e87eb 100644
--- a/src/test/regress/sql/collate.icu.utf8.sql
+++ b/src/test/regress/sql/collate.icu.utf8.sql
@@ -688,38 +688,46 @@ SELECT * FROM test4 WHERE b = 'Cote' COLLATE ignore_accents; -- still case-sens
SELECT * FROM test4 WHERE b = 'Cote' COLLATE case_insensitive;
-- foreign keys (should use collation of primary key)
+CREATE COLLATION ignore_accent_case (provider = icu, deterministic = false, locale = 'und-u-ks-level1');
+CREATE DOMAIN text_d_ignore_accent_case AS text COLLATE ignore_accent_case;
--- PK is case-sensitive, FK is case-insensitive
+-- PK is case-sensitive (deterministic), FK is case-insensitive (indeterministic).
+-- not OK, multiple PK-side value equal to one FK-side value not allowed
+-- e.g. not allowed case: PK values {'a', 'A"} both equal to FK value {'A'}
CREATE TABLE test10pk (x text COLLATE case_sensitive PRIMARY KEY);
-INSERT INTO test10pk VALUES ('abc'), ('def'), ('ghi');
CREATE TABLE test10fk (x text COLLATE case_insensitive REFERENCES test10pk (x) ON UPDATE CASCADE ON DELETE CASCADE);
-INSERT INTO test10fk VALUES ('abc'); -- ok
-INSERT INTO test10fk VALUES ('ABC'); -- error
-INSERT INTO test10fk VALUES ('xyz'); -- error
-SELECT * FROM test10pk;
-SELECT * FROM test10fk;
--- restrict update even though the values are "equal" in the FK table
-UPDATE test10fk SET x = 'ABC' WHERE x = 'abc'; -- error
-SELECT * FROM test10fk;
-DELETE FROM test10pk WHERE x = 'abc';
-SELECT * FROM test10pk;
-SELECT * FROM test10fk;
--- PK is case-insensitive, FK is case-sensitive
+
+-- PK is case-insensitive (indeterministic), FK is case-sensitive (deterministic)
+-- more than one referencing (FK) row value equal to referenced (PK) row should not allowed
+-- e.g. not allowed case: FK values {'a', 'A"} both equal to PK value {'A'}
CREATE TABLE test11pk (x text COLLATE case_insensitive PRIMARY KEY);
-INSERT INTO test11pk VALUES ('abc'), ('def'), ('ghi');
CREATE TABLE test11fk (x text COLLATE case_sensitive REFERENCES test11pk (x) ON UPDATE CASCADE ON DELETE CASCADE);
-INSERT INTO test11fk VALUES ('abc'); -- ok
-INSERT INTO test11fk VALUES ('ABC'); -- ok
-INSERT INTO test11fk VALUES ('xyz'); -- error
-SELECT * FROM test11pk;
-SELECT * FROM test11fk;
--- cascade update even though the values are "equal" in the PK table
-UPDATE test11pk SET x = 'ABC' WHERE x = 'abc';
-SELECT * FROM test11fk;
-DELETE FROM test11pk WHERE x = 'abc';
-SELECT * FROM test11pk;
-SELECT * FROM test11fk;
+drop table test11pk;
+
+
+
+CREATE TABLE test12pk (x text PRIMARY KEY);
+CREATE TABLE test12fk (x text REFERENCES test12pk on update cascade on delete cascade);
+
+--alter table change primary key's collation to indeterministic, should fail.
+alter table test12pk alter column x set data type text COLLATE ignore_accent_case;
+alter table test12pk alter column x set data type text_d_ignore_accent_case;
+
+---should also fail.
+alter table test12pk alter column x set data type text_d_ignore_accent_case COLLATE "C";
+drop table test12pk,test12fk;
+
+
+
+--alter table change foreign key's collation, all these case should fail.
+CREATE TABLE test13pk (x text PRIMARY KEY);
+CREATE TABLE test13fk (x text REFERENCES test13pk on update cascade on delete cascade);
+alter table test13fk alter column x set data type text COLLATE ignore_accent_case;
+alter table test13fk alter column x set data type text_d_ignore_accent_case;
+alter table test13fk alter column x set data type text_d_ignore_accent_case COLLATE ignore_accent_case;
+alter table test13fk alter column x set data type text_d_ignore_accent_case COLLATE "C";
+drop table test13pk,test13fk;
-- partitioning
CREATE TABLE test20 (a int, b text COLLATE case_insensitive) PARTITION BY LIST (b);
base-commit: 31a98934d1699007b50aefaedd68ab4d6b42e3b4
--
2.34.1
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Per-table resync for logical replication subscriptions
@ 2026-07-10 10:38 Cagri Biroglu <[email protected]>
0 siblings, 0 replies; 9+ messages in thread
From: Cagri Biroglu @ 2026-07-10 10:38 UTC (permalink / raw)
To: Masahiko Sawada <[email protected]>; [email protected] <[email protected]>; +Cc: [email protected]
>
> Thanks a lot for looking at this.
>
> Before the point-by-point: one change after getting feedback on this
from the sketch in my first mail
is that I dropped the WITH (copy_data, truncate) options. For refreshing
> a table that is already subscribed, only one combination is useful, so
> REFRESH TABLE now always truncates the local copy and re-copies (what
> would have been copy_data = true, truncate = true). Without truncating,
> the re-copy hits duplicate-key errors or doubles the rows; skipping the
> copy leaves the drift in place. So neither other combination is
> meaningful, and there is nothing to configure.
>
> > REFRESH TABLE is better to me. It's more consistent with the existing
> > commands such as REFRESH SEQUENCES.
>
> Great, I've kept REFRESH TABLE.
>
> > I think it might be a good idea that the REFRESH TABLE command can
> > take multiple tables rather than one table.
>
> Agreed. v2 makes REFRESH TABLE take a comma-separated list:
>
> ALTER SUBSCRIPTION name REFRESH TABLE t1, t2, ...
>
> It's all-or-nothing: every named relation is validated (part of the
> subscription, not a sequence, exists) before any of them is reset, so a
> bad table name aborts the whole command without touching the others. The
> listed tables are also truncated together, which lets a set connected by
> foreign keys be re-seeded as a unit.
>
> One note on scope: this doesn't add resource risk over doing one table
> at a time, because the re-copy is still performed by tablesync workers
> that are capped by max_sync_workers_per_subscription; resetting N tables
> just queues them rather than starting N copies at once.
>
> > Why does the REFRESH TABLE command require for the subscription to be
> > disabled while REFRESH PUBLICATIONS doesn't?
>
> Good question, but I'd actually argue the disabled requirement is a
> reasonable design choice here rather than a limitation to remove.
>
> The reason the two commands differ is that REFRESH PUBLICATION only adds
> *new* relations to pg_subscription_rel (in init state), whereas REFRESH
> TABLE resets a relation that is already in ready state, and those hit
> different caches in the apply worker:
>
> - The set of not-ready relations is cached (table_states_not_ready in
> syncutils.c) and correctly invalidated by the SUBSCRIPTIONRELMAP
> syscache callback (InvalidateSyncingRelStates). So after the reset the
> apply worker does notice the table is non-ready and does launch a
> tablesync worker for it. That part works whether enabled or not.
>
> - However, whether the apply worker applies incoming changes for a
> relation is decided by should_apply_changes_for_rel(), which reads the
> per-relation LogicalRepRelMapEntry.state. That state is only refreshed
> from the catalog while it is not READY:
>
> /* relation.c, logicalrep_rel_open() */
> if (entry->state != SUBREL_STATE_READY)
> entry->state = GetSubscriptionRelState(...);
>
> For an already-ready table the entry stays READY, and the relcache
> invalidation only clears localrelvalid (rebuilds the attrmap); it does
> not reset entry->state. So the running apply worker keeps treating the
> table as READY and keeps applying live changes to it, at the same time
> as the freshly launched tablesync worker re-copies it, with no handoff
> on the sync LSN. In my testing that reliably diverges the table.
>
> For a newly added table (REFRESH PUBLICATION) there is no stale READY
> entry, so the normal init -> datasync -> syncdone -> ready handoff works
> without any disable.
>
> Requiring the subscription to be disabled sidesteps this cleanly. When
> you re-enable, a fresh apply worker starts and reads the reset table's
> state from the catalog as init, so the same init -> datasync -> syncdone
> -> ready handoff kicks in, exactly as for a newly added table. And the
> disable only needs to bracket the quick catalog reset, not the re-copy
> itself: REFRESH TABLE just truncates and flips the state, and after
> ENABLE the tablesync worker does the (possibly long) copy while the
> apply worker resumes the other tables as usual, so the pause is short
> and scoped to the metadata change, not the data copy.
>
> Supporting the enabled case would instead mean reaching into the running
> apply worker to force it to drop and re-read the cached state of the
> reset relation mid-flight. That is doable, but it adds moving parts and
> a new invalidation path to get right, for what is a deliberate,
> occasional maintenance action where briefly disabling the subscription
> does no harm. So my inclination is to keep the disabled requirement here
> rather than trade that simplicity away. Happy to reconsider if you think
> the enabled case matters enough to justify the extra machinery or i am
> missing sth.
Regards,
> Cagri Biroglu
>
Attachments:
[application/octet-stream] v2-0001-refresh-table.patch (19.3K, ../../CAA36msq=JYBN+zm2MoKqXZCSjMhhdmhYwRWVZnhPKdE8zaUGFA@mail.gmail.com/3-v2-0001-refresh-table.patch)
download | inline diff:
From fa82f9f0ece03430671273905073f3364e9ee6d4 Mon Sep 17 00:00:00 2001
From: cagrib <[email protected]>
Date: Tue, 7 Jul 2026 14:17:19 +0200
Subject: [PATCH v2] Add ALTER SUBSCRIPTION ... REFRESH TABLE to resync tables
REFRESH PUBLICATION only starts syncing tables that were newly added to
the publication; it never re-copies a table that is already part of the
subscription. Today the only ways to re-seed an already-subscribed table
are to churn publication membership (which is not local to one subscriber
and can disturb sibling subscribers or cascade downstream) or to hand-edit
pg_subscription_rel (unsupported and racy against the apply worker).
Add a subscriber-local command:
ALTER SUBSCRIPTION name REFRESH TABLE table_name [, ...]
It truncates the local copy of each named table and resets its state in
pg_subscription_rel back to init so that a tablesync worker re-copies just
those tables, reusing the existing table synchronization machinery. All
named relations are validated before any is reset, so the command is
all-or-nothing, and they are truncated together so a set connected by
foreign keys can be re-seeded as a unit. Publication membership and other
tables are left untouched. This mirrors REFRESH SEQUENCES, which likewise
re-synchronizes objects already known to the subscription rather than only
discovering newly added ones.
This first version requires the subscription to be disabled, which avoids
racing the apply worker's cached relation state.
A TAP test is included.
---
src/backend/commands/subscriptioncmds.c | 187 +++++++++++++++
src/backend/parser/gram.y | 10 +
src/include/nodes/parsenodes.h | 2 +
src/test/subscription/t/039_refresh_table.pl | 228 +++++++++++++++++++
4 files changed, 427 insertions(+)
create mode 100644 src/test/subscription/t/039_refresh_table.pl
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 4292e7fb8f4..dd6096d5843 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1405,6 +1405,170 @@ AlterSubscription_refresh_seq(Subscription *sub)
PG_END_TRY();
}
+/*
+ * Resynchronize one or more already-subscribed tables.
+ *
+ * Truncates the local copy of each named table and resets its
+ * pg_subscription_rel state back to init so that, once the subscription is
+ * enabled, a tablesync worker re-copies just those tables. This is
+ * subscriber-local and does not touch publication membership, so sibling
+ * subscribers are unaffected.
+ *
+ * Every named relation is validated before any of them is reset, so the
+ * command is all-or-nothing: a bad table name aborts the whole command
+ * without touching the others. The tables are truncated together, which lets
+ * a set connected by foreign keys be re-seeded as a unit.
+ *
+ * The caller must ensure the subscription is disabled: with no apply worker
+ * running there is no cached relation state to invalidate and no race against
+ * a concurrently launched tablesync worker.
+ */
+static void
+AlterSubscription_refresh_table(Subscription *sub, List *relations)
+{
+ Relation rel;
+ ListCell *lc;
+ List *relids = NIL;
+ List *truncrels = NIL;
+ TruncateStmt *tstmt;
+
+ /*
+ * Lock pg_subscription_rel with AccessExclusiveLock, matching
+ * AlterSubscription_refresh(), so the relation states cannot change under
+ * us for the duration of the command.
+ */
+ rel = table_open(SubscriptionRelRelationId, AccessExclusiveLock);
+
+ /*
+ * First pass: validate every named relation before changing anything, so
+ * the command is all-or-nothing. Duplicates in the list are ignored.
+ */
+ foreach(lc, relations)
+ {
+ RangeVar *rv = lfirst_node(RangeVar, lc);
+ Oid relid;
+ char relstate;
+ XLogRecPtr relstatelsn;
+
+ relid = RangeVarGetRelid(rv, AccessShareLock, false);
+
+ if (get_rel_relkind(relid) == RELKIND_SEQUENCE)
+ ereport(ERROR,
+ errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot refresh sequence \"%s\" as a table",
+ rv->relname),
+ errhint("Use ALTER SUBSCRIPTION ... REFRESH SEQUENCES instead."));
+
+ /* The relation must already be part of the subscription. */
+ relstate = GetSubscriptionRelState(sub->oid, relid, &relstatelsn);
+ if (relstate == SUBREL_STATE_UNKNOWN)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("table \"%s\" is not part of the subscription \"%s\"",
+ rv->relname, sub->name));
+
+ if (list_member_oid(relids, relid))
+ continue;
+
+ relids = lappend_oid(relids, relid);
+ truncrels = lappend(truncrels, rv);
+ }
+
+ /*
+ * Second pass: stop any leftover tablesync worker for each relation and,
+ * if it was caught mid-sync, clean up its origin and publisher slot.
+ */
+ foreach_oid(relid, relids)
+ {
+ char relstate;
+ XLogRecPtr relstatelsn;
+
+ relstate = GetSubscriptionRelState(sub->oid, relid, &relstatelsn);
+
+ /* Stop any leftover tablesync worker for this relation. */
+ logicalrep_worker_stop(WORKERTYPE_TABLESYNC, sub->oid, relid);
+
+ /*
+ * If the relation was caught mid-sync, drop its tablesync origin. For
+ * READY state the tablesync worker already dropped it, so nothing is
+ * needed there. Pass missing_ok = true as the origin may not exist
+ * yet.
+ */
+ if (relstate != SUBREL_STATE_READY)
+ {
+ char originname[NAMEDATALEN];
+
+ ReplicationOriginNameForLogicalRep(sub->oid, relid, originname,
+ sizeof(originname));
+ replorigin_drop_by_name(originname, true, false);
+ }
+
+ /*
+ * Likewise drop the tablesync slot on the publisher for a mid-sync
+ * relation. READY/SYNCDONE relations have no such slot, so the common
+ * case needs no publisher connection at all.
+ */
+ if (relstate != SUBREL_STATE_READY && relstate != SUBREL_STATE_SYNCDONE)
+ {
+ char syncslotname[NAMEDATALEN] = {0};
+ char *err = NULL;
+ WalReceiverConn *wrconn;
+ bool must_use_password;
+
+ load_file("libpqwalreceiver", false);
+
+ must_use_password = sub->passwordrequired && !sub->ownersuperuser;
+ wrconn = walrcv_connect(sub->conninfo, true, true, must_use_password,
+ sub->name, &err);
+ if (!wrconn)
+ ereport(ERROR,
+ errcode(ERRCODE_CONNECTION_FAILURE),
+ errmsg("subscription \"%s\" could not connect to the publisher: %s",
+ sub->name, err));
+
+ PG_TRY();
+ {
+ ReplicationSlotNameForTablesync(sub->oid, relid, syncslotname,
+ sizeof(syncslotname));
+ ReplicationSlotDropAtPubNode(wrconn, syncslotname, true);
+ }
+ PG_FINALLY();
+ {
+ walrcv_disconnect(wrconn);
+ }
+ PG_END_TRY();
+ }
+ }
+
+ /*
+ * Clear the local copies so the re-copy starts from empty. Truncating the
+ * tables together lets a set connected by foreign keys be re-seeded as a
+ * unit.
+ */
+ tstmt = makeNode(TruncateStmt);
+ tstmt->relations = truncrels;
+ tstmt->restart_seqs = false;
+ tstmt->behavior = DROP_RESTRICT;
+ ExecuteTruncate(tstmt);
+
+ /*
+ * Reset each relation to init so that a tablesync worker re-copies it once
+ * the subscription is enabled.
+ */
+ foreach_oid(relid, relids)
+ {
+ UpdateSubscriptionRelState(sub->oid, relid, SUBREL_STATE_INIT,
+ InvalidXLogRecPtr, false);
+
+ ereport(DEBUG1,
+ errmsg_internal("table \"%s.%s\" of subscription \"%s\" reset for resync",
+ get_namespace_name(get_rel_namespace(relid)),
+ get_rel_name(relid), sub->name));
+ }
+
+ table_close(rel, NoLock);
+}
+
/*
* Common checks for altering failover, two_phase, and retain_dead_tuples
* options.
@@ -2296,6 +2460,29 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
break;
}
+ case ALTER_SUBSCRIPTION_REFRESH_TABLE:
+ {
+ /*
+ * The first version requires the subscription to be disabled.
+ * With no apply worker running there is no cached relation
+ * state to invalidate and no race against a concurrently
+ * launched tablesync worker while we reset the relation.
+ */
+ if (sub->enabled)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("%s is not allowed for enabled subscriptions",
+ "ALTER SUBSCRIPTION ... REFRESH TABLE"),
+ errhint("Disable the subscription with ALTER SUBSCRIPTION ... DISABLE first."));
+
+ PreventInTransactionBlock(isTopLevel,
+ "ALTER SUBSCRIPTION ... REFRESH TABLE");
+
+ AlterSubscription_refresh_table(sub, stmt->relations);
+
+ break;
+ }
+
case ALTER_SUBSCRIPTION_SKIP:
{
/* ALTER SUBSCRIPTION ... SKIP supports only LSN option */
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index ff4e1388c55..82716c46947 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -11617,6 +11617,16 @@ AlterSubscriptionStmt:
n->subname = $3;
$$ = (Node *) n;
}
+ | ALTER SUBSCRIPTION name REFRESH TABLE qualified_name_list
+ {
+ AlterSubscriptionStmt *n =
+ makeNode(AlterSubscriptionStmt);
+
+ n->kind = ALTER_SUBSCRIPTION_REFRESH_TABLE;
+ n->subname = $3;
+ n->relations = $6;
+ $$ = (Node *) n;
+ }
| ALTER SUBSCRIPTION name ADD_P PUBLICATION name_list opt_definition
{
AlterSubscriptionStmt *n =
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index e03556399ab..cfeb09ab417 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -4566,6 +4566,7 @@ typedef enum AlterSubscriptionType
ALTER_SUBSCRIPTION_DROP_PUBLICATION,
ALTER_SUBSCRIPTION_REFRESH_PUBLICATION,
ALTER_SUBSCRIPTION_REFRESH_SEQUENCES,
+ ALTER_SUBSCRIPTION_REFRESH_TABLE,
ALTER_SUBSCRIPTION_ENABLED,
ALTER_SUBSCRIPTION_SKIP,
} AlterSubscriptionType;
@@ -4578,6 +4579,7 @@ typedef struct AlterSubscriptionStmt
char *servername; /* Server name of publisher */
char *conninfo; /* Connection string to publisher */
List *publication; /* One or more publication to subscribe to */
+ List *relations; /* Tables to resync (for REFRESH TABLE) */
List *options; /* List of DefElem nodes */
} AlterSubscriptionStmt;
diff --git a/src/test/subscription/t/039_refresh_table.pl b/src/test/subscription/t/039_refresh_table.pl
new file mode 100644
index 00000000000..b09f1e149bf
--- /dev/null
+++ b/src/test/subscription/t/039_refresh_table.pl
@@ -0,0 +1,228 @@
+
+# Copyright (c) 2021-2026, PostgreSQL Global Development Group
+
+# Tests for ALTER SUBSCRIPTION ... REFRESH TABLE, which re-copies one or more
+# already-subscribed tables on the subscriber without touching publication
+# membership or other tables. The first version requires the subscription
+# to be disabled.
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# Initialize publisher and subscriber nodes
+my $node_publisher = PostgreSQL::Test::Cluster->new('publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->start;
+
+my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
+$node_subscriber->init;
+$node_subscriber->append_conf('postgresql.conf',
+ "wal_retrieve_retry_interval = 1ms");
+$node_subscriber->start;
+
+my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+
+# Preexisting content on the publisher: two published tables.
+$node_publisher->safe_psql(
+ 'postgres', qq(
+ CREATE TABLE tab_res (a int primary key, b text);
+ CREATE TABLE tab_other (a int primary key, b text);
+ INSERT INTO tab_res SELECT g, 'p' || g FROM generate_series(1, 100) g;
+ INSERT INTO tab_other SELECT g, 'q' || g FROM generate_series(1, 100) g;
+ CREATE PUBLICATION tap_pub FOR TABLE tab_res, tab_other;
+));
+
+# Matching structure on the subscriber, plus objects used for error cases.
+$node_subscriber->safe_psql(
+ 'postgres', qq(
+ CREATE TABLE tab_res (a int primary key, b text);
+ CREATE TABLE tab_other (a int primary key, b text);
+ CREATE TABLE tab_local (a int primary key);
+ CREATE SEQUENCE seq_local;
+));
+
+$node_subscriber->safe_psql('postgres',
+ "CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr' PUBLICATION tap_pub"
+);
+
+# Wait for initial sync of both tables.
+$node_subscriber->wait_for_subscription_sync($node_publisher, 'tap_sub');
+
+is( $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM tab_res"),
+ '100', 'initial sync of tab_res');
+is( $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM tab_other"),
+ '100', 'initial sync of tab_other');
+
+# A small helper: run SQL expected to fail, and check the error message.
+sub refresh_should_fail
+{
+ my ($sql, $pattern, $desc) = @_;
+ my ($ret, $stdout, $stderr) = ('', '', '');
+ $ret = $node_subscriber->psql('postgres', $sql,
+ stdout => \$stdout, stderr => \$stderr);
+ ok($ret != 0 && $stderr =~ /$pattern/,
+ $desc)
+ or diag("got ret=$ret stderr=$stderr");
+}
+
+# REFRESH TABLE is not allowed while the subscription is enabled.
+refresh_should_fail(
+ "ALTER SUBSCRIPTION tap_sub REFRESH TABLE tab_res",
+ qr/not allowed for enabled subscriptions/,
+ 'REFRESH TABLE rejected while enabled');
+
+# Disable the subscription and wait for its workers to stop.
+$node_subscriber->safe_psql('postgres',
+ "ALTER SUBSCRIPTION tap_sub DISABLE");
+$node_subscriber->poll_query_until('postgres',
+ "SELECT count(*) = 0 FROM pg_stat_subscription WHERE subname = 'tap_sub' AND pid IS NOT NULL"
+) or die "Timed out waiting for subscription workers to stop";
+
+# A table that is not part of the subscription is rejected.
+refresh_should_fail(
+ "ALTER SUBSCRIPTION tap_sub REFRESH TABLE tab_local",
+ qr/is not part of the subscription/,
+ 'REFRESH TABLE rejected for table not in subscription');
+
+# A sequence cannot be refreshed as a table.
+refresh_should_fail(
+ "ALTER SUBSCRIPTION tap_sub REFRESH TABLE seq_local",
+ qr/cannot refresh sequence/,
+ 'REFRESH TABLE rejected for a sequence');
+
+# The command cannot run inside a transaction block.
+refresh_should_fail(
+ "BEGIN; ALTER SUBSCRIPTION tap_sub REFRESH TABLE tab_res;",
+ qr/cannot run inside a transaction block/,
+ 'REFRESH TABLE rejected inside a transaction block');
+
+# All-or-nothing: a list containing one bad table aborts the whole command and
+# leaves the valid table untouched.
+refresh_should_fail(
+ "ALTER SUBSCRIPTION tap_sub REFRESH TABLE tab_res, tab_local",
+ qr/is not part of the subscription/,
+ 'REFRESH TABLE with a bad table in the list is rejected');
+is( $node_subscriber->safe_psql(
+ 'postgres',
+ "SELECT srsubstate FROM pg_subscription_rel r JOIN pg_class c ON c.oid = r.srrelid WHERE c.relname = 'tab_res'"
+ ),
+ 'r',
+ 'valid table not reset when another table in the list is invalid');
+
+# Introduce drift on the subscriber, only in tab_res.
+$node_subscriber->safe_psql(
+ 'postgres', qq(
+ DELETE FROM tab_res WHERE a <= 40;
+ UPDATE tab_res SET b = 'CORRUPT' WHERE a = 60;
+));
+is( $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM tab_res"),
+ '60', 'drift introduced in tab_res');
+
+# Record the pre-refresh state of the other table.
+my $other_state_before = $node_subscriber->safe_psql('postgres',
+ "SELECT srsubstate FROM pg_subscription_rel r JOIN pg_class c ON c.oid = r.srrelid WHERE c.relname = 'tab_other'"
+);
+is($other_state_before, 'r', 'tab_other is ready before refresh');
+
+# Resync just tab_res.
+$node_subscriber->safe_psql('postgres',
+ "ALTER SUBSCRIPTION tap_sub REFRESH TABLE tab_res");
+
+# Only tab_res is reset to init; tab_other is untouched; local copy truncated.
+is( $node_subscriber->safe_psql(
+ 'postgres',
+ "SELECT srsubstate FROM pg_subscription_rel r JOIN pg_class c ON c.oid = r.srrelid WHERE c.relname = 'tab_res'"
+ ),
+ 'i',
+ 'tab_res reset to init state');
+is( $node_subscriber->safe_psql(
+ 'postgres',
+ "SELECT srsubstate FROM pg_subscription_rel r JOIN pg_class c ON c.oid = r.srrelid WHERE c.relname = 'tab_other'"
+ ),
+ 'r',
+ 'tab_other left untouched (still ready)');
+is( $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM tab_res"),
+ '0', 'tab_res truncated locally by REFRESH while disabled');
+
+# Re-enable and wait for the single table to re-copy.
+$node_subscriber->safe_psql('postgres',
+ "ALTER SUBSCRIPTION tap_sub ENABLE");
+$node_subscriber->wait_for_subscription_sync($node_publisher, 'tap_sub');
+
+is( $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM tab_res"),
+ '100', 'tab_res re-copied after enable');
+is( $node_subscriber->safe_psql('postgres',
+ "SELECT b FROM tab_res WHERE a = 60"),
+ 'p60', 'tab_res corruption repaired by resync');
+
+# Full content matches the publisher.
+my $pub_md5 = $node_publisher->safe_psql('postgres',
+ "SELECT md5(string_agg(a || ':' || b, ',' ORDER BY a)) FROM tab_res");
+my $sub_md5 = $node_subscriber->safe_psql('postgres',
+ "SELECT md5(string_agg(a || ':' || b, ',' ORDER BY a)) FROM tab_res");
+is($sub_md5, $pub_md5, 'tab_res matches publisher after resync');
+
+# tab_other was never disturbed.
+is( $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM tab_other"),
+ '100', 'tab_other intact throughout');
+
+# Ongoing replication still works for the resynced table.
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_res VALUES (101, 'p101')");
+$node_publisher->wait_for_catchup('tap_sub');
+is( $node_subscriber->safe_psql('postgres',
+ "SELECT count(*) FROM tab_res WHERE a = 101"),
+ '1', 'streaming resumes on resynced table');
+
+# Multiple tables can be resynced in a single command. Disable, drift both
+# tables, and refresh them together.
+$node_subscriber->safe_psql('postgres',
+ "ALTER SUBSCRIPTION tap_sub DISABLE");
+$node_subscriber->poll_query_until('postgres',
+ "SELECT count(*) = 0 FROM pg_stat_subscription WHERE subname = 'tap_sub' AND pid IS NOT NULL"
+) or die "Timed out waiting for subscription workers to stop";
+
+$node_subscriber->safe_psql(
+ 'postgres', qq(
+ DELETE FROM tab_res WHERE a <= 20;
+ DELETE FROM tab_other WHERE a <= 20;
+));
+
+$node_subscriber->safe_psql('postgres',
+ "ALTER SUBSCRIPTION tap_sub REFRESH TABLE tab_res, tab_other");
+
+# Both listed tables are reset to init and truncated locally.
+is( $node_subscriber->safe_psql(
+ 'postgres',
+ "SELECT string_agg(srsubstate, ',' ORDER BY c.relname) FROM pg_subscription_rel r JOIN pg_class c ON c.oid = r.srrelid WHERE c.relname IN ('tab_other', 'tab_res')"
+ ),
+ 'i,i',
+ 'both listed tables reset to init state');
+is( $node_subscriber->safe_psql('postgres',
+ "SELECT (SELECT count(*) FROM tab_res) + (SELECT count(*) FROM tab_other)"),
+ '0', 'both listed tables truncated locally');
+
+# Re-enable and wait for both tables to re-copy.
+$node_subscriber->safe_psql('postgres',
+ "ALTER SUBSCRIPTION tap_sub ENABLE");
+$node_subscriber->wait_for_subscription_sync($node_publisher, 'tap_sub');
+
+my $pub_res = $node_publisher->safe_psql('postgres',
+ "SELECT md5(string_agg(a || ':' || b, ',' ORDER BY a)) FROM tab_res");
+my $sub_res = $node_subscriber->safe_psql('postgres',
+ "SELECT md5(string_agg(a || ':' || b, ',' ORDER BY a)) FROM tab_res");
+is($sub_res, $pub_res, 'tab_res matches publisher after multi-table resync');
+
+my $pub_oth = $node_publisher->safe_psql('postgres',
+ "SELECT md5(string_agg(a || ':' || b, ',' ORDER BY a)) FROM tab_other");
+my $sub_oth = $node_subscriber->safe_psql('postgres',
+ "SELECT md5(string_agg(a || ':' || b, ',' ORDER BY a)) FROM tab_other");
+is($sub_oth, $pub_oth, 'tab_other matches publisher after multi-table resync');
+
+$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION tap_sub");
+$node_subscriber->stop('fast');
+$node_publisher->stop('fast');
+
+done_testing();
--
2.54.0
^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2026-07-10 10:38 UTC | newest]
Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-04-13 13:13 Re: altering a column's collation leaves an invalid foreign key jian he <[email protected]>
2024-06-07 06:39 ` jian he <[email protected]>
2024-06-07 20:12 ` Tom Lane <[email protected]>
2024-06-08 04:14 ` jian he <[email protected]>
2024-06-18 08:50 ` Peter Eisentraut <[email protected]>
2024-07-16 08:29 ` jian he <[email protected]>
2024-09-03 09:41 ` Peter Eisentraut <[email protected]>
2024-09-04 06:54 ` jian he <[email protected]>
2026-07-10 10:38 Re: Per-table resync for logical replication subscriptions Cagri Biroglu <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox