On Wed, Apr 1, 2026 at 8:56 PM Junwang Zhao <[email protected]> wrote: > On Wed, Apr 1, 2026 at 5:51 PM Amit Langote <[email protected]> wrote: > > > > On Wed, Apr 1, 2026 at 5:51 PM Amit Langote <[email protected]> wrote: > > > On Wed, Apr 1, 2026 at 12:54 AM Junwang Zhao <[email protected]> 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.
Attached patch is updated that way.
I will continue looking at this tomorrow morning with the aim of
committing it by EOD or Friday.
--
Thanks, Amit Langote
v16-0001-Batch-FK-rows-and-use-SK_SEARCHARRAY-for-fast-pa.patch
Description: Binary data
