On Thu, Aug 6, 2026 at 23:59 Amit Langote <[email protected]> wrote:
> HI Ayush, > > On Thu, Aug 6, 2026 at 5:30 AM Ayush Tiwari <[email protected]> > wrote: > > On Fri, 24 Jul 2026 at 15:37, Amit Langote <[email protected]> > wrote: > >> On Tue, Jul 21, 2026 at 7:59 PM Ayush Tiwari > >> <[email protected]> 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
