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

Hi,

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.

Apart from that, the commit/abort/prepare cleanup and the `fn_mcxt`
ownership
looked reasonable in my testing.

Regards,
Ayush