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

Hi,

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:
> > > On Thu, Aug 20, 2026 at 5:34 PM Amit Langote <amitlangote09(at)gmail(dot)com>
> wrote:
> > > > On Wed, Aug 19, 2026 at 10:12 PM Amit Langote <
> amitlangote09(at)gmail(dot)com> wrote:
> > > > > On Wed, Aug 19, 2026 at 12:09 AM Noah Misch <noah(at)leadboat(dot)com>
> wrote:
> > > > > > On Tue, Aug 18, 2026 at 10:42:14PM +0900, Amit Langote wrote:
> > > > > > > After considering Peter's report of another bug [1] that is
> fixed by
> > > > > > > 0001, I tested both his reproducer and nested firing involving
> the
> > > > > > > same constraint.
> > > > > > >
> > > > > > > The latter exposed a problem in v1: the cache was still keyed
> only by
> > > > > > > constraint OID, so a nested check of the same constraint
> reused the
> > > > > > > outer entry and did not register a callback at the nested
> query depth.
> > > > > > > In v2, the key is now (constraint OID, query depth), giving
> each
> > > > > > > firing level its own entry and callback.
> > > > > > >
> > > > > > > 0001 now includes regression tests for both cases. 0002 and
> 0003 are unchanged.
> > > > > > >
> > > > > > > I would like to commit these sometime this week and would
> appreciate a review.
> > > > > >
> > > > > > I won't be able to review this. The list should consider it up
> for grabs.
> > > > >
> > > > > Attached is v3, rebased over latest master.
> > > > >
> > > > > I reorganized the series to separate firing-state restoration
> (0001),
> > > > > per-firing-cycle batch and callback scoping that fixes Peter's
> report
> > > > > [1] (0002), and per-subtransaction batch tracking as suggested by
> Noah
> > > > > (0003). 0003 also folds in the invariant assertion previously sent
> > > > > separately. The combined code is otherwise unchanged from v2.
> > > > >
> > > > > [1]
> https://postgr.es/m/CAH2-Wz%3DD533JbF_ak_Pc8kP0FKse-ju8DnMxtjvY%3D%3DyHsP4xgw%40mail.gmail.com
> > > >
> > > > I've now pushed 0001 and 0002. Since they fix live bugs, I decided to
> > > > commit them sooner rather than later.
> > > >
> > > > I plan to commit the attached remaining patch tomorrow, barring
> > > > objections, and then close this item.
> > >
> > > 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.

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?

Regards,
Ayush