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

Hi Ayush,

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.

--
Thanks, Amit Langote