Re: InvalidateConstraintCacheCallBack() can free fpmeta while it's in use

On Tue, Aug 18, 2026 at 9:51 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Tue, Aug 11, 2026 at 6:08 PM Ayush Tiwari
> <ayushtiwari(dot)slg01(at)gmail(dot)com> wrote:
> > On Fri, 7 Aug 2026 at 18:39, Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> >> On Thu, Aug 6, 2026 at 11:50 PM Amit Langote <amitlangote09(at)gmail(dot)com> 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