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

Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> writes:
> I understand that switching to rd_indexcxt is intended to mitigate the risk that external index AMs might still palloc the returned IndexAmRoutine.

> However, I don’t see a way to actually enforce external AMs to always return a static pointer. In particular, if an AM switches to a different memory context internally, the MemoryContextSwitchTo() here would not help.

I do not think we need to "enforce" that, and as you say it'd be quite
hard to do so. The point of this MemoryContextSwitchTo is just to
allow existing AMs that naively do palloc() as they were told will
continue to work (modulo an increased chance of memory-leak issues
since we removed some pfree's). If we don't do the switching, a
non-updated AM will fail in very hard-to-diagnose ways once one
of its rd_indam pointers becomes dangling because the context
that was active at relcache-entry creation time has gone away.
I think it's worth a small number of cycles to save external AM
authors from having that debugging experience.

I did test this, by dint of installing all the changes except the
ones in the amhandler functions. That still passed check-world.
I didn't attempt to analyze whether there were any new leaks of
any significance.

The main objection that could be raised to this is that the old coding
ensures that any memory leaked during GetIndexAmRoutine() will be
leaked in the expected-to-be-short-lived caller's context, but now
it'd be leaked in the cache's rd_indexcxt. I don't believe that
fmgr_info can leak any memory when calling a built-in function, but
for extension functions there will be a syscache lookup and that
could potentially have some incidental leaks. All in all though,
I think this is a good tradeoff. We could perhaps remove those
MemoryContextSwitchTos in a few years when we think everybody's
updated their index AMs.

regards, tom lane