public inbox for [email protected]help / color / mirror / Atom feed
Re: Add comments about fire_triggers argument in ri_triggers.c 5+ messages / 2 participants [nested] [flat]
* Re: Add comments about fire_triggers argument in ri_triggers.c @ 2026-01-29 00:56 surya poondla <[email protected]> 2026-03-27 00:39 ` Re: Add comments about fire_triggers argument in ri_triggers.c Amit Langote <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: surya poondla @ 2026-01-29 00:56 UTC (permalink / raw) To: Yugo Nagata <[email protected]>; +Cc: pgsql-hackers 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 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Add comments about fire_triggers argument in ri_triggers.c 2026-01-29 00:56 Re: Add comments about fire_triggers argument in ri_triggers.c surya poondla <[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]> 0 siblings, 1 reply; 5+ 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] 5+ messages in thread
* Re: Add comments about fire_triggers argument in ri_triggers.c 2026-01-29 00:56 Re: Add comments about fire_triggers argument in ri_triggers.c surya poondla <[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; 5+ 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] 5+ messages in thread
* Re: Add comments about fire_triggers argument in ri_triggers.c 2026-01-29 00:56 Re: Add comments about fire_triggers argument in ri_triggers.c surya poondla <[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; 5+ 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] 5+ messages in thread
* Re: Add comments about fire_triggers argument in ri_triggers.c 2026-01-29 00:56 Re: Add comments about fire_triggers argument in ri_triggers.c surya poondla <[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; 5+ 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] 5+ messages in thread
end of thread, other threads:[~2026-03-30 17:34 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-01-29 00:56 Re: Add comments about fire_triggers argument in ri_triggers.c surya poondla <[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