On Fri, Sep 4, 2026 at 8:36 PM Amit Langote <[email protected]> wrote: > 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 > >> > > 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
v4-0002-Fix-RI-fast-path-batching-in-a-nested-SET-CONSTRA.patch
Description: Binary data
v4-0001-Don-t-let-ALTER-TABLE-validation-join-a-trigger-s.patch
Description: Binary data
