public inbox for [email protected]
help / color / mirror / Atom feedFrom: Amit Langote <[email protected]>
To: Chao Li <[email protected]>
Cc: Junwang Zhao <[email protected]>
Cc: Haibo Yan <[email protected]>
Cc: Pavel Stehule <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Cc: Tomas Vondra <[email protected]>
Subject: Re: Eliminating SPI / SQL from some RI triggers - take 3
Date: Mon, 6 Apr 2026 18:45:05 +0900
Message-ID: <CA+HiwqH39QU7vGVx65JH1e0nzVvQc5eVmuY7=qyj0T_+b-HO3A@mail.gmail.com> (raw)
In-Reply-To: <CA+HiwqFx=aciJYkkaviyTutUm303QXz6GtXSqzG7nfd4MAzddQ@mail.gmail.com>
References: <CA+HiwqF4C0ws3cO+z5cLkPuvwnAwkSp7sfvgGj3yQ=Li6KNMqA@mail.gmail.com>
<CA+HiwqGM6nvAV5O+=Nr+BXMPWOma0oeCRVzVP0XiLE8zX5TVAg@mail.gmail.com>
<CA+HiwqGMaovCUgDbGxVGnK0Mrivr+ph3YE2Ws+47-ugyPb4f7g@mail.gmail.com>
<CAFj8pRDaiBe_GOLk_yyYHTtPiDAAaLOM8u1-=Q3ZgXBTH+1igg@mail.gmail.com>
<CA+HiwqGA5Ay_MR0eJEEbt4j6WrVh4F+AasTp8yCbs5aJLOJn6Q@mail.gmail.com>
<CAEG8a3JM=NoqiTK0V6S9FNxZPvy1+C5F7rfafTtPKBVWnunL-g@mail.gmail.com>
<CA+HiwqEyiLCY6MTLbOJXDdLNNQLaURYHvdW797MQgbjEK9od4Q@mail.gmail.com>
<CAEG8a3+VBpwPf1Rm-ECD90whM9b3YnGhux5CVXdsL6khiBfzRQ@mail.gmail.com>
<CA+HiwqF2UHzF0sKCp-F2a-U29rqh_9ZPy=f1h+Fh_=M8efj3pg@mail.gmail.com>
<CAEG8a3L9Ew-WL8sxLROVOcypeaENPmd8qCmMvz4geoGL1TDGCA@mail.gmail.com>
<CAEG8a3+nUFQo4sdPQF9xy0J73J8RFJ5U9A5+_kMosGDaZ+1sXA@mail.gmail.com>
<[email protected]>
<CAEG8a3JyKdizWvYsF+z_mA1BKy=dpW11iKVMOG=bk6Tbz6M1Bw@mail.gmail.com>
<CAEG8a3+Hf4tvvbts29_k_AFhWQmRYfEo_SW4C5FY_140iKghBw@mail.gmail.com>
<CA+HiwqFV-PY-3BxM6j5TaAiC3AwedDxo-6vwRSbvygg3zF+xAQ@mail.gmail.com>
<CA+HiwqHpaisS-e+0gAgzh92qZAFxncAJMmmTRZZN=efoeTPgOg@mail.gmail.com>
<CA+HiwqFwZ6WLRbY8R7VC7JVp5Jot6ktZOkr9wDxTjoK=W1SgdQ@mail.gmail.com>
<CA+HiwqF_Kz=R8juHJBiOATvabWSOugK4VaGOxoJ_n=E2c7UM9w@mail.gmail.com>
<CA+HiwqHCB7kcbspkhaLN9enoj5x=ehzhFM4PXDgWUUP8Px41GA@mail.gmail.com>
<[email protected]>
<CA+HiwqHpVtP485wEKuXdOkdoZWhvVvfFH40-og07Jp3MPx21eg@mail.gmail.com>
<CAEG8a3JWHkJSXe9nNcVK7wnYKUEqWuMGFDhy5BynB_9tEjmEUg@mail.gmail.com>
<CA+HiwqFjfumKrWy03q5M309xJJVYt0WgGfH6AZ8BjFhSwppwsQ@mail.gmail.com>
<CAEG8a3JjP1LaKSv-r3AMJLRyLMzENJrKshWsDvDouMPM_sizmA@mail.gmail.com>
<CA+HiwqFQ+ZA7hSOygv4uv_t75B3r0_gosjadetCsAEoaZwTu6g@mail.gmail.com>
<CA+HiwqHdB0r8z6Mut13BxpYNq2W-os+Arju4mcZbCyU9PeaVog@mail.gmail.com>
<CAEG8a3K5ayVNkCDnK9OtNb+4OY0chJtW6ObgEOSFjqyymQda8Q@mail.gmail.com>
<CA+HiwqGJYCgEs_F-LBtrRdx-Y969LMr-OVogjFXU6U-0q5bOwQ@mail.gmail.com>
<CA+HiwqF2Ma6R_QyjfPBvFreYbezFpGcwASJE0a2bM+Y0jvof+g@mail.gmail.com>
<[email protected]>
<CA+HiwqGFRAH2O5bGpNMErFopFyn-2-Zu3+5y+BFQim9TE8z+Pw@mail.gmail.com>
<[email protected]>
<CA+HiwqFx=aciJYkkaviyTutUm303QXz6GtXSqzG7nfd4MAzddQ@mail.gmail.com>
On Fri, Apr 3, 2026 at 6:39 PM Amit Langote <[email protected]> wrote:
> On Fri, Apr 3, 2026 at 5:58 PM Chao Li <[email protected]> wrote:
> > > On Apr 3, 2026, at 13:52, Amit Langote <[email protected]> wrote:
> > > On Thu, Apr 2, 2026 at 5:00 PM Chao Li <[email protected]> wrote:
> > >> I plan to spend time testing and tracing this patch tomorrow. But I don’t want to block your progress, if I find anything, I will report to you.
> > >
> > > Sure, I didn't want to leave committing this to the weekend or the next week.
> >
> > I spent several hours debugging this patch today, and I found a problem where the batch mode doesn't seem to handle deferred RI triggers, although the commit message suggests that it should.
> >
> > I traced this scenario:
> > ```
> > CREATE TABLE pk (a int primary key);
> > CREATE TABLE fk (a int references pk(a) DEFERRABLE INITIALLY DEFERRED);
> > BEGIN;
> > INSERT INTO fk VALUES (1);
> > INSERT INTO pk VALUES (1);
> > COMMIT;
> > ```
> >
> > When COMMIT is executed, it reaches RI_FKey_check(), where AfterTriggerIsActive() checks whether afterTriggers.query_depth >= 0. But in the deferred case, afterTriggers.query_depth is -1.
> >
> > From the code:
> > ```
> > if (ri_fastpath_is_applicable(riinfo))
> > {
> > if (AfterTriggerIsActive())
> > {
> > /* Batched path: buffer and probe in groups */
> > ri_FastPathBatchAdd(riinfo, fk_rel, newslot);
> > }
> > else
> > {
> > /* ALTER TABLE validation: per-row, no cache */
> > ri_FastPathCheck(riinfo, fk_rel, newslot);
> > }
> > return PointerGetDatum(NULL);
> > }
> > ```
> >
> > So this ends up falling back to the per-row path for deferred RI checks at COMMIT, even though the intent here seems to be only to bypass the ALTER TABLE validation case, where batch callbacks would never fire, and MyTriggerDepth is 0. So, maybe we can just check MyTriggerDepth>0 in AfterTriggerIsActive().
> >
> > I tried the attached fix. With it, deferred triggers go through the batch mode, and all existing tests still pass.
>
> I think you might be right. Thanks for the patch. It looks correct
> to me at a glance, but I will need to check it a bit more closely
> before committing.
Thinking about this some more, your fix is on the right track but
needs a bit more work -- MyTriggerDepth > 0 is too broad since it
fires for BEFORE triggers too. I have a revised version using a new
afterTriggerFiringDepth counter that I'll push shortly.
Added an open item for tracking in the meantime:
https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items#Open_Issues
--
Thanks, Amit Langote
Attachments:
[application/octet-stream] v2-0001-Fix-deferred-FK-check-batching-introduced-by-comm.patch (5.3K, 2-v2-0001-Fix-deferred-FK-check-batching-introduced-by-comm.patch)
download | inline diff:
From c208c7cf13c6968a12e4c9b321ebeebebd931a42 Mon Sep 17 00:00:00 2001
From: Amit Langote <[email protected]>
Date: Mon, 6 Apr 2026 18:40:05 +0900
Subject: [PATCH v2] Fix deferred FK check batching introduced by commit
b7b27eb41a5
That commit introduced AfterTriggerIsActive() to detect whether
we are inside the after-trigger firing machinery, so that RI trigger
functions can take the batched fast path. It was implemented using
query_depth >= 0, which correctly identified immediate trigger firing
but missed the deferred case where query_depth is -1 at COMMIT via
AfterTriggerFireDeferred(). This caused deferred FK checks to fall
back to the per-row fast path instead of the batched path.
The correct check is whether we are inside an after-trigger firing
loop specifically. Introduce afterTriggerFiringDepth, a counter
incremented around the trigger-firing loops in AfterTriggerEndQuery,
AfterTriggerFireDeferred, and AfterTriggerSetState, and decremented
after FireAfterTriggerBatchCallbacks() returns. AfterTriggerIsActive()
now returns afterTriggerFiringDepth > 0.
Reported-by: Chao Li <[email protected]>
Author: Chao Li <[email protected]>
Co-authored-by: Amit Langote <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/commands/trigger.c | 26 ++++++++++++++++++++++----
1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 4d4e96a5302..5fe2585c88f 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -3940,6 +3940,13 @@ typedef struct AfterTriggerCallbackItem
static AfterTriggersData afterTriggers;
+/*
+ * Incremented before invoking afterTriggerInvokeEvents(). Used by
+ * AfterTriggerIsActive() to determine whether batch callbacks will fire,
+ * so that RI trigger functions can take the batched fast path.
+ */
+static int afterTriggerFiringDepth = 0;
+
static void AfterTriggerExecute(EState *estate,
AfterTriggerEvent event,
ResultRelInfo *relInfo,
@@ -5113,6 +5120,7 @@ AfterTriggerBeginXact(void)
Assert(afterTriggers.events.head == NULL);
Assert(afterTriggers.trans_stack == NULL);
Assert(afterTriggers.maxtransdepth == 0);
+ Assert(afterTriggerFiringDepth == 0);
}
@@ -5184,6 +5192,7 @@ AfterTriggerEndQuery(EState *estate)
*/
qs = &afterTriggers.query_stack[afterTriggers.query_depth];
+ afterTriggerFiringDepth++;
for (;;)
{
if (afterTriggerMarkEvents(&qs->events, &afterTriggers.events, true))
@@ -5234,6 +5243,7 @@ AfterTriggerEndQuery(EState *estate)
AfterTriggerFreeQuery(&afterTriggers.query_stack[afterTriggers.query_depth]);
afterTriggers.query_depth--;
+ afterTriggerFiringDepth--;
}
@@ -5329,6 +5339,7 @@ AfterTriggerFireDeferred(void)
* Run all the remaining triggers. Loop until they are all gone, in case
* some trigger queues more for us to do.
*/
+ afterTriggerFiringDepth++;
while (afterTriggerMarkEvents(events, NULL, false))
{
CommandId firing_id = afterTriggers.firing_counter++;
@@ -5340,6 +5351,8 @@ AfterTriggerFireDeferred(void)
/* Flush any fast-path batches accumulated by the triggers just fired. */
FireAfterTriggerBatchCallbacks();
+ afterTriggerFiringDepth--;
+
/*
* We don't bother freeing the event list, since it will go away anyway
* (and more efficiently than via pfree) in AfterTriggerEndXact.
@@ -5404,6 +5417,8 @@ AfterTriggerEndXact(bool isCommit)
/* No more afterTriggers manipulation until next transaction starts. */
afterTriggers.query_depth = -1;
+
+ afterTriggerFiringDepth = 0;
}
/*
@@ -6053,6 +6068,7 @@ AfterTriggerSetState(ConstraintsSetStmt *stmt)
AfterTriggerEventList *events = &afterTriggers.events;
bool snapshot_set = false;
+ afterTriggerFiringDepth++;
while (afterTriggerMarkEvents(events, NULL, true))
{
CommandId firing_id = afterTriggers.firing_counter++;
@@ -6086,6 +6102,7 @@ AfterTriggerSetState(ConstraintsSetStmt *stmt)
* Flush any fast-path batches accumulated by the triggers just fired.
*/
FireAfterTriggerBatchCallbacks();
+ afterTriggerFiringDepth--;
if (snapshot_set)
PopActiveSnapshot();
@@ -6806,10 +6823,10 @@ RegisterAfterTriggerBatchCallback(AfterTriggerBatchCallback callback,
* Allocate in TopTransactionContext so the item survives for the duration
* of the batch, which may span multiple trigger invocations.
*
- * Must be called while afterTriggers is active (query_depth >= 0);
- * callbacks registered outside a trigger-firing context would never fire.
+ * Must be called while afterTriggers is active; callbacks registered
+ * outside a trigger-firing context would never fire.
*/
- Assert(afterTriggers.query_depth >= 0);
+ Assert(afterTriggerFiringDepth > 0);
oldcxt = MemoryContextSwitchTo(TopTransactionContext);
item = palloc(sizeof(AfterTriggerCallbackItem));
item->callback = callback;
@@ -6836,6 +6853,7 @@ FireAfterTriggerBatchCallbacks(void)
if (afterTriggers.query_depth > 0)
return;
+ Assert(afterTriggerFiringDepth > 0);
foreach(lc, afterTriggers.batch_callbacks)
{
AfterTriggerCallbackItem *item = lfirst(lc);
@@ -6858,5 +6876,5 @@ FireAfterTriggerBatchCallbacks(void)
bool
AfterTriggerIsActive(void)
{
- return afterTriggers.query_depth >= 0;
+ return afterTriggerFiringDepth > 0;
}
--
2.47.3
view thread (63+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Eliminating SPI / SQL from some RI triggers - take 3
In-Reply-To: <CA+HiwqH39QU7vGVx65JH1e0nzVvQc5eVmuY7=qyj0T_+b-HO3A@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox