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

On Mon, 29 Dec 2025 at 17:05, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> writes:
> > I noticed this problem while reviewing Tom’s patch [1]. In lsyscache.c,
> > there are multiple places that get IndexAmRoutine
> > by GetIndexAmRoutineByAmId(). Only one function get_opmethod_canorder()
> > pfree the returned object, while the other 3
> > functions get_op_index_interpretation(), equality_ops_are_compatible()
> > and comparison_ops_are_compatible() all call GetIndexAmRoutineByAmId() in
> > for loops but without freeing the memory.
>
> > While these paths are not hot enough to cause leaks that matter in
> > practice, I think for consistency, we should free the memory.
>
> I wonder if it wouldn't be better to rethink the convention that
> IndexAmRoutine structs are palloc'd. Is there any AM for which
> the contents aren't constant, so that we could just return a pointer
> to a static constant struct and save the palloc/pfree overhead?

I'm not aware of any such AMs, and the only reason I can think of to
change this dynamically is for specialization: the amroutine itself
doesn't get sufficient information to return a specialized
IndexAmRoutine pointer; and assuming that rd_indam itself isn't
`const`-ified the specializing code would just have to change the
pointed-to IndexAmRoutine instead of replacing individual am*
functions in the struct.

Requiring `const static` for IndexAMRoutine would make it more
complicated to do JIT for index AMs correctly and without resource
leaks, but such a feature would probably require more resource
management hooks than are currently available to extensions, so I
don't think we'll lose much there.

> We'd want to const-ify the result type of GetIndexAmRoutine, and
> maybe that'd result in more notational churn than it's worth,
> but it seems like we're missing a bet.
> (I would not have proposed this before we started using C99
> struct initializers. But now that we can, it doesn't seem
> like writing out the struct contents as an initializer would
> be too painful.)

Yes, let's do it.
Here's my patch that does this, pulled from [0]. It was originally
built to reduce the per-index catcache overhead; as using static const
pointers reduces the data allocated into the "index info" context by
264 bytes.

Kind regards,

Matthias van de Meent
Databricks (https://www.databricks.com)

[0]: https://www.postgresql.org/message-id/CAEze2Wi7tDidbDVJhu=Pstb2hbUXDCxx_VAZnKSqbTMf7k8+uQ@mail.gmail.com