Hi, 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: > > > On Thu, Aug 20, 2026 at 5:34 PM Amit Langote <[email protected]> > wrote: > > > > On Wed, Aug 19, 2026 at 10:12 PM Amit Langote < > [email protected]> wrote: > > > > > On Wed, Aug 19, 2026 at 12:09 AM Noah Misch <[email protected]> > 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
