On Mon, Aug 31, 2026 at 10:38 PM Ayush Tiwari <[email protected]> wrote: > On Mon, 31 Aug 2026 at 14:33, Amit Langote <[email protected]> wrote: >> >> On Mon, Aug 31, 2026 at 5:57 PM Amit Langote <[email protected]> wrote: >> > On Sat, Aug 29, 2026 at 8:06 PM Ayush Tiwari >> > <[email protected]> wrote: >> > > On Thu, 27 Aug 2026 at 18:11, Amit Langote <[email protected]> >> > > wrote: >> > >> On Wed, Aug 26, 2026 at 10:25 PM Amit Langote <[email protected]> >> > >> wrote: >> > >> > On Sat, Aug 22, 2026 at 4:43 PM Amit Langote >> > >> > <[email protected]> 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
v3-0002-Fix-RI-fast-path-batching-in-a-nested-SET-CONSTRA.patch
Description: Binary data
v3-0001-Don-t-let-ALTER-TABLE-validation-join-a-trigger-s.patch
Description: Binary data
