Re: PG19 FK fast path: OOB write and missed FK checks during batched

On Mon, Aug 31, 2026 at 10:38 PM Ayush Tiwari
<ayushtiwari(dot)slg01(at)gmail(dot)com> wrote:
> On Mon, 31 Aug 2026 at 14:33, Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>>
>> On Mon, Aug 31, 2026 at 5:57 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>> > On Sat, Aug 29, 2026 at 8:06 PM Ayush Tiwari
>> > <ayushtiwari(dot)slg01(at)gmail(dot)com> wrote:
>> > > On Thu, 27 Aug 2026 at 18:11, Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>> > >> On Wed, Aug 26, 2026 at 10:25 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>> > >> > On Sat, Aug 22, 2026 at 4:43 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>> > >> > > Pushed and closed the item.
>> > >> >
>> > >> > Ayush Tiwari reported another hole in the per-firing-cycle fix to me
>> > >> > off-list. Here is his reproducer:
>> > >> >
>> > >> > CREATE TABLE pk (id int PRIMARY KEY);
>> > >> > INSERT INTO pk VALUES (1);
>> > >> >
>> > >> > CREATE TABLE fk (
>> > >> > a int REFERENCES pk (id),
>> > >> > b int CONSTRAINT fk_deferred REFERENCES pk (id)
>> > >> > DEFERRABLE INITIALLY DEFERRED);
>> > >> >
>> > >> > CREATE FUNCTION check_now() RETURNS trigger LANGUAGE plpgsql AS $$
>> > >> > BEGIN
>> > >> > BEGIN
>> > >> > SET CONSTRAINTS fk_deferred IMMEDIATE;
>> > >> > EXCEPTION WHEN foreign_key_violation THEN
>> > >> > RAISE NOTICE 'caught by SET CONSTRAINTS';
>> > >> > END;
>> > >> > RETURN NEW;
>> > >> > END$$;
>> > >> >
>> > >> > -- Name sorts after the RI trigger, so column a is already batched.
>> > >> > CREATE TRIGGER zz_check_now AFTER INSERT ON fk
>> > >> > FOR EACH ROW EXECUTE FUNCTION check_now();
>> > >> >
>> > >> > BEGIN;
>> > >> > INSERT INTO fk VALUES (1, 999);
>> > >> >
>> > >> > On the current master with my fixes from last week, an
>> > >> > assertion-enabled build produces:
>> > >> >
>> > >> > CREATE TABLE
>> > >> > INSERT 0 1
>> > >> > CREATE TABLE
>> > >> > CREATE FUNCTION
>> > >> > CREATE TRIGGER
>> > >> > BEGIN
>> > >> > WARNING: resource was not closed: relation "pk_pkey"
>> > >> > WARNING: resource was not closed: relation "pk"
>> > >> > WARNING: resource was not closed: TupleDesc 0xffff812677f0 (16392,-1)
>> > >> > WARNING: resource was not closed: TupleDesc 0xffff81270500 (16386,-1)
>> > >> > server closed the connection unexpectedly
>> > >> > This probably means the server terminated abnormally
>> > >> > before or while processing the request.
>> > >> > The connection to the server was lost.
>> > >> >
>> > >> > The relevant part of the backtrace is:
>> > >> >
>> > >> > #3 ExceptionalCondition (conditionName="false",
>> > >> > fileName="../src/backend/utils/adt/ri_triggers.c", lineNumber=4543)
>> > >> > #4 AtEOSubXact_RI (isCommit=true, mySubid=2, parentSubid=1)
>> > >> > at ../src/backend/utils/adt/ri_triggers.c:4543
>> > >> > #5 CommitSubTransaction ()
>> > >> > at ../src/backend/access/transam/xact.c:5247
>> > >> > #6 ReleaseCurrentSubTransaction ()
>> > >> > at ../src/backend/access/transam/xact.c:4836
>> > >> > #7 exec_stmt_block (...)
>> > >> > at ../src/pl/plpgsql/src/pl_exec.c:1859
>> > >> >
>> > >> > SET CONSTRAINTS ... IMMEDIATE starts a nested firing cycle without
>> > >> > opening a new query level, so keying the cache by constraint OID and
>> > >> > query depth does not distinguish this cycle from the enclosing one.
>> > >> >
>> > >> > I am preparing a patch that uses firing depth, gives the SET
>> > >> > CONSTRAINTS cycle its own callback list, and removes stale entries on
>> > >> > either subtransaction commit or abort. I will post it shortly.
>> > >>
>> > >> Here is that patch.
>> > >>
>> > >> One thing missing from my report: the crash wasn't the worst of it.
>> > >> The nested cycle's batch was never flushed, so SET CONSTRAINTS ...
>> > >> IMMEDIATE returned success for a constraint it hadn't checked, and
>> > >> the orphan row reached commit.
>> > >>
>> > >> I'm not proposing to commit this yet. I'm assessing whether the
>> > >> batching layer holds up under a test harness and will give the RMT my
>> > >> assessment by Monday on whether to keep it in v19. If the batching
>> > >> layer is reverted from v19, this patch is master-only, assuming it's
>> > >> ok for master to keep the layer in its current form. If it stays, it
>> > >> belongs in both branches. I'm happy to commit and backpatch sooner if
>> > >> that's preferred for the open item.
>> > >
>> > >
>> > > Thanks for the patch. I'm aware that you are still testing this but wanted to
>> > > add that I do see a problem when ALTER TABLE ... ADD FORIEGN KEY
>> > > is run from an AFTER trigger.
>> >
>> > Thanks for the report.
>> >
>> > > I tried such a case in a non-cassert build, with RLS forcing the existing rows
>> > > to be checked one at a time. The command appeared to succeed and the
>> > > constraint was marked valid, even though the table still contained an orphan
>> > > row. I also saw warnings about relation and TupleDesc resources not being
>> > > closed.
>> > >
>> > > The reproducer I used was:
>> > >
>> > > CREATE ROLE fp_alter_role;
>> > > CREATE TABLE fp_alter_pk (id int PRIMARY KEY);
>> > > INSERT INTO fp_alter_pk VALUES (1);
>> > > ALTER TABLE fp_alter_pk ENABLE ROW LEVEL SECURITY;
>> > > CREATE POLICY fp_alter_pk_all ON fp_alter_pk USING (true);
>> > > GRANT REFERENCES, SELECT ON fp_alter_pk TO fp_alter_role;
>> > >
>> > > CREATE TABLE fp_alter_fk (a int);
>> > > INSERT INTO fp_alter_fk VALUES (1), (999);
>> > > ALTER TABLE fp_alter_fk OWNER TO fp_alter_role;
>> > >
>> > > CREATE TABLE fp_alter_outer (a int);
>> > > ALTER TABLE fp_alter_outer OWNER TO fp_alter_role;
>> > >
>> > > CREATE FUNCTION fp_alter_from_trigger() RETURNS trigger
>> > > LANGUAGE plpgsql AS $$
>> > > BEGIN
>> > > BEGIN
>> > > EXECUTE 'ALTER TABLE fp_alter_fk ADD CONSTRAINT '
>> > > 'fp_alter_bad_fk FOREIGN KEY (a) '
>> > > 'REFERENCES fp_alter_pk(id)';
>> > > EXCEPTION WHEN others THEN
>> > > RAISE;
>> > > END;
>> > > RETURN NEW;
>> > > END
>> > > $$;
>> > >
>> > > CREATE TRIGGER fp_alter_trg
>> > > AFTER INSERT ON fp_alter_outer
>> > > FOR EACH ROW EXECUTE FUNCTION fp_alter_from_trigger();
>> > >
>> > > SET ROLE fp_alter_role;
>> > > INSERT INTO fp_alter_outer VALUES (1);
>> > > RESET ROLE;
>> > >
>> > > SELECT conname, convalidated
>> > > FROM pg_constraint
>> > > WHERE conname = 'fp_alter_bad_fk';
>> > > TABLE fp_alter_fk;
>> > >
>> > > Could the validation calls be joining the outer trigger's batch because
>> > > AfterTriggerIsActive() is true, and then be removed by AtEOSubXact_RI() before
>> > > they are checked? The validation path sets trig.tgoid to InvalidOid, so would
>> > > it make sense to use that to keep these calls on the non-batched path?
>> >
>> > Your diagnosis is correct; tgoid would work. Though, I'd rather make
>> > the caller state that explicitly instead of gleaning it from
>> > trigger.c-internal state, which is what I should have done originally.
>> > Add a allow_batch parameter to RI_FKey_check() and new validation
>> > function called from ALTER TABLE code, instead of RI_FKey_check_ins(),
>> > which calls RI_FKey_check() with 'false' for allow_batch. Attached
>> > 0001 does that and also contains your test case. 0002 unchanged.
>>
>> Added a separate open item for this, so there are two for 0001 and 0002, resp.
>>
>> Fixed by 0001 (just added):
>> RI fast-path batching wrongly used by ALTER TABLE inside a trigger
>> Commit: b7b27eb41a5
>> Owner: Amit Langote
>>
>> Fixed by 0002:
>> RI fast-path batching fails during nested SET CONSTRAINTS
>> Commit: 6fc2a486417d
>> Owner: Amit Langote
>
>
> Thanks for the updated patches.
>
> I tested them and both look good to me.
>
> Just one small nit on comments:
>
> The validation comments in validateForeignKeyConstraint() still mention
> calling RI_FKey_check_ins() and flinfo, although 0001 now calls
> RI_FKey_check_validate() directly. The firing_depth field comment also
> still mentions the removed AfterTriggerIsActive() helper.
>
> Other than those points, I did not find a correctness issue in the series.

Thanks, Ayush; fixed those in the attached. Will push these to only
master after the revert-this-code-from-19 thread [1] settles.

--
Thanks, Amit Langote

[1] https://www.postgresql.org/message-id/CA%2BHiwqEDcCSDoYTtoXH4Ta_BpGN%2BrevMZSLLj6GRz2_5ybwE1g%40mail.gmail.com