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

On Thu, Aug 6, 2026 at 11:50 PM Amit Langote <amitlangote09@gmail.com> wrote:

Hi,

$SUBJECT was reported to me off-list.

Commit e484b0eea6 added a pfree() of the entry's FastPathMeta to
InvalidateConstraintCacheCallBack(), to fix a leak I'd noticed under
CLOBBER_CACHE_ALWAYS: ri_LoadConstraintInfo() NULLs riinfo->fpmeta
when it refills a recycled entry, so the old allocation was lost. That
was the wrong place to fix it. The comment atop the callback says:

* Note: at the time a cache invalidation message is processed there may be
* active references to the cache. Because of this we never remove entries
* from the cache, but only mark them invalid, which is harmless to active
* uses.

The fast-path metadata is subject to the same rule, and I didn't take
sufficient notice of it. ri_FastPathCheck() and
ri_FastPathBatchFlush() latch riinfo->fpmeta into a local, and
ri_FastPathFlushArray() also takes FmgrInfo pointers into it
(cast_func_finfo, eq_opr_finfo) before its "walk all matches" loop.
That loop calls index_getnext_slot(), ri_LockPKTuple(), and, for a
cross-type FK, the user's cast and equality functions. Any of those
can accept invalidation messages, so the callback can free the object
the loop is still reading, after which the loop calls through
FmgrInfos in freed memory. This doesn't require a concurrent DDL to
reach: a user-defined cast or equality function that performs DDL
itself will process the invalidation synchronously in the middle of
the loop.

To be clear on scope, the issue is the object's lifetime, not the
freshness of its contents. Stale contents are covered by the
parenthetical above: under the locks the flush already holds, nothing
that the metadata derives from can change underneath it, so an
invalidation observed mid-loop would rebuild identical metadata.

So I propose to unlink the metadata from the entry in the callback,
leaving the next check to rebuild it as it does today, and defer the
free to a point where no RI check can be on the stack -- AtEOXact_RI()
is the obvious candidate. Detached objects get chained and released
there, and the call sites that test riinfo->fpmeta == NULL don't
change. The cost is holding the memory until the end of the
transaction rather than freeing it immediately. This cost is bounded
by a few kB per detached object and reached only by transactions that
interleave DDL with FK-checking DML.

Other approaches I looked at and am not proposing: keeping the
allocation for the entry's lifetime and repopulating it in place,
which would be a smaller patch but overwrites the FmgrInfos while a
call through one of them may still be in progress, and
fmgr_info_copy() clears fn_extra, so any per-call-site state the
running function cached there disappears underneath it; having the
flush take a private copy up front, which is easy to reason about but
costs a few kB of memcpy per flush; and refcounting the cache entry so
it's freed when the last reference drops, which is what plancache does
for the analogous problem and is probably the better long-term answer,
but is more machinery than I'd want to introduce at this point in the
cycle.

I'll post a patch shortly and add an open item once this hits the archive.

Patches attached.

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]/messages/by-id/CA+HiwqHHK2D69+QqAom+th1kjZGK-0dYnrZujkEGFWpX_ZtxqQ@mail.gmail.com, 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]/messages/by-id/CA+HiwqHHK2D69+QqAom+th1kjZGK-0dYnrZujkEGFWpX_ZtxqQ@mail.gmail.com, 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]: /messages/by-id/CA+HiwqHHK2D69+QqAom+th1kjZGK-0dYnrZujkEGFWpX_ZtxqQ@mail.gmail.com

--
Thanks, Amit Langote

Hi,

On Fri, 7 Aug 2026 at 18:39, Amit Langote <amitlangote09@gmail.com> wrote:

On Thu, Aug 6, 2026 at 11:50 PM Amit Langote <amitlangote09@gmail.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]
/messages/by-id/CA+HiwqHHK2D69+QqAom+th1kjZGK-0dYnrZujkEGFWpX_ZtxqQ@mail.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

Hi Ayush,

On Tue, Aug 11, 2026 at 6:08 PM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:

On Fri, 7 Aug 2026 at 18:39, Amit Langote <amitlangote09@gmail.com> wrote:

On Thu, Aug 6, 2026 at 11:50 PM Amit Langote <amitlangote09@gmail.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] /messages/by-id/CA+HiwqHHK2D69+QqAom+th1kjZGK-0dYnrZujkEGFWpX_ZtxqQ@mail.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

On Tue, Aug 18, 2026 at 9:51 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Tue, Aug 11, 2026 at 6:08 PM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:

On Fri, 7 Aug 2026 at 18:39, Amit Langote <amitlangote09@gmail.com> wrote:

On Thu, Aug 6, 2026 at 11:50 PM Amit Langote <amitlangote09@gmail.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] /messages/by-id/CA+HiwqHHK2D69+QqAom+th1kjZGK-0dYnrZujkEGFWpX_ZtxqQ@mail.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