MIME-Version: 1.0
References: <20250331212648.ad4ab804559001d7f0788741@sraoss.co.jp>
 <CALdSSPg-rnJ27pxR=7Z20q8+Vob2dqjSStUOyWCqpRtNWKw-qQ@mail.gmail.com>
In-Reply-To: 
 <CALdSSPg-rnJ27pxR=7Z20q8+Vob2dqjSStUOyWCqpRtNWKw-qQ@mail.gmail.com>
From: Amit Langote <amitlangote09@gmail.com>
Date: Wed, 26 Nov 2025 11:48:06 +0900
Message-ID: 
 <CA+HiwqFg2RTK-VcBLCC5h8K34kNXmQAvz+qB7Wm5cENXw=utWg@mail.gmail.com>
Subject: Re: Add comments about fire_triggers argument in ri_triggers.c
To: Kirill Reshke <reshkekirill@gmail.com>
Cc: Yugo Nagata <nagata@sraoss.co.jp>, pgsql-hackers@postgresql.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CA%2BHiwqFg2RTK-VcBLCC5h8K34kNXmQAvz%2BqB7Wm5cENXw%3DutWg%40mail.gmail.com>
Precedence: bulk

Hi,

On Mon, Nov 24, 2025 at 6:23=E2=80=AFPM Kirill Reshke <reshkekirill@gmail.c=
om> 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 norma=
l case,
> > but set to false in RI triggers.
> >
> > This is introduced by 9cb84097623e in 2007. It is aimed to fire check t=
riggers
> > after all RI updates on the same row are complete.
> >
> >  However, I cannot find explanation of"why this is required" in the cod=
ebase.
> > Therefore, I've attached a patch add comments in ri_trigger.c for expla=
ining 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() a=
re used
> > only for SELECT quereis in other places. Therefore, I wonder fire_trigg=
ers 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 clea=
r.
>
> Your patch LGTM

Hmm, isn=E2=80=99t that already explained in the comment for SPI_execute_sn=
apshot()?

/*
 * SPI_execute_snapshot -- identical to SPI_execute_plan, except that we al=
low
 * the caller to specify exactly which snapshots to use, which will be
 * registered here.  Also, the caller may specify that AFTER triggers shoul=
d 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 =3D false.
 */

--=20
Thanks, Amit Langote