Re: ri_Fast* crash w/ nullable UNIQUE constraint

Hi,

On Fri, 24 Jul 2026 at 15:37, Amit Langote <amitlangote09(at)gmail(dot)com> wrote:

> Hi Ayush,
>
> On Tue, Jul 21, 2026 at 7:59 PM Ayush Tiwari
> <ayushtiwari(dot)slg01(at)gmail(dot)com> wrote:
> > On Tue, 21 Jul 2026 at 11:27, Amit Langote <amitlangote09(at)gmail(dot)com>
> wrote:
> >> I plan to go on vacation starting Sunday for a couple of weeks, so I
> >> was hoping to push these patches this week after taking another look.
> >> If you or anyone else has comments, please let me know.
> >
> > Thanks for the patches!
> >
> > I was reviewing it yesterday and got stalled on 0004, went through
> > rest today. 0001 to 0003 looked good to me.
>
> Thanks for the review.
>
> > One thing I got stuck on in 0004 (resettable fn_mcxt): each flush does
> > MemoryContextReset(scratch_cxt), but I couldn't find where the cached
> > FmgrInfos' fn_extra is cleared. record_eq() (and some cast/eq
> > functions) cache state via fn_extra allocated in fn_mcxt, so after the
> > reset fn_extra seems to dangle, and the next flush reusing the same
> > FmgrInfo reads it back as valid; fmgr_info_copy() zeroes fn_extra for
> > what looks like this reason. With a composite key over two batches,
> > and an assert after the reset, fn_extra was non-NULL on the second
> > batch. Would clearing fn_extra (or re-copying the FmgrInfos) on reset
> > make sense, maybe with a record-typed two-batch test?
>
> Good catch. I don't think scratch_cxt should be reset per flush at all
> -- fn_extra points into it, so the reset frees state fn_extra still
> refers to. Clearing fn_extra too would fix the dangling pointer but
> discard the cache every flush, and nothing accumulates during use
> anyway: record_eq() allocates only when fn_extra is NULL.
>
> The actual leak is at invalidation, not during use: with fn_mcxt =
> TopMemoryContext the cached state outlives fpmeta, so each
> repopulation orphans the previous one. v2 keeps the context but never
> resets it, deleting it with fpmeta in
> InvalidateConstraintCacheCallBack().
>
> I don't think we need a regression test here, since the fix only
> prevents a leak.
>
> > Also, the reset is only in ri_FastPathBatchFlush(); ri_FastPathCheck()
> > (ALTER TABLE validate / sub-transaction path) uses the same cached
> > FmgrInfos and scratch_cxt but doesn't reset. I wonder if the growth this
> > patch targets still applies there.
>
> Yes, that applies there too. The problem isn't anything piling up
> across flushes, it's that the cached state gets left behind when
> fpmeta goes away, so the calling path doesn't come into it at all.
>
> > Doubt: 0005 moves index_beginscan() after the user-id switch in
> > ri_FastPathCheck(), but ri_FastPathBatchFlush() still begins the scan
> > before the switch. Is that intentional, or worth the same reordering?
>
> Fixed, both sites are consistent now.
>
> Updated patches attached. I don't think much time is left between now
> and when I board a plane, so I will commit these when I'm back from
> vacation; Aug 5 or 6.
>

Thanks for the updated patches!

I went through v2. 0001, 0002, 0003, 0005 and 0006 look good to me, and
I agree with not resetting the context in 0004.

One small question on 0004. It adds a MemoryContextDelete to
InvalidateConstraintCacheCallBack(), next to the existing pfree(fpmeta).
The comment above that function says entries are never removed, only
marked invalid, because there may be active references at that point.
ri_FastPathFlushArray() and build_index_scankeys() do hold fpmeta, and
pointers into it, across the user supplied cast and equality functions.
ISTM something must keep an invalidation from arriving there, but I
could not work out what. Is that guaranteed somewhere, and would a
comment help?

Regards,
Ayush