Hi Ayush, 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. -- Thanks, Amit Langote
v2-0001-Don-t-free-fast-path-FK-metadata-from-the-inval-c.patch
Description: Binary data
v2-0002-Give-RI-fast-path-cached-FmgrInfos-their-own-memo.patch
Description: Binary data
