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

On Fri, Sep 4, 2026 at 8:36 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> 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
> >> > > 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.

An AI suggested that more comment fixes were needed given 0001's
changes (removal of notion of after trigger being "active"), so fixed
those too. Here's another version.

--
Thanks, Amit Langote