Re: lsyscache: free IndexAmRoutine objects returned by GetIndexAmRoutineByAmId()

Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> writes:
> On Tue, 30 Dec 2025 at 16:25, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I am not terribly concerned by one-time leaks of that sort, so
>> I don't really feel an urge to try to complain about them.

> If it's difficult to filter out one-time leaks into the context caused
> by e.g. fmgr infra, then -indeed- it's probably not worth the effort.
> In which case, v3 LGTM.

Pushed then. We can always tweak it if we think of a better answer.

I did spend a bit of time thinking about this sketch:

1. Invent a memory context support method along the lines of
bool ContextContainsPointer(MemoryContext cxt, const void *ptr)
that returns "true" if ptr points anywhere within the context's
owned memory space. The implementation I have in mind is just to
scan the context's list of blocks and see if ptr >= block_start &&
ptr < block_end for any of them. This dodges problems like whether
we can tell if the pointer points at a valid chunk. We aren't
promising that, only that the pointer points somewhere in that
memory area.

2. Remove the MemoryContextSwitchTo that's now in InitIndexAmRoutine,
reverting to running the amhandler in the caller's context.
Instead, in an assert-enabled build, throw error if
ContextContainsPointer(CurrentMemoryContext, relation->rd_indam).

This definitely will throw error if the amhandler just did a palloc(),
and it definitely will not if the amhandler returned a pointer to a
static variable. It won't throw error if the amhandler returned a
pointer to malloc'd space (which is fine now, but would have been an
error before) or space palloc'd from a different context.
In those cases we have to assume the amhandler knows what it's doing.

However, I feel slightly queasy about this idea anyway, because
relcache.c doesn't really have a lot of business assuming what
CurrentMemoryContext it's called in. Consider an extension AM that
has a non-constant IndexAmRoutine (maybe it wants to point to JITted
methods) and palloc's that in some suitably long-lived context.
Is there any way in which we could reach InitIndexAmRoutine while
running in that same long-lived context? This'd likely require a
recursive index_open call while the AM is building its result, which
is not all that hard to credit if the AM is trying to invoke JIT or
the like. However any such index_opens are probably for system
catalogs, which are not likely to be using extension AMs, so maybe the
case can't be reached after all. Nonetheless it doesn't seem quite
impossible to have a false positive, especially if the AM chooses a
common context like CacheMemoryContext or TopMemoryContext for this
purpose.

Another objection is the tedium of writing all those
ContextContainsPointer methods. I suppose that (at least for now)
we could stub out all the ones except aset.c's.

Anyway, I don't find this idea quite attractive enough to pursue,
but maybe somebody else will think differently. It'd be more
attractive if we had additional use-cases for ContextContainsPointer.

regards, tom lane