Re: Eliminating SPI / SQL from some RI triggers - take 3

> On Apr 2, 2026, at 15:41, Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>
> On Wed, Apr 1, 2026 at 9:18 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>> On Wed, Apr 1, 2026 at 8:56 PM Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
>>> On Wed, Apr 1, 2026 at 5:51 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>>>>
>>>> On Wed, Apr 1, 2026 at 5:51 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>>>>> On Wed, Apr 1, 2026 at 12:54 AM Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
>>>>>> + if (riinfo->fpmeta == NULL)
>>>>>> + {
>>>>>> + /* Reload to ensure it's valid. */
>>>>>> + riinfo = ri_LoadConstraintInfo(riinfo->constraint_id);
>>>>>>
>>>>>> I was thinking of wrapping the reload in a conditional check like
>>>>>> `!riinfo->valid`, since `riinfo` can be valid even when `fpmeta == NULL`.
>>>>>> However, `if (riinfo->fpmeta == NULL)` should rarely be true, so the
>>>>>> unconditional reload is harmless, and the code is cleaner.
>>>>>>
>>>>>> +1 to the fix.
>>>>>
>>>>> Thanks for checking.
>>>>>
>>>>> I have just pushed a slightly modified version of that.
>>>>>
>>>>>>> 0002 is the rebased batching patch.
>>>>>>
>>>>>> The change of RI_FastPathEntry from storing riinfo to fk_relid
>>>>>> makes sense to me. I'll do another review on 0002 tomorrow.
>>>>>
>>>>> Here's another version.
>>>>>
>>>>> 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.
>>>>
>>>> Pushed.
>>>>
>>>>> 0002 is rebased over that.
>>>>
>>>> Rebased again.
>>>
>>> +static void
>>> +ri_FastPathBatchFlush(RI_FastPathEntry *fpentry, Relation fk_rel)
>>> +{
>>> + /* Reload; may have been invalidated since last batch accumulation. */
>>> + const RI_ConstraintInfo *riinfo = ri_LoadConstraintInfo(fpentry->conoid);
>>>
>>> ...
>>> + if (riinfo->fpmeta == NULL)
>>> + {
>>> + /* Reload to ensure it's valid. */
>>> + riinfo = ri_LoadConstraintInfo(riinfo->constraint_id);
>>> + ri_populate_fastpath_metadata((RI_ConstraintInfo *) riinfo,
>>> + fk_rel, idx_rel);
>>> + }
>>>
>>> ri_LoadConstraintInfo is currently invoked twice within
>>> ri_FastPathBatchFlush. Should we eliminate the second call?
>>
>> 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.
>>
>>> 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.
>>
>> Yeah, I think that's fine. Done that way in the attached.
>>
>> Also, I realized that we could do:
>>
>> @@ -2937,7 +2937,7 @@ ri_FastPathBatchFlush(RI_FastPathEntry *fpentry,
>> Relation fk_rel)
>> fk_rel, idx_rel);
>> }
>> Assert(riinfo->fpmeta);
>> - if (riinfo->nkeys == 1)
>> + if (riinfo->nkeys == 1 && fpentry->batch_count > 1)
>> violation_index = ri_FastPathFlushArray(fpentry, fk_slot, riinfo,
>> fk_rel, snapshot, scandesc);
>>
>> 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.
>
> 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.
>
> 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.
>
> Will push tomorrow morning (Friday) barring objections.
>
> --
> 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 “at batch”.

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.

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