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

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.

--
Thanks, Amit Langote