On Tue, Aug 18, 2026 at 9:51 PM Amit Langote <[email protected]> wrote: > On Tue, Aug 11, 2026 at 6:08 PM Ayush Tiwari > <[email protected]> wrote: > > On Fri, 7 Aug 2026 at 18:39, Amit Langote <[email protected]> wrote: > >> On Thu, Aug 6, 2026 at 11:50 PM Amit Langote <[email protected]> > >> wrote: > >> > > >> > Hi, > >> > > >> > $SUBJECT was reported to me off-list. > >> > >> Patches attached. > > > > Thanks for the patches! > >> > >> 0001 is what I described upthread. InvalidateConstraintCacheCallBack() > >> unlinks the metadata from the cache entry rather than freeing it, so > >> the next check rebuilds it exactly as it does today, and the detached > >> object is chained onto a list that AtEOXact_RI() releases, at which > >> point no RI check can be on the stack. The call sites that test > >> riinfo->fpmeta == NULL are unchanged. > >> > >> 0002 is what was 0004 in the series I last posted at [1], and fixes a > >> separate leak in the same struct. ri_populate_fastpath_metadata() > >> copies the cast and equality FmgrInfos with fn_mcxt set to > >> TopMemoryContext, and fn_mcxt is scratch space for the called > >> function: record_eq(), and the record I/O functions generally, > >> allocate their per-call cache there and keep a pointer to it in > >> fn_extra. That scratch space outlives the metadata, so any cached data > >> is orphaned each time the metadata is discarded. 0002 gives the > >> FmgrInfos a context of their own and deletes it alongside the struct > >> in the AtEOXact_RI() cleanup. > >> > >> 0002 must follow 0001 because there's no safe place to delete the > >> context until 0001 adds the deferred release. In the callback it would > >> be a second use-after-free, this time in memory the called function > >> owns rather than memory we do: an in-flight record_eq() has fn_extra > >> pointing into the context and fn_mcxt at the context itself, so it > >> would read back freed state or palloc into a deleted context. > >> > >> Ayush Tiwari reviewed 0002 at [1], and while doing so, independently > >> raised the same lifetime problem this thread is about and 0001 is > >> meant to fix. > >> > >> I've added open items for both, noting that the second must be > >> committed with / after the first. > >> > >> [1] > >> https://www.postgresql.org/message-id/CA%2BHiwqHHK2D69%2BQqAom%2Bth1kjZGK-0dYnrZujkEGFWpX_ZtxqQ%40mail.gmail.com > > > > > > I tried the patches and the overall deferred-free approach looks sensible to > > me. One corner case made me wonder if `fpmeta` also needs to be latched for > > the whole multi-column batch flush. > > > > `ri_FastPathFlushLoop()` calls `build_index_scankeys()` for each row, and > > that > > function reads `riinfo->fpmeta` again. In a small test where the first > > row's > > cast renames the FK constraint, the invalidation clears that field and the > > second row reaches `Assert(fpmeta)`. I may be missing another invariant > > here, > > but passing a pointer latched at the start of the flush seems to avoid it, > > much > > like the array path already does. > > Good catch. Attached v2 makes the flush functions take fpmeta as a > parameter, so only the top-level dispatch reads it from riinfo. > Interestingly your case fails earlier on unpatched master: the first > key's cast frees fpmeta and the second key then reads clobbered > memory, so it never reaches the assert you saw. With 0001 applied, it > does. I've added a regression test based on yours. > > > Apart from that, the commit/abort/prepare cleanup and the `fn_mcxt` > > ownership > > looked reasonable in my testing. > > Thanks for the review. > > I'll study this some more before committing, which I'd like to do > tomorrow. Let me know if you have more thoughts.
I have now pushed 0001 and 0002, and closed the two open items corresponding to them. -- Thanks, Amit Langote
