Subject: Add comments about fire_triggers argument in ri_triggers.c

From: Yugo Nagata <[email-masked]>
Date: Mon, 31 Mar 2025 21:26:48 +0900

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-masked]>
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,

---

From: Kirill Reshke <[email-masked]>
Date: Mon, 24 Nov 2025 14:22:56 +0500

> not needed to be false in these places, but I left them as is.

Hi!

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

Your patch LGTM

-- 
Best regards,
Kirill Reshke

---

From: Amit Langote <[email-masked]>
Date: Wed, 26 Nov 2025 11:48:06 +0900

Hi,

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

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

---

From: Amit Langote <[email-masked]>
Date: Fri, 27 Mar 2026 09:39:17 +0900

On Fri, Mar 27, 2026 at 12:56 AM Yugo Nagata <[email-masked]> 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

---

From: Amit Langote <[email-masked]>
Date: Fri, 27 Mar 2026 14:36:21 +0900

On Fri, Mar 27, 2026 at 12:01 PM Yugo Nagata <[email-masked]> wrote:
> On Fri, 27 Mar 2026 09:39:17 +0900
> Amit Langote <[email-masked]> wrote:
>
> 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

---

From: Amit Langote <[email-masked]>
Date: Mon, 30 Mar 2026 10:16:02 +0900

> 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

---

From: surya poondla <[email-masked]>
Date: Mon, 30 Mar 2026 10:34:27 -0700

Hi Amit, Nagata

The final patch looks good and gives more context.

Thanks for merging this.

Regards,
Surya Poondla