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

Hi Ayush,

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.

This solidifies my conviction that the batching layer will keep
showing issues if hammered like this, so I'm now inclined to revert it
at least from v19 to unblock the release process. I will post
separately about that.

--
Thanks, Amit Langote