Re: Add comments about fire_triggers argument in ri_triggers.c
On Mon, 30 Mar 2026 10:16:02 +0900
Amit Langote wrote:
> On Fri, Mar 27, 2026 at 2:36 PM Amit Langote wrote:
> > On Fri, Mar 27, 2026 at 12:01 PM Yugo Nagata wrote:
> > > On Fri, 27 Mar 2026 09:39:17 +0900
> > > Amit Langote wrote:
> > >
> > > > On Fri, Mar 27, 2026 at 12:56 AM Yugo Nagata
> > > > wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > Thank you all for the review and comments.
> > > > >
> > > > > > Yes Amit, I agree that SPI_execute_snapshot() comments do provide
> > > > > > some
> > > > > > context on AFTER triggers, but I still feel the newly added comment
> > > > > > in ri_PerformCheck() gives additional context on why the
> > > > > > fire_triggers is
> > > > > > set to false.
> > > > >
> > > > > Yes, that is what I intended. The existing comments on
> > > > > SPI_execute_snapshot() explain how the fire_triggers parameter works,
> > > > > but I would like to add a comment explaining why the AFTER trigger for
> > > > > RI needs to set it to false.
> > > > >
> > > > > If the explanation of the effect of fire_triggers seems redundant, I
> > > > > am
> > > > > fine with the following shorter version:
> > > > >
> > > > > +* Set fire_triggers to false to ensure that check triggers
> > > > > fire after all
> > > > > +* RI updates on the same row are complete.
> > > >
> > > > Thanks for the updated patch. Yes, adding the comment might be good,
> > > > but I'd suggest a small tweak:
> > > >
> > > > +* Set fire_triggers to false to ensure that AFTER triggers
> > > > are queued in
> > > > +* the outer query's after-trigger context and fire after all
> > > > RI updates on
> > > > +* the same row are complete, rather than immediately.
> > > >
> > > > Two changes:
> > > >
> > > > * "check triggers" -> "AFTER triggers", since fire_triggers=false
> > > > affects any AFTER triggers queued during the SPI execution, not just
> > > > RI check triggers.
> > > >
> > > > * mention of the outer query's after-trigger context to explain the
> > > > mechanism by which the deferral works.
> > > >
> > > > Does that additional context help?
> > >
> > > Thank you for the suggestion.
> > > That looks good to me. It is clearer than the previous version.
> >
> > Ok, will push the attached.
>
> Pushed.
Thank you!
> I verified locally with a test case involving a CASCADE DELETE on two
> parent rows that the comment is indeed accurate:
>
> CREATE TABLE parent (id int PRIMARY KEY);
> CREATE TABLE child (id int REFERENCES parent ON DELETE CASCADE);
> CREATE TABLE log (seq serial, msg text);
>
> CREATE OR REPLACE FUNCTION child_after_del() RETURNS trigger AS $$
> BEGIN
> INSERT INTO log(msg) VALUES ('child deleted: ' || OLD.id);
> RETURN NULL;
> END;
> $$ LANGUAGE plpgsql;
>
> CREATE TRIGGER "A_child_after_del_trig"
> AFTER DELETE ON child
> FOR EACH ROW EXECUTE FUNCTION child_after_del();
> CREATE OR REPLACE FUNCTION parent_after_del() RETURNS trigger AS $$
> BEGIN
> INSERT INTO log(msg) VALUES ('parent deleted: ' || OLD.id);
> RETURN NULL;
> END;
> $$ LANGUAGE plpgsql;
>
> CREATE TRIGGER "A_parent_after_del_trig"
> AFTER DELETE ON parent
> FOR EACH ROW EXECUTE FUNCTION parent_after_del();
>
> INSERT INTO parent VALUES (1), (2);
> INSERT INTO child VALUES (1), (2);
> DELETE FROM parent;
>
> SELECT msg FROM log ORDER BY seq;
> msg
> ---
> parent deleted: 1
> parent deleted: 2
> child deleted: 1
> child deleted: 2
> (4 rows)
>
Thank you for the verification.
This behavior seems consistent with the comment.
Regards,
Yugo Nagata
--
Yugo Nagata
Re: Add comments about fire_triggers argument in ri_triggers.c
Hi Amit, Nagata The final patch looks good and gives more context. Thanks for merging this. Regards, Surya Poondla
Re: Add comments about fire_triggers argument in ri_triggers.c
On Fri, Mar 27, 2026 at 2:36 PM Amit Langote wrote:
> On Fri, Mar 27, 2026 at 12:01 PM Yugo Nagata wrote:
> > On Fri, 27 Mar 2026 09:39:17 +0900
> > Amit Langote wrote:
> >
> > > On Fri, Mar 27, 2026 at 12:56 AM Yugo Nagata wrote:
> > > >
> > > > Hi,
> > > >
> > > > Thank you all for the review and comments.
> > > >
> > > > > Yes Amit, I agree that SPI_execute_snapshot() comments do provide some
> > > > > context on AFTER triggers, but I still feel the newly added comment
> > > > > in ri_PerformCheck() gives additional context on why the
> > > > > fire_triggers is
> > > > > set to false.
> > > >
> > > > Yes, that is what I intended. The existing comments on
> > > > SPI_execute_snapshot() explain how the fire_triggers parameter works,
> > > > but I would like to add a comment explaining why the AFTER trigger for
> > > > RI needs to set it to false.
> > > >
> > > > If the explanation of the effect of fire_triggers seems redundant, I am
> > > > fine with the following shorter version:
> > > >
> > > > +* Set fire_triggers to false to ensure that check triggers
> > > > fire after all
> > > > +* RI updates on the same row are complete.
> > >
> > > Thanks for the updated patch. Yes, adding the comment might be good,
> > > but I'd suggest a small tweak:
> > >
> > > +* Set fire_triggers to false to ensure that AFTER triggers
> > > are queued in
> > > +* the outer query's after-trigger context and fire after all
> > > RI updates on
> > > +* the same row are complete, rather than immediately.
> > >
> > > Two changes:
> > >
> > > * "check triggers" -> "AFTER triggers", since fire_triggers=false
> > > affects any AFTER triggers queued during the SPI execution, not just
> > > RI check triggers.
> > >
> > > * mention of the outer query's after-trigger context to explain the
> > > mechanism by which the deferral works.
> > >
> > > Does that additional context help?
> >
> > Thank you for the suggestion.
> > That looks good to me. It is clearer than the previous version.
>
> Ok, will push the attached.
Pushed.
I verified locally with a test case involving a CASCADE DELETE on two
parent rows that the comment is indeed accurate:
CREATE TABLE parent (id int PRIMARY KEY);
CREATE TABLE child (id int REFERENCES parent ON DELETE CASCADE);
CREATE TABLE log (seq serial, msg text);
CREATE OR REPLACE FUNCTION child_after_del() RETURNS trigger AS $$
BEGIN
INSERT INTO log(msg) VALUES ('child deleted: ' || OLD.id);
RETURN NULL;
END;
$$ LANGUAGE plpgsql;
CREATE TRIGGER "A_child_after_del_trig"
AFTER DELETE ON child
FOR EACH ROW EXECUTE FUNCTION child_after_del();
CREATE OR REPLACE FUNCTION parent_after_del() RETURNS trigger AS $$
BEGIN
INSERT INTO log(msg) VALUES ('parent deleted: ' || OLD.id);
RETURN NULL;
END;
$$ LANGUAGE plpgsql;
CREATE TRIGGER "A_parent_after_del_trig"
AFTER DELETE ON parent
FOR EACH ROW EXECUTE FUNCTION parent_after_del();
INSERT INTO parent VALUES (1), (2);
INSERT INTO child VALUES (1), (2);
DELETE FROM parent;
SELECT msg FROM log ORDER BY seq;
msg
---
parent deleted: 1
parent deleted: 2
child deleted: 1
child deleted: 2
(4 rows)
Thanks again Nagata-san for the patch.
--
Thanks, Amit Langote
Re: Add comments about fire_triggers argument in ri_triggers.c
On Fri, Mar 27, 2026 at 12:01 PM Yugo Nagata wrote: > On Fri, 27 Mar 2026 09:39:17 +0900 > Amit Langote wrote: > > > On Fri, Mar 27, 2026 at 12:56 AM Yugo Nagata wrote: > > > > > > Hi, > > > > > > Thank you all for the review and comments. > > > > > > > Yes Amit, I agree that SPI_execute_snapshot() comments do provide some > > > > context on AFTER triggers, but I still feel the newly added comment > > > > in ri_PerformCheck() gives additional context on why the fire_triggers > > > > is > > > > set to false. > > > > > > Yes, that is what I intended. The existing comments on > > > SPI_execute_snapshot() explain how the fire_triggers parameter works, > > > but I would like to add a comment explaining why the AFTER trigger for > > > RI needs to set it to false. > > > > > > If the explanation of the effect of fire_triggers seems redundant, I am > > > fine with the following shorter version: > > > > > > +* Set fire_triggers to false to ensure that check triggers fire > > > after all > > > +* RI updates on the same row are complete. > > > > Thanks for the updated patch. Yes, adding the comment might be good, > > but I'd suggest a small tweak: > > > > +* Set fire_triggers to false to ensure that AFTER triggers > > are queued in > > +* the outer query's after-trigger context and fire after all > > RI updates on > > +* the same row are complete, rather than immediately. > > > > Two changes: > > > > * "check triggers" -> "AFTER triggers", since fire_triggers=false > > affects any AFTER triggers queued during the SPI execution, not just > > RI check triggers. > > > > * mention of the outer query's after-trigger context to explain the > > mechanism by which the deferral works. > > > > Does that additional context help? > > Thank you for the suggestion. > That looks good to me. It is clearer than the previous version. Ok, will push the attached. -- Thanks, Amit Langote v3-0001-Add-comment-explaining-fire_triggers-false-in-ri_.patch Description: Binary data
Re: Add comments about fire_triggers argument in ri_triggers.c
On Fri, 27 Mar 2026 09:39:17 +0900 Amit Langote wrote: > On Fri, Mar 27, 2026 at 12:56 AM Yugo Nagata wrote: > > > > Hi, > > > > Thank you all for the review and comments. > > > > > Yes Amit, I agree that SPI_execute_snapshot() comments do provide some > > > context on AFTER triggers, but I still feel the newly added comment > > > in ri_PerformCheck() gives additional context on why the fire_triggers is > > > set to false. > > > > Yes, that is what I intended. The existing comments on > > SPI_execute_snapshot() explain how the fire_triggers parameter works, > > but I would like to add a comment explaining why the AFTER trigger for > > RI needs to set it to false. > > > > If the explanation of the effect of fire_triggers seems redundant, I am > > fine with the following shorter version: > > > > +* Set fire_triggers to false to ensure that check triggers fire > > after all > > +* RI updates on the same row are complete. > > Thanks for the updated patch. Yes, adding the comment might be good, > but I'd suggest a small tweak: > > +* Set fire_triggers to false to ensure that AFTER triggers > are queued in > +* the outer query's after-trigger context and fire after all > RI updates on > +* the same row are complete, rather than immediately. > > Two changes: > > * "check triggers" -> "AFTER triggers", since fire_triggers=false > affects any AFTER triggers queued during the SPI execution, not just > RI check triggers. > > * mention of the outer query's after-trigger context to explain the > mechanism by which the deferral works. > > Does that additional context help? Thank you for the suggestion. That looks good to me. It is clearer than the previous version. Regards, Yugo Nagata -- Yugo Nagata
Re: Add comments about fire_triggers argument in ri_triggers.c
On Fri, Mar 27, 2026 at 12:56 AM Yugo Nagata wrote: > > Hi, > > Thank you all for the review and comments. > > > Yes Amit, I agree that SPI_execute_snapshot() comments do provide some > > context on AFTER triggers, but I still feel the newly added comment > > in ri_PerformCheck() gives additional context on why the fire_triggers is > > set to false. > > Yes, that is what I intended. The existing comments on > SPI_execute_snapshot() explain how the fire_triggers parameter works, > but I would like to add a comment explaining why the AFTER trigger for > RI needs to set it to false. > > If the explanation of the effect of fire_triggers seems redundant, I am > fine with the following shorter version: > > +* Set fire_triggers to false to ensure that check triggers fire > after all > +* RI updates on the same row are complete. Thanks for the updated patch. Yes, adding the comment might be good, but I'd suggest a small tweak: +* Set fire_triggers to false to ensure that AFTER triggers are queued in +* the outer query's after-trigger context and fire after all RI updates on +* the same row are complete, rather than immediately. Two changes: * "check triggers" -> "AFTER triggers", since fire_triggers=false affects any AFTER triggers queued during the SPI execution, not just RI check triggers. * mention of the outer query's after-trigger context to explain the mechanism by which the deferral works. Does that additional context help? -- Thanks, Amit Langote
Re: Add comments about fire_triggers argument in ri_triggers.c
Hi, Thank you all for the review and comments. > Yes Amit, I agree that SPI_execute_snapshot() comments do provide some > context on AFTER triggers, but I still feel the newly added comment > in ri_PerformCheck() gives additional context on why the fire_triggers is > set to false. Yes, that is what I intended. The existing comments on SPI_execute_snapshot() explain how the fire_triggers parameter works, but I would like to add a comment explaining why the AFTER trigger for RI needs to set it to false. If the explanation of the effect of fire_triggers seems redundant, I am fine with the following shorter version: +* Set fire_triggers to false to ensure that check triggers fire after all +* RI updates on the same row are complete. I've attached an updated version reflecting this. Regards, Yugo Nagata -- Yugo Nagata diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index d22b8ef7f3c..4cbce56951b 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -2582,7 +2582,12 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo, save_sec_context | SECURITY_LOCAL_USERID_CHANGE | SECURITY_NOFORCE_RLS); - /* Finally we can run the query. */ + /* + * Finally we can run the query. + * + * Set fire_triggers to false to ensure that check triggers fire after all + * RI updates on the same row are complete. + */ spi_result = SPI_execute_snapshot(qplan, vals, nulls, test_snapshot, crosscheck_snapshot,
Re: Add comments about fire_triggers argument in ri_triggers.c
Hi Yugo, Your patch change looks good. Yes Amit, I agree that SPI_execute_snapshot() comments do provide some context on AFTER triggers, but I still feel the newly added comment in ri_PerformCheck() gives additional context on why the fire_triggers is set to false. Regards, Surya Poondla
Re: Add comments about fire_triggers argument in ri_triggers.c
Hi, On Mon, Nov 24, 2025 at 6:23 PM Kirill Reshke wrote: > On Mon, 31 Mar 2025 at 17:27, Yugo Nagata wrote: > > > > Hi, > > > > SPI_execute_snapshot() has a argument called "fire_triggers". If this is > > false, > > AFTER triggers are postponed to end of the query. This is true in normal > > case, > > but set to false in RI triggers. > > > > This is introduced by 9cb84097623e in 2007. It is aimed to fire check > > triggers > > after all RI updates on the same row are complete. > > > > However, I cannot find explanation of"why this is required" in the > > codebase. > > Therefore, I've attached a patch add comments in ri_trigger.c for > > explaining why > > fire_triggers is specified to false. > > > > SPI_execute_snapshot() are used in a few places in ri_trigger.c, but I added > > the comments only in ri_PerformCheck() because SPI_execute_snapshot() are > > used > > only for SELECT quereis in other places. Therefore, I wonder fire_triggers > > is > > not needed to be false in these places, but I left them as is. > > I checked your patch and I agree that your comment makes things more clear. > > Your patch LGTM Hmm, isn’t that already explained in the comment for SPI_execute_snapshot()? /* * SPI_execute_snapshot -- identical to SPI_execute_plan, except that we allow * the caller to specify exactly which snapshots to use, which will be * registered here. Also, the caller may specify that AFTER triggers should be * queued as part of the outer query rather than being fired immediately at the * end of the command. That said, we could perhaps add a one-liner in ri_triggers.c as follows: /* * Finally we can run the query. * See SPI_execute_snapshot() for why fire_triggers = false. */ -- Thanks, Amit Langote
Re: Add comments about fire_triggers argument in ri_triggers.c
On Mon, 31 Mar 2025 at 17:27, Yugo Nagata wrote: > > Hi, > > SPI_execute_snapshot() has a argument called "fire_triggers". If this is > false, > AFTER triggers are postponed to end of the query. This is true in normal case, > but set to false in RI triggers. > > This is introduced by 9cb84097623e in 2007. It is aimed to fire check triggers > after all RI updates on the same row are complete. > > However, I cannot find explanation of"why this is required" in the codebase. > Therefore, I've attached a patch add comments in ri_trigger.c for explaining > why > fire_triggers is specified to false. > > SPI_execute_snapshot() are used in a few places in ri_trigger.c, but I added > the comments only in ri_PerformCheck() because SPI_execute_snapshot() are used > only for SELECT quereis in other places. Therefore, I wonder fire_triggers is > not needed to be false in these places, but I left them as is. > > Regards, > Yugo Nagata > > -- > Yugo Nagata Hi! I checked your patch and I agree that your comment makes things more clear. Your patch LGTM -- Best regards, Kirill Reshke
