Re: Add comments about fire_triggers argument in ri_triggers.c

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