Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.400.21\))
Subject: Re: Eliminating SPI / SQL from some RI triggers - take 3
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: 
 <CA+HiwqF2Ma6R_QyjfPBvFreYbezFpGcwASJE0a2bM+Y0jvof+g@mail.gmail.com>
Date: Thu, 2 Apr 2026 15:59:37 +0800
Cc: Junwang Zhao <zhjwpku@gmail.com>,
 Haibo Yan <tristan.yim@gmail.com>,
 Pavel Stehule <pavel.stehule@gmail.com>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>,
 Tomas Vondra <tomas@vondra.me>
Content-Transfer-Encoding: quoted-printable
Message-Id: <C3ACBAE1-CC5F-4688-9828-4668CDE37F83@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>
 <2BE661BA-D909-4093-BF78-DB9B0C099337@gmail.com>
 <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>
 <77FA04FE-1F84-4DA1-8855-8BBFD8CC889A@gmail.com>
 <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>
To: Amit Langote <amitlangote09@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/C3ACBAE1-CC5F-4688-9828-4668CDE37F83%40gmail.com>
Precedence: bulk


> On Apr 2, 2026, at 15:41, Amit Langote <amitlangote09@gmail.com> =
wrote:
>=20
> On Wed, Apr 1, 2026 at 9:18=E2=80=AFPM Amit Langote =
<amitlangote09@gmail.com> wrote:
>> On Wed, Apr 1, 2026 at 8:56=E2=80=AFPM Junwang Zhao =
<zhjwpku@gmail.com> wrote:
>>> On Wed, Apr 1, 2026 at 5:51=E2=80=AFPM Amit Langote =
<amitlangote09@gmail.com> wrote:
>>>>=20
>>>> On Wed, Apr 1, 2026 at 5:51=E2=80=AFPM Amit Langote =
<amitlangote09@gmail.com> wrote:
>>>>> On Wed, Apr 1, 2026 at 12:54=E2=80=AFAM Junwang Zhao =
<zhjwpku@gmail.com> wrote:
>>>>>> +       if (riinfo->fpmeta =3D=3D NULL)
>>>>>> +       {
>>>>>> +               /* Reload to ensure it's valid. */
>>>>>> +               riinfo =3D =
ri_LoadConstraintInfo(riinfo->constraint_id);
>>>>>>=20
>>>>>> I was thinking of wrapping the reload in a conditional check like
>>>>>> `!riinfo->valid`, since `riinfo` can be valid even when `fpmeta =
=3D=3D NULL`.
>>>>>> However,  `if (riinfo->fpmeta =3D=3D NULL)` should rarely be =
true, so the
>>>>>> unconditional reload is harmless, and the code is cleaner.
>>>>>>=20
>>>>>> +1 to the fix.
>>>>>=20
>>>>> Thanks for checking.
>>>>>=20
>>>>> I have just pushed a slightly modified version of that.
>>>>>=20
>>>>>>> 0002 is the rebased batching patch.
>>>>>>=20
>>>>>> The change of RI_FastPathEntry from storing riinfo to fk_relid
>>>>>> makes sense to me. I'll do another review on 0002 tomorrow.
>>>>>=20
>>>>> Here's another version.
>>>>>=20
>>>>> This time, I have another fixup patch (0001) to make FastPathMeta
>>>>> self-contained by copying the FmgrInfo structs it needs out of
>>>>> RI_CompareHashEntry rather than storing pointers into it.  This =
avoids
>>>>> any dependency on those cache entries remaining stable.  I'll push
>>>>> that once the just committed patch has seen enough BF animals.
>>>>=20
>>>> Pushed.
>>>>=20
>>>>> 0002 is rebased over that.
>>>>=20
>>>> Rebased again.
>>>=20
>>> +static void
>>> +ri_FastPathBatchFlush(RI_FastPathEntry *fpentry, Relation fk_rel)
>>> +{
>>> + /* Reload; may have been invalidated since last batch =
accumulation. */
>>> + const RI_ConstraintInfo *riinfo =3D =
ri_LoadConstraintInfo(fpentry->conoid);
>>>=20
>>> ...
>>> + if (riinfo->fpmeta =3D=3D NULL)
>>> + {
>>> + /* Reload to ensure it's valid. */
>>> + riinfo =3D ri_LoadConstraintInfo(riinfo->constraint_id);
>>> + ri_populate_fastpath_metadata((RI_ConstraintInfo *) riinfo,
>>> +   fk_rel, idx_rel);
>>> + }
>>>=20
>>> ri_LoadConstraintInfo is currently invoked twice within
>>> ri_FastPathBatchFlush. Should we eliminate the second call?
>>=20
>> I think we can't because the entry may be stale by the time we get to
>> the ri_populate_fastpath_metadata() call due to intervening steps;
>> even something as benign-looking index_beginscan() may call code =
paths
>> that can trigger invalidation in rare cases. Maybe predictably in
>> CLOBBER_CACHE_ALWAYS builds.
>>=20
>>> Alternatively, we could refactor ri_FastPathBatchFlush to accept
>>> an additional parameter, `const RI_ConstraintInfo *riinfo`, so we
>>> can remove the need for the first call. In that case, we need to =
call
>>> ri_LoadConstraintInfo in ri_FastPathEndBatch.
>>=20
>> Yeah, I think that's fine.  Done that way in the attached.
>>=20
>> Also, I realized that we could do:
>>=20
>> @@ -2937,7 +2937,7 @@ ri_FastPathBatchFlush(RI_FastPathEntry =
*fpentry,
>> Relation fk_rel)
>>                                       fk_rel, idx_rel);
>>     }
>>     Assert(riinfo->fpmeta);
>> -    if (riinfo->nkeys =3D=3D 1)
>> +    if (riinfo->nkeys =3D=3D 1 && fpentry->batch_count > 1)
>>         violation_index =3D ri_FastPathFlushArray(fpentry, fk_slot, =
riinfo,
>>                                                 fk_rel, snapshot, =
scandesc);
>>=20
>> so that the fixed overhead of ri_FastPathFlushArray (allocating
>> matched[] array on stack and constructing ArrayType, etc.) is not =
paid
>> unnecessarily for single-row batches.
>=20
> There's another case in which it is not ok to use FlushArray and that
> is if the index AM's amsearcharray is false (should be true in all
> cases because the unique index used for PK is always btree).  Added an
> Assert to that effect next to where SK_SEARCHARRAY is set in
> ri_FastPathFlushArray rather than a runtime check in the dispatch
> condition.
>=20
> Patch updated.  Also added a comment about invalidation requirement or
> lack thereof for RI_FastPathEntry, rename AfterTriggerBatchIsActive()
> to simply AfterTriggerIsActive(), fixed the comments in trigger.h
> describing the callback mechanism.
>=20
> Will push tomorrow morning (Friday) barring objections.
>=20
> --=20
> Thanks, Amit Langote
> <v17-0001-Batch-FK-rows-and-use-SK_SEARCHARRAY-for-fast-pa.patch>

With a quick eyeball review, I found a typo:
```
+ * relcache invalidation.  The entry itself is torn down at batch at =
batch end
```

There are two =E2=80=9Cat batch=E2=80=9D.

I plan to spend time testing and tracing this patch tomorrow. But I =
don=E2=80=99t want to block your progress, if I find anything, I will =
report to you.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/