public inbox for [email protected]help / color / mirror / Atom feed
Add comments about fire_triggers argument in ri_triggers.c 7+ messages / 4 participants [nested] [flat]
* Add comments about fire_triggers argument in ri_triggers.c @ 2025-03-31 12:26 Yugo Nagata <[email protected]> 2025-11-24 09:22 ` Re: Add comments about fire_triggers argument in ri_triggers.c Kirill Reshke <[email protected]> 2026-03-27 00:39 ` Re: Add comments about fire_triggers argument in ri_triggers.c Amit Langote <[email protected]> 0 siblings, 2 replies; 7+ messages in thread From: Yugo Nagata @ 2025-03-31 12:26 UTC (permalink / raw) To: pgsql-hackers 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 <[email protected]> Attachments: [text/x-diff] add_comments_fire_triggers_in_ri_triggers.patch (759B, 2-add_comments_fire_triggers_in_ri_triggers.patch) download | inline diff: diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index c4ff18ce65e..45d7e6268e4 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -2580,7 +2580,13 @@ 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 so that AFTER triggers run at the end of + * the query. This ensures 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, ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Add comments about fire_triggers argument in ri_triggers.c 2025-03-31 12:26 Add comments about fire_triggers argument in ri_triggers.c Yugo Nagata <[email protected]> @ 2025-11-24 09:22 ` Kirill Reshke <[email protected]> 2025-11-26 02:48 ` Re: Add comments about fire_triggers argument in ri_triggers.c Amit Langote <[email protected]> 1 sibling, 1 reply; 7+ messages in thread From: Kirill Reshke @ 2025-11-24 09:22 UTC (permalink / raw) To: Yugo Nagata <[email protected]>; +Cc: pgsql-hackers On Mon, 31 Mar 2025 at 17:27, Yugo Nagata <[email protected]> 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 <[email protected]> Hi! I checked your patch and I agree that your comment makes things more clear. Your patch LGTM -- Best regards, Kirill Reshke ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Add comments about fire_triggers argument in ri_triggers.c 2025-03-31 12:26 Add comments about fire_triggers argument in ri_triggers.c Yugo Nagata <[email protected]> 2025-11-24 09:22 ` Re: Add comments about fire_triggers argument in ri_triggers.c Kirill Reshke <[email protected]> @ 2025-11-26 02:48 ` Amit Langote <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Amit Langote @ 2025-11-26 02:48 UTC (permalink / raw) To: Kirill Reshke <[email protected]>; +Cc: Yugo Nagata <[email protected]>; pgsql-hackers Hi, On Mon, Nov 24, 2025 at 6:23 PM Kirill Reshke <[email protected]> wrote: > On Mon, 31 Mar 2025 at 17:27, Yugo Nagata <[email protected]> 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 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Add comments about fire_triggers argument in ri_triggers.c 2025-03-31 12:26 Add comments about fire_triggers argument in ri_triggers.c Yugo Nagata <[email protected]> @ 2026-03-27 00:39 ` Amit Langote <[email protected]> 2026-03-27 05:36 ` Re: Add comments about fire_triggers argument in ri_triggers.c Amit Langote <[email protected]> 1 sibling, 1 reply; 7+ messages in thread From: Amit Langote @ 2026-03-27 00:39 UTC (permalink / raw) To: Yugo Nagata <[email protected]>; +Cc: surya poondla <[email protected]>; pgsql-hackers; Kirill Reshke <[email protected]> On Fri, Mar 27, 2026 at 12:56 AM Yugo Nagata <[email protected]> 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 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Add comments about fire_triggers argument in ri_triggers.c 2025-03-31 12:26 Add comments about fire_triggers argument in ri_triggers.c Yugo Nagata <[email protected]> 2026-03-27 00:39 ` Re: Add comments about fire_triggers argument in ri_triggers.c Amit Langote <[email protected]> @ 2026-03-27 05:36 ` Amit Langote <[email protected]> 2026-03-30 01:16 ` Re: Add comments about fire_triggers argument in ri_triggers.c Amit Langote <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Amit Langote @ 2026-03-27 05:36 UTC (permalink / raw) To: Yugo Nagata <[email protected]>; +Cc: surya poondla <[email protected]>; pgsql-hackers; Kirill Reshke <[email protected]> On Fri, Mar 27, 2026 at 12:01 PM Yugo Nagata <[email protected]> wrote: > On Fri, 27 Mar 2026 09:39:17 +0900 > Amit Langote <[email protected]> wrote: > > > On Fri, Mar 27, 2026 at 12:56 AM Yugo Nagata <[email protected]> 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 Attachments: [application/octet-stream] v3-0001-Add-comment-explaining-fire_triggers-false-in-ri_.patch (1.7K, 2-v3-0001-Add-comment-explaining-fire_triggers-false-in-ri_.patch) download | inline diff: From 08c90997b682d88d59d6481ea00542108b867a59 Mon Sep 17 00:00:00 2001 From: Amit Langote <[email protected]> Date: Fri, 27 Mar 2026 14:33:35 +0900 Subject: [PATCH v3] Add comment explaining fire_triggers=false in ri_PerformCheck() The reason for passing fire_triggers=false to SPI_execute_snapshot() in ri_PerformCheck() was not documented, making it unclear why it was done that way. Add a comment explaining that it ensures AFTER triggers are queued in the outer query's after-trigger context and fire only after all RI updates on the same row are complete. Author: Yugo Nagata <[email protected]> Reviewed-by: Kirill Reshke <[email protected]> Reviewed-by: Surya Poondla <[email protected]> Discussion: https://postgr.es/m/[email protected] --- src/backend/utils/adt/ri_triggers.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index d22b8ef7f3c..6230a2ea9ad 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -2582,7 +2582,13 @@ 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 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. + */ spi_result = SPI_execute_snapshot(qplan, vals, nulls, test_snapshot, crosscheck_snapshot, -- 2.47.3 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Add comments about fire_triggers argument in ri_triggers.c 2025-03-31 12:26 Add comments about fire_triggers argument in ri_triggers.c Yugo Nagata <[email protected]> 2026-03-27 00:39 ` Re: Add comments about fire_triggers argument in ri_triggers.c Amit Langote <[email protected]> 2026-03-27 05:36 ` Re: Add comments about fire_triggers argument in ri_triggers.c Amit Langote <[email protected]> @ 2026-03-30 01:16 ` Amit Langote <[email protected]> 2026-03-30 17:34 ` Re: Add comments about fire_triggers argument in ri_triggers.c surya poondla <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Amit Langote @ 2026-03-30 01:16 UTC (permalink / raw) To: Yugo Nagata <[email protected]>; +Cc: surya poondla <[email protected]>; pgsql-hackers; Kirill Reshke <[email protected]> On Fri, Mar 27, 2026 at 2:36 PM Amit Langote <[email protected]> wrote: > On Fri, Mar 27, 2026 at 12:01 PM Yugo Nagata <[email protected]> wrote: > > On Fri, 27 Mar 2026 09:39:17 +0900 > > Amit Langote <[email protected]> wrote: > > > > > On Fri, Mar 27, 2026 at 12:56 AM Yugo Nagata <[email protected]> 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 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Add comments about fire_triggers argument in ri_triggers.c 2025-03-31 12:26 Add comments about fire_triggers argument in ri_triggers.c Yugo Nagata <[email protected]> 2026-03-27 00:39 ` Re: Add comments about fire_triggers argument in ri_triggers.c Amit Langote <[email protected]> 2026-03-27 05:36 ` Re: Add comments about fire_triggers argument in ri_triggers.c Amit Langote <[email protected]> 2026-03-30 01:16 ` Re: Add comments about fire_triggers argument in ri_triggers.c Amit Langote <[email protected]> @ 2026-03-30 17:34 ` surya poondla <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: surya poondla @ 2026-03-30 17:34 UTC (permalink / raw) To: Amit Langote <[email protected]>; +Cc: Yugo Nagata <[email protected]>; pgsql-hackers; Kirill Reshke <[email protected]> Hi Amit, Nagata The final patch looks good and gives more context. Thanks for merging this. Regards, Surya Poondla ^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2026-03-30 17:34 UTC | newest] Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2025-03-31 12:26 Add comments about fire_triggers argument in ri_triggers.c Yugo Nagata <[email protected]> 2025-11-24 09:22 ` Kirill Reshke <[email protected]> 2025-11-26 02:48 ` Amit Langote <[email protected]> 2026-03-27 00:39 ` Amit Langote <[email protected]> 2026-03-27 05:36 ` Amit Langote <[email protected]> 2026-03-30 01:16 ` Amit Langote <[email protected]> 2026-03-30 17:34 ` surya poondla <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox