Add comments about fire_triggers argument in ri_triggers.c

On Mon, 31 Mar 2025 at 17:27, Yugo Nagata <nagata@sraoss.co.jp> 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 <nagata@sraoss.co.jp>

Hi!

I checked your patch and I agree that your comment makes things more clear.

Your patch LGTM

--
Best regards,
Kirill Reshke

Hi,

On Mon, Nov 24, 2025 at 6:23 PM Kirill Reshke <reshkekirill@gmail.com> wrote:

On Mon, 31 Mar 2025 at 17:27, Yugo Nagata <nagata@sraoss.co.jp> 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

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

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 <nagata@sraoss.co.jp>

On Fri, Mar 27, 2026 at 12:56 AM Yugo Nagata <nagata@sraoss.co.jp> 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

On Fri, 27 Mar 2026 09:39:17 +0900
Amit Langote <amitlangote09@gmail.com> wrote:

On Fri, Mar 27, 2026 at 12:56 AM Yugo Nagata <nagata@sraoss.co.jp> 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 <nagata@sraoss.co.jp>

On Fri, Mar 27, 2026 at 12:01 PM Yugo Nagata <nagata@sraoss.co.jp> wrote:

On Fri, 27 Mar 2026 09:39:17 +0900
Amit Langote <amitlangote09@gmail.com> wrote:

On Fri, Mar 27, 2026 at 12:56 AM Yugo Nagata <nagata@sraoss.co.jp> 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

On Fri, Mar 27, 2026 at 2:36 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Fri, Mar 27, 2026 at 12:01 PM Yugo Nagata <nagata@sraoss.co.jp> wrote:

On Fri, 27 Mar 2026 09:39:17 +0900
Amit Langote <amitlangote09@gmail.com> wrote:

On Fri, Mar 27, 2026 at 12:56 AM Yugo Nagata <nagata@sraoss.co.jp> 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

Hi Amit, Nagata

The final patch looks good and gives more context.

Thanks for merging this.

Regards,
Surya Poondla

On Mon, 30 Mar 2026 10:16:02 +0900
Amit Langote <amitlangote09@gmail.com> wrote:

On Fri, Mar 27, 2026 at 2:36 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Fri, Mar 27, 2026 at 12:01 PM Yugo Nagata <nagata@sraoss.co.jp> wrote:

On Fri, 27 Mar 2026 09:39:17 +0900
Amit Langote <amitlangote09@gmail.com> wrote:

On Fri, Mar 27, 2026 at 12:56 AM Yugo Nagata <nagata@sraoss.co.jp> 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 <nagata@sraoss.co.jp>