Re: ri_Fast* crash w/ nullable UNIQUE constraint

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, Amit Langote