| From: | Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com> |
|---|---|
| To: | Hannu Krosing <hannuk(at)google(dot)com> |
| Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: [PATCH] Extending FK check skipping on replicas to ADD FK and TRUNCATE |
| Date: | 2026-07-20 11:18:23 |
| Message-ID: | CA+FpmFdNFehW5r8KpVEFO+swCeTKc6pEETfwKkE8242wKg-dHw@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Sun, 11 Jan 2026 at 14:29, Hannu Krosing <hannuk(at)google(dot)com> wrote:
> rebased to latest
>
> On Sun, Jul 6, 2025 at 4:48 PM Hannu Krosing <hannuk(at)google(dot)com> wrote:
>
>> And now it also passes tests.
>>
>> Still learning about the git way of generating PostgreSQL patches,
>> that's why there are two separate ones
>>
>> On Sun, Jul 6, 2025 at 4:30 PM Hannu Krosing <hannuk(at)google(dot)com> wrote:
>> >
>> > Managed to send wrong patch earlier, this one actually compiles
>> >
>> > On Sun, Jul 6, 2025 at 1:48 PM Hannu Krosing <hannuk(at)google(dot)com> wrote:
>> > >
>> > > 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 <hannuk(at)google(dot)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 <hannuk(at)google(dot)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
>>
>
I looked into this patch and found a case which doesn't look right,
CREATE TABLE pkt(id int PRIMARY KEY);
CREATE TABLE fkt(fk int);
INSERT INTO fkt VALUES(1); -- no matching pkt row
SET session_replication_role=replica;
ALTER TABLE fkt ADD FOREIGN KEY (fk) REFERENCES pkt(id);
The Alter command goes through successfully. So, basically, fkt has one row
which doesn't follow the constraint.
--
Regards,
Rafia Sabih
CYBERTEC PostgreSQL International GmbH
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Aleksander Alekseev | 2026-07-20 11:42:56 | Re: [PATCH] Cover get_json_table_plan() with tests |
| Previous Message | Ian Lawrence Barwick | 2026-07-20 11:10:11 | Re: [PATCH] Add pg_get_event_trigger_ddl() function |