Re: ri_Fast* crash w/ nullable UNIQUE constraint

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:

https://www.postgresql.org/message-id/CA%2BHiwqFFB6vzx8v3t2%3DrbNYyxMistLf5kkJfqzJ81nadFyLrxA%40mail.gmail.com

Your analysis there is the same as mine, arrived at independently.

--
Thanks, Amit Langote