Обсуждение: Add comments about fire_triggers argument in ri_triggers.c
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>
Вложения
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