Re: ri_Fast* crash w/ nullable UNIQUE constraint

On Thu, Aug 6, 2026 at 23:59 Amit Langote <amitlangote09(at)gmail(dot)com> wrote:

> HI Ayush,
>
> On Thu, Aug 6, 2026 at 5:30 AM Ayush Tiwari <ayushtiwari(dot)slg01(at)gmail(dot)com>
> wrote:
> > On Fri, 24 Jul 2026 at 15:37, Amit Langote <amitlangote09(at)gmail(dot)com>
> wrote:
> >> On Tue, Jul 21, 2026 at 7:59 PM Ayush Tiwari
> >> <ayushtiwari(dot)slg01(at)gmail(dot)com> wrote:
> >> > 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.
> >>
> >
> > 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.
>
> Thanks for checking. I'll push 0001, 0002, 0003, 0005 and 0006
> shortly. 0004 needs more thought first -- see below.
>
> > 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?
>
> Good question, and the answer is that nothing guarantees it -- you've
> found a live bug. An invalidation really can arrive inside those
> user-supplied functions, and the pfree() that's there today then frees
> fpmeta while ri_FastPathFlushArray() is still reading it and calling
> through FmgrInfos in it. 0004 makes that worse rather than better,
> since MemoryContextDelete() next to the pfree() has the same problem.
> So I'll hold 0004 back for now.
>
> I'd started looking at this from a report off-list and posted it
> separately just before your mail arrived:

I meant to write “before I had the chance to read your email properly” ;-).

- Amit