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
