Re: Add comments about fire_triggers argument in ri_triggers.c

Hi,

On Mon, Nov 24, 2025 at 6:23 PM Kirill Reshke <reshkekirill(at)gmail(dot)com> wrote:
> On Mon, 31 Mar 2025 at 17:27, Yugo Nagata <nagata(at)sraoss(dot)co(dot)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