Here is a rebased patch
this time I did not indent the part under
if(SessionReplicationRole != SESSION_REPLICATION_ROLE_REPLICA)
{
... (<did not add indent>
}
so it is immediately obviuos from the patch what is added.
I can add the indent later, or just let pg_ident take care of this in due time
On Sat, May 24, 2025 at 4:02 PM Hannu Krosing <han...@google.com> wrote:
>
> I would also argue for treating this as a bug and back-porting to all
> supported versions - a quick look at v13 seems to confirm that the
> wrapped code has not changed at least since then.
>
> I don't think we can claim the current state is Working As Intended
> unless someone stands up and says they really intended it to work this
> way :)
>
> On Sat, May 24, 2025 at 3:47 PM Hannu Krosing <han...@google.com> wrote:
> >
> > Hello Everybody,
> >
> > Currently setting `session_replication_role` to `replica` disables
> > foreign key checks allowing, among other things, free table copy order
> > and faster CDC apply in logical replication.
> >
> > But two other cases of foreign keys are still restricted or blocked
> > even with this setting
> >
> >
> > 1. Foreign Key checks during `ALTER TABLE <fkt> ADD FOREIGN KEY
> > (<fkcol>) REFERENCES <pkt>(<pkcol>);`
> >
> > These can be really, Really, REALLY slow when the PK table is huge
> > (hundreds of billions of rows). And here I mean taking-tens-of-days
> > slow in extreme cases.
> >
> > And they are also completely unnecessary when the table data comes
> > from a known good source.
> >
> >
> > 2. `TRUNCATE <referenced table>` is blocked when there are any foreign
> > keys referencing the <referenced table>
> >
> > But you still can mess up your database in exactly the same way by
> > running `DELETE FROM <referenced table>`, just a little (or a lot)
> > slower.
> >
> >
> > The attached patch fixes this, allowing both cases to work when `SET
> > session_replication_role=replica;` is in force:
> >
> > ### Test for `ALTER TABLE <fkt> ADD FOREIGN KEY (<fkcol>) REFERENCES
> > <pkt>(<pkcol>);`
> >
> > CREATE TABLE pkt(id int PRIMARY KEY);
> > CREATE TABLE fkt(fk int);
> > INSERT INTO fkt VALUES(1);
> >
> > ALTER TABLE fkt ADD FOREIGN KEY (fk) REFERENCES pkt(id); -- fails
> >
> > SET session_replication_role=replica;
> > ALTER TABLE fkt ADD FOREIGN KEY (fk) REFERENCES pkt(id); -- now succeeds
> >
> > ### Test for `TRUNCATE <referenced table>`
> >
> > CREATE TABLE pkt(id int PRIMARY KEY);
> > CREATE TABLE fkt(fk int REFERENCES pkt(id));
> >
> > TRUNCATE pkt; -- fails
> >
> > SET session_replication_role=replica;
> > TRUNCATE pkt; -- now succeeds
> >
> >
> >
> > The patch just wraps two sections of code in
> >
> > if (SessionReplicationRole != SESSION_REPLICATION_ROLE_REPLICA) {
> > ...
> > }
> >
> > and the rest is added indentation for the wrapped.
> >
> > Plus I added tests, including the until now missing test for
> > explicitly the foreign key checks (which can be argued to already be
> > covered by generic trigger tests)
> >
> >
> > I am not sure if we need to add anything to current documentation[*]
> > which says just this about foreign keys and
> > `session_replication_role=replica`
> >
> > > Since foreign keys are implemented as triggers, setting this parameter to
> > > replica also disables all foreign key checks, which can leave data in an
> > > inconsistent state if improperly used.
> >
> > [*]
> > https://www.postgresql.org/docs/17/runtime-config-client.html#GUC-SESSION-REPLICATION-ROLE
> >
> > Any comments and suggestions are welcome
From 2191e2beec99fc6ca767d248e9adcc38d0339509 Mon Sep 17 00:00:00 2001
From: Hannu Krosing <han...@google.com>
Date: Sun, 6 Jul 2025 13:39:04 +0200
Subject: [PATCH v2] rebased, also did not indent the contents of if to make it
clearer what is added
---
src/backend/commands/tablecmds.c | 18 ++++++--
src/test/regress/expected/foreign_key.out | 35 ++++++++++++++++
src/test/regress/sql/foreign_key.sql | 51 +++++++++++++++++++++++
3 files changed, 101 insertions(+), 3 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index cb811520c29..bdd786b4b43 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2035,13 +2035,19 @@ ExecuteTruncateGuts(List *explicit_rels,
* Check foreign key references. In CASCADE mode, this should be
* unnecessary since we just pulled in all the references; but as a
* cross-check, do it anyway if in an Assert-enabled build.
+ *
+ * Skip foreign key checks when `session_replication_role = replica` to
+ * match the behaviour of disabling FK triggers in the same situation
*/
+ if (SessionReplicationRole != SESSION_REPLICATION_ROLE_REPLICA)
+ {
#ifdef USE_ASSERT_CHECKING
- heap_truncate_check_FKs(rels, false);
-#else
- if (behavior == DROP_RESTRICT)
heap_truncate_check_FKs(rels, false);
+#else
+ if (behavior == DROP_RESTRICT)
+ heap_truncate_check_FKs(rels, false);
#endif
+ }
/*
* If we are asked to restart sequences, find all the sequences, lock them
@@ -6043,7 +6049,12 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
* theoretically possible that we have changed both relations of the
* foreign key, and we'd better have finished both rewrites before we try
* to read the tables.
+ *
+ * Skip the check when `session_replication_mode = replica` to save time
+ * and to match the FK trigger behaviour in the same situation
*/
++ if (SessionReplicationRole != SESSION_REPLICATION_ROLE_REPLICA)
+ {
foreach(ltab, *wqueue)
{
AlteredTableInfo *tab = (AlteredTableInfo *) lfirst(ltab);
@@ -6088,6 +6099,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
if (rel)
table_close(rel, NoLock);
}
+ }
/* Finally, run any afterStmts that were queued up */
foreach(ltab, *wqueue)
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index f9bd252444f..32f39e8a57d 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -3402,6 +3402,41 @@ ALTER TABLE fk_r DROP CONSTRAINT fk_r_p_id_p_jd_fkey_1;
ERROR: cannot drop inherited constraint "fk_r_p_id_p_jd_fkey_1" of relation "fk_r"
ALTER TABLE fk_r_2 DROP CONSTRAINT fk_r_p_id_p_jd_fkey;
ERROR: cannot drop inherited constraint "fk_r_p_id_p_jd_fkey" of relation "fk_r_2"
+-- tests for SET session_replication_role = replica;
+RESET session_replication_role;
+-- disabling FK checks
+CREATE TABLE pkt(id int PRIMARY KEY);
+CREATE TABLE fkt(fk int REFERENCES pkt(id));
+INSERT INTO fkt VALUES(1); -- should fail
+ERROR: insert or update on table "fkt" violates foreign key constraint "fkt_fk_fkey"
+DETAIL: Key (fk)=(1) is not present in table "pkt".
+SET session_replication_role=replica;
+INSERT INTO fkt VALUES(1); -- should succeed now
+DROP TABLE fkt, pkt;
+RESET session_replication_role;
+-- skipping FK validation during ALTER TABLE ... ADD FOREIGN KEY
+CREATE TABLE pkt(id int PRIMARY KEY);
+CREATE TABLE fkt(fk int);
+INSERT INTO fkt VALUES(1);
+ALTER TABLE fkt ADD FOREIGN KEY (fk) REFERENCES pkt(id); -- should fail
+ERROR: insert or update on table "fkt" violates foreign key constraint "fkt_fk_fkey"
+DETAIL: Key (fk)=(1) is not present in table "pkt".
+SET session_replication_role=replica;
+ALTER TABLE fkt ADD FOREIGN KEY (fk) REFERENCES pkt(id); -- should succeed now
+DROP TABLE fkt, pkt;
+RESET session_replication_role;
+-- skipping FK existence checks during TRUNCATE
+CREATE TABLE pkt(id int PRIMARY KEY);
+CREATE TABLE fkt(fk int REFERENCES pkt(id));
+TRUNCATE pkt; -- should fail
+ERROR: cannot truncate a table referenced in a foreign key constraint
+DETAIL: Table "fkt" references "pkt".
+HINT: Truncate table "fkt" at the same time, or use TRUNCATE ... CASCADE.
+SET session_replication_role=replica;
+TRUNCATE pkt; -- should succeed now
+DROP TABLE fkt, pkt;
+RESET session_replication_role;
+-- end of tests for SET session_replication_role = replica;
SET client_min_messages TO warning;
DROP SCHEMA fkpart12 CASCADE;
RESET client_min_messages;
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index cfcecb4e911..d826694947f 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -1208,6 +1208,10 @@ commit;
drop table pktable2, fktable2;
+--
+--
+--
+
--
-- Test keys that "look" different but compare as equal
--
@@ -2382,7 +2386,54 @@ ALTER TABLE fk_r_1 DROP CONSTRAINT fk_r_p_id_p_jd_fkey;
ALTER TABLE fk_r DROP CONSTRAINT fk_r_p_id_p_jd_fkey_1;
ALTER TABLE fk_r_2 DROP CONSTRAINT fk_r_p_id_p_jd_fkey;
+-- tests for SET session_replication_role = replica;
+
+RESET session_replication_role;
+
+-- disabling FK checks
+
+CREATE TABLE pkt(id int PRIMARY KEY);
+CREATE TABLE fkt(fk int REFERENCES pkt(id));
+
+INSERT INTO fkt VALUES(1); -- should fail
+
+SET session_replication_role=replica;
+INSERT INTO fkt VALUES(1); -- should succeed now
+
+DROP TABLE fkt, pkt;
+RESET session_replication_role;
+
+-- skipping FK validation during ALTER TABLE ... ADD FOREIGN KEY
+
+CREATE TABLE pkt(id int PRIMARY KEY);
+CREATE TABLE fkt(fk int);
+INSERT INTO fkt VALUES(1);
+
+ALTER TABLE fkt ADD FOREIGN KEY (fk) REFERENCES pkt(id); -- should fail
+
+SET session_replication_role=replica;
+ALTER TABLE fkt ADD FOREIGN KEY (fk) REFERENCES pkt(id); -- should succeed now
+
+DROP TABLE fkt, pkt;
+RESET session_replication_role;
+
+-- skipping FK existence checks during TRUNCATE
+
+CREATE TABLE pkt(id int PRIMARY KEY);
+CREATE TABLE fkt(fk int REFERENCES pkt(id));
+
+TRUNCATE pkt; -- should fail
+
+SET session_replication_role=replica;
+TRUNCATE pkt; -- should succeed now
+
+DROP TABLE fkt, pkt;
+RESET session_replication_role;
+
+-- end of tests for SET session_replication_role = replica;
+
SET client_min_messages TO warning;
DROP SCHEMA fkpart12 CASCADE;
RESET client_min_messages;
RESET search_path;
+
--
2.50.0.727.gbf7dc18ff4-goog
