Обсуждение: lsyscache: free IndexAmRoutine objects returned by GetIndexAmRoutineByAmId()
Hi Hackers,
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.
Вложения
Chao Li <li.evan.chao@gmail.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?
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.)
regards, tom lane
Re: lsyscache: free IndexAmRoutine objects returned by GetIndexAmRoutineByAmId()
От
Matthias van de Meent
Дата:
On Mon, 29 Dec 2025 at 17:05, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Chao Li <li.evan.chao@gmail.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
Вложения
Matthias van de Meent <boekewurm+postgres@gmail.com> writes:
> On Mon, 29 Dec 2025 at 17:05, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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.
Yeah, it's hard to credit any need for per-call specialization
given that the amhandler receives no parameters. I can imagine
per-session specialization, for instance as a result of wanting
to JIT-compile the AM's methods. But you could still do that:
on first call, build one copy of the IndexAMRoutine struct in
a long-lived context, and then just keep returning pointers to
that struct. The "const" requirement applies to the core code's
references to the IndexAMRoutine struct, it does not constrain
the code creating such a struct.
> 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.
Cool, I forgot you'd already been poking at this. The patch
is kinda long, but not as bad as I feared, and it all looks
quite mechanical. It lacks documentation updates though.
regards, tom lane
... oh, one other thought: instead of what you did in
InitIndexAmRoutine, we should probably do something like
{
MemoryContext oldcontext;
/*
* We formerly specified that the amhandler should return a
* palloc'd struct. That's now deprecated in favor of returning
* a pointer to a static struct, but to avoid completely breaking
* old external AMs, run the amhandler in the relation's rd_indexcxt.
*/
oldcontext = MemoryContextSwitchTo(relation->rd_indexcxt);
relation->rd_indam = GetIndexAmRoutine(relation->rd_amhandler);
MemoryContextSwitchTo(oldcontext);
}
regards, tom lane
Re: lsyscache: free IndexAmRoutine objects returned by GetIndexAmRoutineByAmId()
От
Matthias van de Meent
Дата:
On Mon, 29 Dec 2025 at 20:01, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Matthias van de Meent <boekewurm+postgres@gmail.com> writes >> 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. > > Cool, I forgot you'd already been poking at this. The patch > is kinda long, but not as bad as I feared, and it all looks > quite mechanical. It lacks documentation updates though. Attached v2, in which I've updated the one place where I could find IndexAmRoutine's allocation policy being described, and in which I've also updated InitIndexAmRoutine with the suggested changes of your other mail. Thanks! Kind regards, Matthias van de Meent Databricks (https://www.databricks.com)
Вложения
Matthias van de Meent <boekewurm+postgres@gmail.com> writes:
> Attached v2, in which I've updated the one place where I could find
> IndexAmRoutine's allocation policy being described, and in which I've
> also updated InitIndexAmRoutine with the suggested changes of your
> other mail. Thanks!
I went through this and made some mostly-cosmetic changes.
I think the attached v3 is ready to go, but I'll wait a day
or so to see if anyone has any comments.
regards, tom lane
From 8c0761af39762597fa1d2e1cc6e9d7cb5059aa52 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Mon, 29 Dec 2025 17:49:44 -0500
Subject: [PATCH v3] Change IndexAmRoutines to be statically-allocated structs.
Up to now, index amhandlers were expected to produce a new, palloc'd
struct on each call. That requires palloc/pfree overhead, and creates
a risk of memory leaks if the caller fails to pfree, and the time
taken to fill such a large structure isn't nil. Moreover, we were
storing these things in the relcache, eating several hundred bytes for
each cached index. There is not anything in these structs that needs
to vary at runtime, so let's change the definition so that an
amhandler can return a pointer to a "static const" struct of which
there's only one copy per index AM. Mark all the core code's
IndexAmRoutine pointers const so that we catch anyplace that might
still try to change or pfree one.
(This is similar to the way we were already handling TableAmRoutine
structs. This commit does fix one comment that was infelicitously
copied-and-pasted into tableamapi.c.)
This commit needs to be called out in the v19 release notes as an API
change for extension index AMs. An un-updated AM will still work
(as of now, anyway) but it risks memory leaks and will be slower than
necessary.
Author: Matthias van de Meent <boekewurm+postgres@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAEoWx2=vApYk2LRu8R0DdahsPNEhWUxGBZ=rbZo1EXE=uA+opQ@mail.gmail.com
---
contrib/bloom/blutils.c | 111 ++++++++---------
doc/src/sgml/indexam.sgml | 12 +-
src/backend/access/brin/brin.c | 113 +++++++++---------
src/backend/access/gin/ginutil.c | 109 ++++++++---------
src/backend/access/gist/gist.c | 113 +++++++++---------
src/backend/access/hash/hash.c | 113 +++++++++---------
src/backend/access/index/amapi.c | 18 ++-
src/backend/access/nbtree/nbtree.c | 113 +++++++++---------
src/backend/access/spgist/spgutils.c | 113 +++++++++---------
src/backend/access/table/tableamapi.c | 5 +-
src/backend/catalog/index.c | 4 +-
src/backend/commands/indexcmds.c | 5 +-
src/backend/commands/opclasscmds.c | 10 +-
src/backend/executor/execAmi.c | 3 +-
src/backend/optimizer/util/plancat.c | 2 +-
src/backend/utils/adt/amutils.c | 4 +-
src/backend/utils/adt/ruleutils.c | 2 +-
src/backend/utils/cache/lsyscache.c | 35 ++----
src/backend/utils/cache/relcache.c | 21 ++--
src/include/access/amapi.h | 8 +-
src/include/utils/rel.h | 2 +-
.../modules/dummy_index_am/dummy_index_am.c | 101 ++++++++--------
22 files changed, 501 insertions(+), 516 deletions(-)
diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c
index 7a468b4a173..50f461fa540 100644
--- a/contrib/bloom/blutils.c
+++ b/contrib/bloom/blutils.c
@@ -102,61 +102,62 @@ makeDefaultBloomOptions(void)
Datum
blhandler(PG_FUNCTION_ARGS)
{
- IndexAmRoutine *amroutine = makeNode(IndexAmRoutine);
-
- amroutine->amstrategies = BLOOM_NSTRATEGIES;
- amroutine->amsupport = BLOOM_NPROC;
- amroutine->amoptsprocnum = BLOOM_OPTIONS_PROC;
- amroutine->amcanorder = false;
- amroutine->amcanorderbyop = false;
- amroutine->amcanhash = false;
- amroutine->amconsistentequality = false;
- amroutine->amconsistentordering = false;
- amroutine->amcanbackward = false;
- amroutine->amcanunique = false;
- amroutine->amcanmulticol = true;
- amroutine->amoptionalkey = true;
- amroutine->amsearcharray = false;
- amroutine->amsearchnulls = false;
- amroutine->amstorage = false;
- amroutine->amclusterable = false;
- amroutine->ampredlocks = false;
- amroutine->amcanparallel = false;
- amroutine->amcanbuildparallel = false;
- amroutine->amcaninclude = false;
- amroutine->amusemaintenanceworkmem = false;
- amroutine->amparallelvacuumoptions =
- VACUUM_OPTION_PARALLEL_BULKDEL | VACUUM_OPTION_PARALLEL_CLEANUP;
- amroutine->amkeytype = InvalidOid;
-
- amroutine->ambuild = blbuild;
- amroutine->ambuildempty = blbuildempty;
- amroutine->aminsert = blinsert;
- amroutine->aminsertcleanup = NULL;
- amroutine->ambulkdelete = blbulkdelete;
- amroutine->amvacuumcleanup = blvacuumcleanup;
- amroutine->amcanreturn = NULL;
- amroutine->amcostestimate = blcostestimate;
- amroutine->amgettreeheight = NULL;
- amroutine->amoptions = bloptions;
- amroutine->amproperty = NULL;
- amroutine->ambuildphasename = NULL;
- amroutine->amvalidate = blvalidate;
- amroutine->amadjustmembers = NULL;
- amroutine->ambeginscan = blbeginscan;
- amroutine->amrescan = blrescan;
- amroutine->amgettuple = NULL;
- amroutine->amgetbitmap = blgetbitmap;
- amroutine->amendscan = blendscan;
- amroutine->ammarkpos = NULL;
- amroutine->amrestrpos = NULL;
- amroutine->amestimateparallelscan = NULL;
- amroutine->aminitparallelscan = NULL;
- amroutine->amparallelrescan = NULL;
- amroutine->amtranslatestrategy = NULL;
- amroutine->amtranslatecmptype = NULL;
-
- PG_RETURN_POINTER(amroutine);
+ static const IndexAmRoutine amroutine = {
+ .type = T_IndexAmRoutine,
+ .amstrategies = BLOOM_NSTRATEGIES,
+ .amsupport = BLOOM_NPROC,
+ .amoptsprocnum = BLOOM_OPTIONS_PROC,
+ .amcanorder = false,
+ .amcanorderbyop = false,
+ .amcanhash = false,
+ .amconsistentequality = false,
+ .amconsistentordering = false,
+ .amcanbackward = false,
+ .amcanunique = false,
+ .amcanmulticol = true,
+ .amoptionalkey = true,
+ .amsearcharray = false,
+ .amsearchnulls = false,
+ .amstorage = false,
+ .amclusterable = false,
+ .ampredlocks = false,
+ .amcanparallel = false,
+ .amcanbuildparallel = false,
+ .amcaninclude = false,
+ .amusemaintenanceworkmem = false,
+ .amparallelvacuumoptions =
+ VACUUM_OPTION_PARALLEL_BULKDEL | VACUUM_OPTION_PARALLEL_CLEANUP,
+ .amkeytype = InvalidOid,
+
+ .ambuild = blbuild,
+ .ambuildempty = blbuildempty,
+ .aminsert = blinsert,
+ .aminsertcleanup = NULL,
+ .ambulkdelete = blbulkdelete,
+ .amvacuumcleanup = blvacuumcleanup,
+ .amcanreturn = NULL,
+ .amcostestimate = blcostestimate,
+ .amgettreeheight = NULL,
+ .amoptions = bloptions,
+ .amproperty = NULL,
+ .ambuildphasename = NULL,
+ .amvalidate = blvalidate,
+ .amadjustmembers = NULL,
+ .ambeginscan = blbeginscan,
+ .amrescan = blrescan,
+ .amgettuple = NULL,
+ .amgetbitmap = blgetbitmap,
+ .amendscan = blendscan,
+ .ammarkpos = NULL,
+ .amrestrpos = NULL,
+ .amestimateparallelscan = NULL,
+ .aminitparallelscan = NULL,
+ .amparallelrescan = NULL,
+ .amtranslatestrategy = NULL,
+ .amtranslatecmptype = NULL,
+ };
+
+ PG_RETURN_POINTER(&amroutine);
}
/*
diff --git a/doc/src/sgml/indexam.sgml b/doc/src/sgml/indexam.sgml
index 63d7e376f19..f48da318530 100644
--- a/doc/src/sgml/indexam.sgml
+++ b/doc/src/sgml/indexam.sgml
@@ -70,9 +70,15 @@
single argument of type <type>internal</type> and to return the
pseudo-type <type>index_am_handler</type>. The argument is a dummy value that
simply serves to prevent handler functions from being called directly from
- SQL commands. The result of the function must be a palloc'd struct of
- type <structname>IndexAmRoutine</structname>, which contains everything
- that the core code needs to know to make use of the index access method.
+ SQL commands.
+ The result of the handler function must be a pointer to a permanently
+ allocated struct of type <structname>IndexAmRoutine</structname>, which
+ contains everything that the core code needs to know to make use of the
+ index access method. (Typically this struct can be pre-initialized and
+ marked <literal>static const</literal>; but if that is inconvenient,
+ build it in some long-lived context. In any case, repeat calls within
+ a process should return the same pointer. The core code will treat the
+ struct as <literal>const</literal>, and will never free it.)
The <structname>IndexAmRoutine</structname> struct, also called the access
method's <firstterm>API struct</firstterm>, includes fields specifying assorted
fixed properties of the access method, such as whether it can support
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 45d306037a4..ec5086a9521 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -249,62 +249,63 @@ static void _brin_parallel_scan_and_build(BrinBuildState *state,
Datum
brinhandler(PG_FUNCTION_ARGS)
{
- IndexAmRoutine *amroutine = makeNode(IndexAmRoutine);
-
- amroutine->amstrategies = 0;
- amroutine->amsupport = BRIN_LAST_OPTIONAL_PROCNUM;
- amroutine->amoptsprocnum = BRIN_PROCNUM_OPTIONS;
- amroutine->amcanorder = false;
- amroutine->amcanorderbyop = false;
- amroutine->amcanhash = false;
- amroutine->amconsistentequality = false;
- amroutine->amconsistentordering = false;
- amroutine->amcanbackward = false;
- amroutine->amcanunique = false;
- amroutine->amcanmulticol = true;
- amroutine->amoptionalkey = true;
- amroutine->amsearcharray = false;
- amroutine->amsearchnulls = true;
- amroutine->amstorage = true;
- amroutine->amclusterable = false;
- amroutine->ampredlocks = false;
- amroutine->amcanparallel = false;
- amroutine->amcanbuildparallel = true;
- amroutine->amcaninclude = false;
- amroutine->amusemaintenanceworkmem = false;
- amroutine->amsummarizing = true;
- amroutine->amparallelvacuumoptions =
- VACUUM_OPTION_PARALLEL_CLEANUP;
- amroutine->amkeytype = InvalidOid;
-
- amroutine->ambuild = brinbuild;
- amroutine->ambuildempty = brinbuildempty;
- amroutine->aminsert = brininsert;
- amroutine->aminsertcleanup = brininsertcleanup;
- amroutine->ambulkdelete = brinbulkdelete;
- amroutine->amvacuumcleanup = brinvacuumcleanup;
- amroutine->amcanreturn = NULL;
- amroutine->amcostestimate = brincostestimate;
- amroutine->amgettreeheight = NULL;
- amroutine->amoptions = brinoptions;
- amroutine->amproperty = NULL;
- amroutine->ambuildphasename = NULL;
- amroutine->amvalidate = brinvalidate;
- amroutine->amadjustmembers = NULL;
- amroutine->ambeginscan = brinbeginscan;
- amroutine->amrescan = brinrescan;
- amroutine->amgettuple = NULL;
- amroutine->amgetbitmap = bringetbitmap;
- amroutine->amendscan = brinendscan;
- amroutine->ammarkpos = NULL;
- amroutine->amrestrpos = NULL;
- amroutine->amestimateparallelscan = NULL;
- amroutine->aminitparallelscan = NULL;
- amroutine->amparallelrescan = NULL;
- amroutine->amtranslatestrategy = NULL;
- amroutine->amtranslatecmptype = NULL;
-
- PG_RETURN_POINTER(amroutine);
+ static const IndexAmRoutine amroutine = {
+ .type = T_IndexAmRoutine,
+ .amstrategies = 0,
+ .amsupport = BRIN_LAST_OPTIONAL_PROCNUM,
+ .amoptsprocnum = BRIN_PROCNUM_OPTIONS,
+ .amcanorder = false,
+ .amcanorderbyop = false,
+ .amcanhash = false,
+ .amconsistentequality = false,
+ .amconsistentordering = false,
+ .amcanbackward = false,
+ .amcanunique = false,
+ .amcanmulticol = true,
+ .amoptionalkey = true,
+ .amsearcharray = false,
+ .amsearchnulls = true,
+ .amstorage = true,
+ .amclusterable = false,
+ .ampredlocks = false,
+ .amcanparallel = false,
+ .amcanbuildparallel = true,
+ .amcaninclude = false,
+ .amusemaintenanceworkmem = false,
+ .amsummarizing = true,
+ .amparallelvacuumoptions =
+ VACUUM_OPTION_PARALLEL_CLEANUP,
+ .amkeytype = InvalidOid,
+
+ .ambuild = brinbuild,
+ .ambuildempty = brinbuildempty,
+ .aminsert = brininsert,
+ .aminsertcleanup = brininsertcleanup,
+ .ambulkdelete = brinbulkdelete,
+ .amvacuumcleanup = brinvacuumcleanup,
+ .amcanreturn = NULL,
+ .amcostestimate = brincostestimate,
+ .amgettreeheight = NULL,
+ .amoptions = brinoptions,
+ .amproperty = NULL,
+ .ambuildphasename = NULL,
+ .amvalidate = brinvalidate,
+ .amadjustmembers = NULL,
+ .ambeginscan = brinbeginscan,
+ .amrescan = brinrescan,
+ .amgettuple = NULL,
+ .amgetbitmap = bringetbitmap,
+ .amendscan = brinendscan,
+ .ammarkpos = NULL,
+ .amrestrpos = NULL,
+ .amestimateparallelscan = NULL,
+ .aminitparallelscan = NULL,
+ .amparallelrescan = NULL,
+ .amtranslatestrategy = NULL,
+ .amtranslatecmptype = NULL,
+ };
+
+ PG_RETURN_POINTER(&amroutine);
}
/*
diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c
index 605f80aad39..e4589de2aab 100644
--- a/src/backend/access/gin/ginutil.c
+++ b/src/backend/access/gin/ginutil.c
@@ -37,60 +37,61 @@
Datum
ginhandler(PG_FUNCTION_ARGS)
{
- IndexAmRoutine *amroutine = makeNode(IndexAmRoutine);
-
- amroutine->amstrategies = 0;
- amroutine->amsupport = GINNProcs;
- amroutine->amoptsprocnum = GIN_OPTIONS_PROC;
- amroutine->amcanorder = false;
- amroutine->amcanorderbyop = false;
- amroutine->amcanhash = false;
- amroutine->amconsistentequality = false;
- amroutine->amconsistentordering = false;
- amroutine->amcanbackward = false;
- amroutine->amcanunique = false;
- amroutine->amcanmulticol = true;
- amroutine->amoptionalkey = true;
- amroutine->amsearcharray = false;
- amroutine->amsearchnulls = false;
- amroutine->amstorage = true;
- amroutine->amclusterable = false;
- amroutine->ampredlocks = true;
- amroutine->amcanparallel = false;
- amroutine->amcanbuildparallel = true;
- amroutine->amcaninclude = false;
- amroutine->amusemaintenanceworkmem = true;
- amroutine->amsummarizing = false;
- amroutine->amparallelvacuumoptions =
- VACUUM_OPTION_PARALLEL_BULKDEL | VACUUM_OPTION_PARALLEL_CLEANUP;
- amroutine->amkeytype = InvalidOid;
-
- amroutine->ambuild = ginbuild;
- amroutine->ambuildempty = ginbuildempty;
- amroutine->aminsert = gininsert;
- amroutine->aminsertcleanup = NULL;
- amroutine->ambulkdelete = ginbulkdelete;
- amroutine->amvacuumcleanup = ginvacuumcleanup;
- amroutine->amcanreturn = NULL;
- amroutine->amcostestimate = gincostestimate;
- amroutine->amgettreeheight = NULL;
- amroutine->amoptions = ginoptions;
- amroutine->amproperty = NULL;
- amroutine->ambuildphasename = ginbuildphasename;
- amroutine->amvalidate = ginvalidate;
- amroutine->amadjustmembers = ginadjustmembers;
- amroutine->ambeginscan = ginbeginscan;
- amroutine->amrescan = ginrescan;
- amroutine->amgettuple = NULL;
- amroutine->amgetbitmap = gingetbitmap;
- amroutine->amendscan = ginendscan;
- amroutine->ammarkpos = NULL;
- amroutine->amrestrpos = NULL;
- amroutine->amestimateparallelscan = NULL;
- amroutine->aminitparallelscan = NULL;
- amroutine->amparallelrescan = NULL;
-
- PG_RETURN_POINTER(amroutine);
+ static const IndexAmRoutine amroutine = {
+ .type = T_IndexAmRoutine,
+ .amstrategies = 0,
+ .amsupport = GINNProcs,
+ .amoptsprocnum = GIN_OPTIONS_PROC,
+ .amcanorder = false,
+ .amcanorderbyop = false,
+ .amcanhash = false,
+ .amconsistentequality = false,
+ .amconsistentordering = false,
+ .amcanbackward = false,
+ .amcanunique = false,
+ .amcanmulticol = true,
+ .amoptionalkey = true,
+ .amsearcharray = false,
+ .amsearchnulls = false,
+ .amstorage = true,
+ .amclusterable = false,
+ .ampredlocks = true,
+ .amcanparallel = false,
+ .amcanbuildparallel = true,
+ .amcaninclude = false,
+ .amusemaintenanceworkmem = true,
+ .amsummarizing = false,
+ .amparallelvacuumoptions =
+ VACUUM_OPTION_PARALLEL_BULKDEL | VACUUM_OPTION_PARALLEL_CLEANUP,
+ .amkeytype = InvalidOid,
+
+ .ambuild = ginbuild,
+ .ambuildempty = ginbuildempty,
+ .aminsert = gininsert,
+ .aminsertcleanup = NULL,
+ .ambulkdelete = ginbulkdelete,
+ .amvacuumcleanup = ginvacuumcleanup,
+ .amcanreturn = NULL,
+ .amcostestimate = gincostestimate,
+ .amgettreeheight = NULL,
+ .amoptions = ginoptions,
+ .amproperty = NULL,
+ .ambuildphasename = ginbuildphasename,
+ .amvalidate = ginvalidate,
+ .amadjustmembers = ginadjustmembers,
+ .ambeginscan = ginbeginscan,
+ .amrescan = ginrescan,
+ .amgettuple = NULL,
+ .amgetbitmap = gingetbitmap,
+ .amendscan = ginendscan,
+ .ammarkpos = NULL,
+ .amrestrpos = NULL,
+ .amestimateparallelscan = NULL,
+ .aminitparallelscan = NULL,
+ .amparallelrescan = NULL,
+ };
+
+ PG_RETURN_POINTER(&amroutine);
}
/*
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index c26d8538f05..4b943b7f43e 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -58,62 +58,63 @@ static void gistprunepage(Relation rel, Page page, Buffer buffer,
Datum
gisthandler(PG_FUNCTION_ARGS)
{
- IndexAmRoutine *amroutine = makeNode(IndexAmRoutine);
-
- amroutine->amstrategies = 0;
- amroutine->amsupport = GISTNProcs;
- amroutine->amoptsprocnum = GIST_OPTIONS_PROC;
- amroutine->amcanorder = false;
- amroutine->amcanorderbyop = true;
- amroutine->amcanhash = false;
- amroutine->amconsistentequality = false;
- amroutine->amconsistentordering = false;
- amroutine->amcanbackward = false;
- amroutine->amcanunique = false;
- amroutine->amcanmulticol = true;
- amroutine->amoptionalkey = true;
- amroutine->amsearcharray = false;
- amroutine->amsearchnulls = true;
- amroutine->amstorage = true;
- amroutine->amclusterable = true;
- amroutine->ampredlocks = true;
- amroutine->amcanparallel = false;
- amroutine->amcanbuildparallel = false;
- amroutine->amcaninclude = true;
- amroutine->amusemaintenanceworkmem = false;
- amroutine->amsummarizing = false;
- amroutine->amparallelvacuumoptions =
- VACUUM_OPTION_PARALLEL_BULKDEL | VACUUM_OPTION_PARALLEL_COND_CLEANUP;
- amroutine->amkeytype = InvalidOid;
-
- amroutine->ambuild = gistbuild;
- amroutine->ambuildempty = gistbuildempty;
- amroutine->aminsert = gistinsert;
- amroutine->aminsertcleanup = NULL;
- amroutine->ambulkdelete = gistbulkdelete;
- amroutine->amvacuumcleanup = gistvacuumcleanup;
- amroutine->amcanreturn = gistcanreturn;
- amroutine->amcostestimate = gistcostestimate;
- amroutine->amgettreeheight = NULL;
- amroutine->amoptions = gistoptions;
- amroutine->amproperty = gistproperty;
- amroutine->ambuildphasename = NULL;
- amroutine->amvalidate = gistvalidate;
- amroutine->amadjustmembers = gistadjustmembers;
- amroutine->ambeginscan = gistbeginscan;
- amroutine->amrescan = gistrescan;
- amroutine->amgettuple = gistgettuple;
- amroutine->amgetbitmap = gistgetbitmap;
- amroutine->amendscan = gistendscan;
- amroutine->ammarkpos = NULL;
- amroutine->amrestrpos = NULL;
- amroutine->amestimateparallelscan = NULL;
- amroutine->aminitparallelscan = NULL;
- amroutine->amparallelrescan = NULL;
- amroutine->amtranslatestrategy = NULL;
- amroutine->amtranslatecmptype = gisttranslatecmptype;
-
- PG_RETURN_POINTER(amroutine);
+ static const IndexAmRoutine amroutine = {
+ .type = T_IndexAmRoutine,
+ .amstrategies = 0,
+ .amsupport = GISTNProcs,
+ .amoptsprocnum = GIST_OPTIONS_PROC,
+ .amcanorder = false,
+ .amcanorderbyop = true,
+ .amcanhash = false,
+ .amconsistentequality = false,
+ .amconsistentordering = false,
+ .amcanbackward = false,
+ .amcanunique = false,
+ .amcanmulticol = true,
+ .amoptionalkey = true,
+ .amsearcharray = false,
+ .amsearchnulls = true,
+ .amstorage = true,
+ .amclusterable = true,
+ .ampredlocks = true,
+ .amcanparallel = false,
+ .amcanbuildparallel = false,
+ .amcaninclude = true,
+ .amusemaintenanceworkmem = false,
+ .amsummarizing = false,
+ .amparallelvacuumoptions =
+ VACUUM_OPTION_PARALLEL_BULKDEL | VACUUM_OPTION_PARALLEL_COND_CLEANUP,
+ .amkeytype = InvalidOid,
+
+ .ambuild = gistbuild,
+ .ambuildempty = gistbuildempty,
+ .aminsert = gistinsert,
+ .aminsertcleanup = NULL,
+ .ambulkdelete = gistbulkdelete,
+ .amvacuumcleanup = gistvacuumcleanup,
+ .amcanreturn = gistcanreturn,
+ .amcostestimate = gistcostestimate,
+ .amgettreeheight = NULL,
+ .amoptions = gistoptions,
+ .amproperty = gistproperty,
+ .ambuildphasename = NULL,
+ .amvalidate = gistvalidate,
+ .amadjustmembers = gistadjustmembers,
+ .ambeginscan = gistbeginscan,
+ .amrescan = gistrescan,
+ .amgettuple = gistgettuple,
+ .amgetbitmap = gistgetbitmap,
+ .amendscan = gistendscan,
+ .ammarkpos = NULL,
+ .amrestrpos = NULL,
+ .amestimateparallelscan = NULL,
+ .aminitparallelscan = NULL,
+ .amparallelrescan = NULL,
+ .amtranslatestrategy = NULL,
+ .amtranslatecmptype = gisttranslatecmptype,
+ };
+
+ PG_RETURN_POINTER(&amroutine);
}
/*
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index e388252afdc..a2172d1063f 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -57,62 +57,63 @@ static void hashbuildCallback(Relation index,
Datum
hashhandler(PG_FUNCTION_ARGS)
{
- IndexAmRoutine *amroutine = makeNode(IndexAmRoutine);
-
- amroutine->amstrategies = HTMaxStrategyNumber;
- amroutine->amsupport = HASHNProcs;
- amroutine->amoptsprocnum = HASHOPTIONS_PROC;
- amroutine->amcanorder = false;
- amroutine->amcanorderbyop = false;
- amroutine->amcanhash = true;
- amroutine->amconsistentequality = true;
- amroutine->amconsistentordering = false;
- amroutine->amcanbackward = true;
- amroutine->amcanunique = false;
- amroutine->amcanmulticol = false;
- amroutine->amoptionalkey = false;
- amroutine->amsearcharray = false;
- amroutine->amsearchnulls = false;
- amroutine->amstorage = false;
- amroutine->amclusterable = false;
- amroutine->ampredlocks = true;
- amroutine->amcanparallel = false;
- amroutine->amcanbuildparallel = false;
- amroutine->amcaninclude = false;
- amroutine->amusemaintenanceworkmem = false;
- amroutine->amsummarizing = false;
- amroutine->amparallelvacuumoptions =
- VACUUM_OPTION_PARALLEL_BULKDEL;
- amroutine->amkeytype = INT4OID;
-
- amroutine->ambuild = hashbuild;
- amroutine->ambuildempty = hashbuildempty;
- amroutine->aminsert = hashinsert;
- amroutine->aminsertcleanup = NULL;
- amroutine->ambulkdelete = hashbulkdelete;
- amroutine->amvacuumcleanup = hashvacuumcleanup;
- amroutine->amcanreturn = NULL;
- amroutine->amcostestimate = hashcostestimate;
- amroutine->amgettreeheight = NULL;
- amroutine->amoptions = hashoptions;
- amroutine->amproperty = NULL;
- amroutine->ambuildphasename = NULL;
- amroutine->amvalidate = hashvalidate;
- amroutine->amadjustmembers = hashadjustmembers;
- amroutine->ambeginscan = hashbeginscan;
- amroutine->amrescan = hashrescan;
- amroutine->amgettuple = hashgettuple;
- amroutine->amgetbitmap = hashgetbitmap;
- amroutine->amendscan = hashendscan;
- amroutine->ammarkpos = NULL;
- amroutine->amrestrpos = NULL;
- amroutine->amestimateparallelscan = NULL;
- amroutine->aminitparallelscan = NULL;
- amroutine->amparallelrescan = NULL;
- amroutine->amtranslatestrategy = hashtranslatestrategy;
- amroutine->amtranslatecmptype = hashtranslatecmptype;
-
- PG_RETURN_POINTER(amroutine);
+ static const IndexAmRoutine amroutine = {
+ .type = T_IndexAmRoutine,
+ .amstrategies = HTMaxStrategyNumber,
+ .amsupport = HASHNProcs,
+ .amoptsprocnum = HASHOPTIONS_PROC,
+ .amcanorder = false,
+ .amcanorderbyop = false,
+ .amcanhash = true,
+ .amconsistentequality = true,
+ .amconsistentordering = false,
+ .amcanbackward = true,
+ .amcanunique = false,
+ .amcanmulticol = false,
+ .amoptionalkey = false,
+ .amsearcharray = false,
+ .amsearchnulls = false,
+ .amstorage = false,
+ .amclusterable = false,
+ .ampredlocks = true,
+ .amcanparallel = false,
+ .amcanbuildparallel = false,
+ .amcaninclude = false,
+ .amusemaintenanceworkmem = false,
+ .amsummarizing = false,
+ .amparallelvacuumoptions =
+ VACUUM_OPTION_PARALLEL_BULKDEL,
+ .amkeytype = INT4OID,
+
+ .ambuild = hashbuild,
+ .ambuildempty = hashbuildempty,
+ .aminsert = hashinsert,
+ .aminsertcleanup = NULL,
+ .ambulkdelete = hashbulkdelete,
+ .amvacuumcleanup = hashvacuumcleanup,
+ .amcanreturn = NULL,
+ .amcostestimate = hashcostestimate,
+ .amgettreeheight = NULL,
+ .amoptions = hashoptions,
+ .amproperty = NULL,
+ .ambuildphasename = NULL,
+ .amvalidate = hashvalidate,
+ .amadjustmembers = hashadjustmembers,
+ .ambeginscan = hashbeginscan,
+ .amrescan = hashrescan,
+ .amgettuple = hashgettuple,
+ .amgetbitmap = hashgetbitmap,
+ .amendscan = hashendscan,
+ .ammarkpos = NULL,
+ .amrestrpos = NULL,
+ .amestimateparallelscan = NULL,
+ .aminitparallelscan = NULL,
+ .amparallelrescan = NULL,
+ .amtranslatestrategy = hashtranslatestrategy,
+ .amtranslatecmptype = hashtranslatecmptype,
+ };
+
+ PG_RETURN_POINTER(&amroutine);
}
/*
diff --git a/src/backend/access/index/amapi.c b/src/backend/access/index/amapi.c
index 60684c53422..453fd2a1dcf 100644
--- a/src/backend/access/index/amapi.c
+++ b/src/backend/access/index/amapi.c
@@ -23,20 +23,20 @@
/*
* GetIndexAmRoutine - call the specified access method handler routine to get
- * its IndexAmRoutine struct, which will be palloc'd in the caller's context.
+ * its IndexAmRoutine struct, which we expect to be statically allocated.
*
* Note that if the amhandler function is built-in, this will not involve
* any catalog access. It's therefore safe to use this while bootstrapping
* indexes for the system catalogs. relcache.c relies on that.
*/
-IndexAmRoutine *
+const IndexAmRoutine *
GetIndexAmRoutine(Oid amhandler)
{
Datum datum;
- IndexAmRoutine *routine;
+ const IndexAmRoutine *routine;
datum = OidFunctionCall0(amhandler);
- routine = (IndexAmRoutine *) DatumGetPointer(datum);
+ routine = (const IndexAmRoutine *) DatumGetPointer(datum);
if (routine == NULL || !IsA(routine, IndexAmRoutine))
elog(ERROR, "index access method handler function %u did not return an IndexAmRoutine struct",
@@ -65,7 +65,7 @@ GetIndexAmRoutine(Oid amhandler)
* If the given OID isn't a valid index access method, returns NULL if
* noerror is true, else throws error.
*/
-IndexAmRoutine *
+const IndexAmRoutine *
GetIndexAmRoutineByAmId(Oid amoid, bool noerror)
{
HeapTuple tuple;
@@ -131,7 +131,7 @@ CompareType
IndexAmTranslateStrategy(StrategyNumber strategy, Oid amoid, Oid opfamily, bool missing_ok)
{
CompareType result;
- IndexAmRoutine *amroutine;
+ const IndexAmRoutine *amroutine;
/* shortcut for common case */
if (amoid == BTREE_AM_OID &&
@@ -161,7 +161,7 @@ StrategyNumber
IndexAmTranslateCompareType(CompareType cmptype, Oid amoid, Oid opfamily, bool missing_ok)
{
StrategyNumber result;
- IndexAmRoutine *amroutine;
+ const IndexAmRoutine *amroutine;
/* shortcut for common case */
if (amoid == BTREE_AM_OID &&
@@ -191,7 +191,7 @@ amvalidate(PG_FUNCTION_ARGS)
HeapTuple classtup;
Form_pg_opclass classform;
Oid amoid;
- IndexAmRoutine *amroutine;
+ const IndexAmRoutine *amroutine;
classtup = SearchSysCache1(CLAOID, ObjectIdGetDatum(opclassoid));
if (!HeapTupleIsValid(classtup))
@@ -210,7 +210,5 @@ amvalidate(PG_FUNCTION_ARGS)
result = amroutine->amvalidate(opclassoid);
- pfree(amroutine);
-
PG_RETURN_BOOL(result);
}
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index b4425231935..4a421f0b8f7 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -115,62 +115,63 @@ static BTVacuumPosting btreevacuumposting(BTVacState *vstate,
Datum
bthandler(PG_FUNCTION_ARGS)
{
- IndexAmRoutine *amroutine = makeNode(IndexAmRoutine);
-
- amroutine->amstrategies = BTMaxStrategyNumber;
- amroutine->amsupport = BTNProcs;
- amroutine->amoptsprocnum = BTOPTIONS_PROC;
- amroutine->amcanorder = true;
- amroutine->amcanorderbyop = false;
- amroutine->amcanhash = false;
- amroutine->amconsistentequality = true;
- amroutine->amconsistentordering = true;
- amroutine->amcanbackward = true;
- amroutine->amcanunique = true;
- amroutine->amcanmulticol = true;
- amroutine->amoptionalkey = true;
- amroutine->amsearcharray = true;
- amroutine->amsearchnulls = true;
- amroutine->amstorage = false;
- amroutine->amclusterable = true;
- amroutine->ampredlocks = true;
- amroutine->amcanparallel = true;
- amroutine->amcanbuildparallel = true;
- amroutine->amcaninclude = true;
- amroutine->amusemaintenanceworkmem = false;
- amroutine->amsummarizing = false;
- amroutine->amparallelvacuumoptions =
- VACUUM_OPTION_PARALLEL_BULKDEL | VACUUM_OPTION_PARALLEL_COND_CLEANUP;
- amroutine->amkeytype = InvalidOid;
-
- amroutine->ambuild = btbuild;
- amroutine->ambuildempty = btbuildempty;
- amroutine->aminsert = btinsert;
- amroutine->aminsertcleanup = NULL;
- amroutine->ambulkdelete = btbulkdelete;
- amroutine->amvacuumcleanup = btvacuumcleanup;
- amroutine->amcanreturn = btcanreturn;
- amroutine->amcostestimate = btcostestimate;
- amroutine->amgettreeheight = btgettreeheight;
- amroutine->amoptions = btoptions;
- amroutine->amproperty = btproperty;
- amroutine->ambuildphasename = btbuildphasename;
- amroutine->amvalidate = btvalidate;
- amroutine->amadjustmembers = btadjustmembers;
- amroutine->ambeginscan = btbeginscan;
- amroutine->amrescan = btrescan;
- amroutine->amgettuple = btgettuple;
- amroutine->amgetbitmap = btgetbitmap;
- amroutine->amendscan = btendscan;
- amroutine->ammarkpos = btmarkpos;
- amroutine->amrestrpos = btrestrpos;
- amroutine->amestimateparallelscan = btestimateparallelscan;
- amroutine->aminitparallelscan = btinitparallelscan;
- amroutine->amparallelrescan = btparallelrescan;
- amroutine->amtranslatestrategy = bttranslatestrategy;
- amroutine->amtranslatecmptype = bttranslatecmptype;
-
- PG_RETURN_POINTER(amroutine);
+ static const IndexAmRoutine amroutine = {
+ .type = T_IndexAmRoutine,
+ .amstrategies = BTMaxStrategyNumber,
+ .amsupport = BTNProcs,
+ .amoptsprocnum = BTOPTIONS_PROC,
+ .amcanorder = true,
+ .amcanorderbyop = false,
+ .amcanhash = false,
+ .amconsistentequality = true,
+ .amconsistentordering = true,
+ .amcanbackward = true,
+ .amcanunique = true,
+ .amcanmulticol = true,
+ .amoptionalkey = true,
+ .amsearcharray = true,
+ .amsearchnulls = true,
+ .amstorage = false,
+ .amclusterable = true,
+ .ampredlocks = true,
+ .amcanparallel = true,
+ .amcanbuildparallel = true,
+ .amcaninclude = true,
+ .amusemaintenanceworkmem = false,
+ .amsummarizing = false,
+ .amparallelvacuumoptions =
+ VACUUM_OPTION_PARALLEL_BULKDEL | VACUUM_OPTION_PARALLEL_COND_CLEANUP,
+ .amkeytype = InvalidOid,
+
+ .ambuild = btbuild,
+ .ambuildempty = btbuildempty,
+ .aminsert = btinsert,
+ .aminsertcleanup = NULL,
+ .ambulkdelete = btbulkdelete,
+ .amvacuumcleanup = btvacuumcleanup,
+ .amcanreturn = btcanreturn,
+ .amcostestimate = btcostestimate,
+ .amgettreeheight = btgettreeheight,
+ .amoptions = btoptions,
+ .amproperty = btproperty,
+ .ambuildphasename = btbuildphasename,
+ .amvalidate = btvalidate,
+ .amadjustmembers = btadjustmembers,
+ .ambeginscan = btbeginscan,
+ .amrescan = btrescan,
+ .amgettuple = btgettuple,
+ .amgetbitmap = btgetbitmap,
+ .amendscan = btendscan,
+ .ammarkpos = btmarkpos,
+ .amrestrpos = btrestrpos,
+ .amestimateparallelscan = btestimateparallelscan,
+ .aminitparallelscan = btinitparallelscan,
+ .amparallelrescan = btparallelrescan,
+ .amtranslatestrategy = bttranslatestrategy,
+ .amtranslatecmptype = bttranslatecmptype,
+ };
+
+ PG_RETURN_POINTER(&amroutine);
}
/*
diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c
index a60ec85e8be..a6866bd3520 100644
--- a/src/backend/access/spgist/spgutils.c
+++ b/src/backend/access/spgist/spgutils.c
@@ -43,62 +43,63 @@
Datum
spghandler(PG_FUNCTION_ARGS)
{
- IndexAmRoutine *amroutine = makeNode(IndexAmRoutine);
-
- amroutine->amstrategies = 0;
- amroutine->amsupport = SPGISTNProc;
- amroutine->amoptsprocnum = SPGIST_OPTIONS_PROC;
- amroutine->amcanorder = false;
- amroutine->amcanorderbyop = true;
- amroutine->amcanhash = false;
- amroutine->amconsistentequality = false;
- amroutine->amconsistentordering = false;
- amroutine->amcanbackward = false;
- amroutine->amcanunique = false;
- amroutine->amcanmulticol = false;
- amroutine->amoptionalkey = true;
- amroutine->amsearcharray = false;
- amroutine->amsearchnulls = true;
- amroutine->amstorage = true;
- amroutine->amclusterable = false;
- amroutine->ampredlocks = false;
- amroutine->amcanparallel = false;
- amroutine->amcanbuildparallel = false;
- amroutine->amcaninclude = true;
- amroutine->amusemaintenanceworkmem = false;
- amroutine->amsummarizing = false;
- amroutine->amparallelvacuumoptions =
- VACUUM_OPTION_PARALLEL_BULKDEL | VACUUM_OPTION_PARALLEL_COND_CLEANUP;
- amroutine->amkeytype = InvalidOid;
-
- amroutine->ambuild = spgbuild;
- amroutine->ambuildempty = spgbuildempty;
- amroutine->aminsert = spginsert;
- amroutine->aminsertcleanup = NULL;
- amroutine->ambulkdelete = spgbulkdelete;
- amroutine->amvacuumcleanup = spgvacuumcleanup;
- amroutine->amcanreturn = spgcanreturn;
- amroutine->amcostestimate = spgcostestimate;
- amroutine->amgettreeheight = NULL;
- amroutine->amoptions = spgoptions;
- amroutine->amproperty = spgproperty;
- amroutine->ambuildphasename = NULL;
- amroutine->amvalidate = spgvalidate;
- amroutine->amadjustmembers = spgadjustmembers;
- amroutine->ambeginscan = spgbeginscan;
- amroutine->amrescan = spgrescan;
- amroutine->amgettuple = spggettuple;
- amroutine->amgetbitmap = spggetbitmap;
- amroutine->amendscan = spgendscan;
- amroutine->ammarkpos = NULL;
- amroutine->amrestrpos = NULL;
- amroutine->amestimateparallelscan = NULL;
- amroutine->aminitparallelscan = NULL;
- amroutine->amparallelrescan = NULL;
- amroutine->amtranslatestrategy = NULL;
- amroutine->amtranslatecmptype = NULL;
-
- PG_RETURN_POINTER(amroutine);
+ static const IndexAmRoutine amroutine = {
+ .type = T_IndexAmRoutine,
+ .amstrategies = 0,
+ .amsupport = SPGISTNProc,
+ .amoptsprocnum = SPGIST_OPTIONS_PROC,
+ .amcanorder = false,
+ .amcanorderbyop = true,
+ .amcanhash = false,
+ .amconsistentequality = false,
+ .amconsistentordering = false,
+ .amcanbackward = false,
+ .amcanunique = false,
+ .amcanmulticol = false,
+ .amoptionalkey = true,
+ .amsearcharray = false,
+ .amsearchnulls = true,
+ .amstorage = true,
+ .amclusterable = false,
+ .ampredlocks = false,
+ .amcanparallel = false,
+ .amcanbuildparallel = false,
+ .amcaninclude = true,
+ .amusemaintenanceworkmem = false,
+ .amsummarizing = false,
+ .amparallelvacuumoptions =
+ VACUUM_OPTION_PARALLEL_BULKDEL | VACUUM_OPTION_PARALLEL_COND_CLEANUP,
+ .amkeytype = InvalidOid,
+
+ .ambuild = spgbuild,
+ .ambuildempty = spgbuildempty,
+ .aminsert = spginsert,
+ .aminsertcleanup = NULL,
+ .ambulkdelete = spgbulkdelete,
+ .amvacuumcleanup = spgvacuumcleanup,
+ .amcanreturn = spgcanreturn,
+ .amcostestimate = spgcostestimate,
+ .amgettreeheight = NULL,
+ .amoptions = spgoptions,
+ .amproperty = spgproperty,
+ .ambuildphasename = NULL,
+ .amvalidate = spgvalidate,
+ .amadjustmembers = spgadjustmembers,
+ .ambeginscan = spgbeginscan,
+ .amrescan = spgrescan,
+ .amgettuple = spggettuple,
+ .amgetbitmap = spggetbitmap,
+ .amendscan = spgendscan,
+ .ammarkpos = NULL,
+ .amrestrpos = NULL,
+ .amestimateparallelscan = NULL,
+ .aminitparallelscan = NULL,
+ .amparallelrescan = NULL,
+ .amtranslatestrategy = NULL,
+ .amtranslatecmptype = NULL,
+ };
+
+ PG_RETURN_POINTER(&amroutine);
}
/*
diff --git a/src/backend/access/table/tableamapi.c b/src/backend/access/table/tableamapi.c
index 476663b66aa..6745d608f93 100644
--- a/src/backend/access/table/tableamapi.c
+++ b/src/backend/access/table/tableamapi.c
@@ -21,8 +21,7 @@
/*
* GetTableAmRoutine
* Call the specified access method handler routine to get its
- * TableAmRoutine struct, which will be palloc'd in the caller's
- * memory context.
+ * TableAmRoutine struct, which we expect to be statically allocated.
*/
const TableAmRoutine *
GetTableAmRoutine(Oid amhandler)
@@ -31,7 +30,7 @@ GetTableAmRoutine(Oid amhandler)
const TableAmRoutine *routine;
datum = OidFunctionCall0(amhandler);
- routine = (TableAmRoutine *) DatumGetPointer(datum);
+ routine = (const TableAmRoutine *) DatumGetPointer(datum);
if (routine == NULL || !IsA(routine, TableAmRoutine))
elog(ERROR, "table access method handler %u did not return a TableAmRoutine struct",
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 8dea58ad96b..86db0541f29 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -289,7 +289,7 @@ ConstructTupleDescriptor(Relation heapRelation,
int numkeyatts = indexInfo->ii_NumIndexKeyAttrs;
ListCell *colnames_item = list_head(indexColNames);
ListCell *indexpr_item = list_head(indexInfo->ii_Expressions);
- IndexAmRoutine *amroutine;
+ const IndexAmRoutine *amroutine;
TupleDesc heapTupDesc;
TupleDesc indexTupDesc;
int natts; /* #atts in heap rel --- for error checks */
@@ -481,8 +481,6 @@ ConstructTupleDescriptor(Relation heapRelation,
populate_compact_attribute(indexTupDesc, i);
}
- pfree(amroutine);
-
return indexTupDesc;
}
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index d9cccb6ac18..67a451463ed 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -190,7 +190,7 @@ CheckIndexCompatible(Oid oldId,
HeapTuple tuple;
Form_pg_index indexForm;
Form_pg_am accessMethodForm;
- IndexAmRoutine *amRoutine;
+ const IndexAmRoutine *amRoutine;
bool amcanorder;
bool amsummarizing;
int16 *coloptions;
@@ -566,7 +566,7 @@ DefineIndex(Oid tableId,
Relation rel;
HeapTuple tuple;
Form_pg_am accessMethodForm;
- IndexAmRoutine *amRoutine;
+ const IndexAmRoutine *amRoutine;
bool amcanorder;
bool amissummarizing;
amoptions_function amoptions;
@@ -895,7 +895,6 @@ DefineIndex(Oid tableId,
amoptions = amRoutine->amoptions;
amissummarizing = amRoutine->amsummarizing;
- pfree(amRoutine);
ReleaseSysCache(tuple);
/*
diff --git a/src/backend/commands/opclasscmds.c b/src/backend/commands/opclasscmds.c
index 992ae789b00..20c54a523c5 100644
--- a/src/backend/commands/opclasscmds.c
+++ b/src/backend/commands/opclasscmds.c
@@ -349,7 +349,7 @@ DefineOpClass(CreateOpClassStmt *stmt)
Relation rel;
HeapTuple tup;
Form_pg_am amform;
- IndexAmRoutine *amroutine;
+ const IndexAmRoutine *amroutine;
Datum values[Natts_pg_opclass];
bool nulls[Natts_pg_opclass];
AclResult aclresult;
@@ -823,7 +823,7 @@ AlterOpFamily(AlterOpFamilyStmt *stmt)
maxProcNumber; /* amsupport value */
HeapTuple tup;
Form_pg_am amform;
- IndexAmRoutine *amroutine;
+ const IndexAmRoutine *amroutine;
/* Get necessary info about access method */
tup = SearchSysCache1(AMNAME, CStringGetDatum(stmt->amname));
@@ -882,7 +882,7 @@ AlterOpFamilyAdd(AlterOpFamilyStmt *stmt, Oid amoid, Oid opfamilyoid,
int maxOpNumber, int maxProcNumber, int optsProcNumber,
List *items)
{
- IndexAmRoutine *amroutine = GetIndexAmRoutineByAmId(amoid, false);
+ const IndexAmRoutine *amroutine = GetIndexAmRoutineByAmId(amoid, false);
List *operators; /* OpFamilyMember list for operators */
List *procedures; /* OpFamilyMember list for support procs */
ListCell *l;
@@ -1165,9 +1165,7 @@ assignOperTypes(OpFamilyMember *member, Oid amoid, Oid typeoid)
* the family has been created but not yet populated with the required
* operators.)
*/
- IndexAmRoutine *amroutine = GetIndexAmRoutineByAmId(amoid, false);
-
- if (!amroutine->amcanorderbyop)
+ if (!GetIndexAmRoutineByAmId(amoid, false)->amcanorderbyop)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
errmsg("access method \"%s\" does not support ordering operators",
diff --git a/src/backend/executor/execAmi.c b/src/backend/executor/execAmi.c
index 1d0e8ad57b4..4893ea3ce91 100644
--- a/src/backend/executor/execAmi.c
+++ b/src/backend/executor/execAmi.c
@@ -605,7 +605,7 @@ IndexSupportsBackwardScan(Oid indexid)
bool result;
HeapTuple ht_idxrel;
Form_pg_class idxrelrec;
- IndexAmRoutine *amroutine;
+ const IndexAmRoutine *amroutine;
/* Fetch the pg_class tuple of the index relation */
ht_idxrel = SearchSysCache1(RELOID, ObjectIdGetDatum(indexid));
@@ -618,7 +618,6 @@ IndexSupportsBackwardScan(Oid indexid)
result = amroutine->amcanbackward;
- pfree(amroutine);
ReleaseSysCache(ht_idxrel);
return result;
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index bf45c355b77..43ca5fd0213 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -231,7 +231,7 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent,
Oid indexoid = lfirst_oid(l);
Relation indexRelation;
Form_pg_index index;
- IndexAmRoutine *amroutine = NULL;
+ const IndexAmRoutine *amroutine = NULL;
IndexOptInfo *info;
int ncolumns,
nkeycolumns;
diff --git a/src/backend/utils/adt/amutils.c b/src/backend/utils/adt/amutils.c
index 0af26d6acfa..59c0112c696 100644
--- a/src/backend/utils/adt/amutils.c
+++ b/src/backend/utils/adt/amutils.c
@@ -156,7 +156,7 @@ indexam_property(FunctionCallInfo fcinfo,
bool isnull = false;
int natts = 0;
IndexAMProperty prop;
- IndexAmRoutine *routine;
+ const IndexAmRoutine *routine;
/* Try to convert property name to enum (no error if not known) */
prop = lookup_prop_name(propname);
@@ -452,7 +452,7 @@ pg_indexam_progress_phasename(PG_FUNCTION_ARGS)
{
Oid amoid = PG_GETARG_OID(0);
int32 phasenum = PG_GETARG_INT32(1);
- IndexAmRoutine *routine;
+ const IndexAmRoutine *routine;
char *name;
routine = GetIndexAmRoutineByAmId(amoid, true);
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 9f85eb86da1..ef6978111e8 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -1281,7 +1281,7 @@ pg_get_indexdef_worker(Oid indexrelid, int colno,
Form_pg_index idxrec;
Form_pg_class idxrelrec;
Form_pg_am amrec;
- IndexAmRoutine *amroutine;
+ const IndexAmRoutine *amroutine;
List *indexprs;
ListCell *indexpr_item;
List *context;
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index 5aa7a26d95c..a14fef3ba9e 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -231,14 +231,7 @@ get_opmethod_canorder(Oid amoid)
case BRIN_AM_OID:
return false;
default:
- {
- bool result;
- IndexAmRoutine *amroutine = GetIndexAmRoutineByAmId(amoid, false);
-
- result = amroutine->amcanorder;
- pfree(amroutine);
- return result;
- }
+ return GetIndexAmRoutineByAmId(amoid, false)->amcanorder;
}
}
@@ -729,7 +722,7 @@ get_op_index_interpretation(Oid opno)
{
HeapTuple op_tuple = &catlist->members[i]->tuple;
Form_pg_amop op_form = (Form_pg_amop) GETSTRUCT(op_tuple);
- IndexAmRoutine *amroutine = GetIndexAmRoutineByAmId(op_form->amopmethod, false);
+ const IndexAmRoutine *amroutine = GetIndexAmRoutineByAmId(op_form->amopmethod, false);
CompareType cmptype;
/* must be ordering index */
@@ -800,15 +793,11 @@ equality_ops_are_compatible(Oid opno1, Oid opno2)
* op_in_opfamily() is cheaper than GetIndexAmRoutineByAmId(), so
* check it first
*/
- if (op_in_opfamily(opno2, op_form->amopfamily))
+ if (op_in_opfamily(opno2, op_form->amopfamily) &&
+ GetIndexAmRoutineByAmId(op_form->amopmethod, false)->amconsistentequality)
{
- IndexAmRoutine *amroutine = GetIndexAmRoutineByAmId(op_form->amopmethod, false);
-
- if (amroutine->amconsistentequality)
- {
- result = true;
- break;
- }
+ result = true;
+ break;
}
}
@@ -856,15 +845,11 @@ comparison_ops_are_compatible(Oid opno1, Oid opno2)
* op_in_opfamily() is cheaper than GetIndexAmRoutineByAmId(), so
* check it first
*/
- if (op_in_opfamily(opno2, op_form->amopfamily))
+ if (op_in_opfamily(opno2, op_form->amopfamily) &&
+ GetIndexAmRoutineByAmId(op_form->amopmethod, false)->amconsistentordering)
{
- IndexAmRoutine *amroutine = GetIndexAmRoutineByAmId(op_form->amopmethod, false);
-
- if (amroutine->amconsistentordering)
- {
- result = true;
- break;
- }
+ result = true;
+ break;
}
}
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 2d0cb7bcfd4..88259f7c228 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -1420,22 +1420,17 @@ RelationInitPhysicalAddr(Relation relation)
static void
InitIndexAmRoutine(Relation relation)
{
- IndexAmRoutine *cached,
- *tmp;
+ MemoryContext oldctx;
/*
- * Call the amhandler in current, short-lived memory context, just in case
- * it leaks anything (it probably won't, but let's be paranoid).
+ * We formerly specified that the amhandler should return a palloc'd
+ * struct. That's now deprecated in favor of returning a pointer to a
+ * static struct, but to avoid completely breaking old external AMs, run
+ * the amhandler in the relation's rd_indexcxt.
*/
- tmp = GetIndexAmRoutine(relation->rd_amhandler);
-
- /* OK, now transfer the data into relation's rd_indexcxt. */
- cached = (IndexAmRoutine *) MemoryContextAlloc(relation->rd_indexcxt,
- sizeof(IndexAmRoutine));
- memcpy(cached, tmp, sizeof(IndexAmRoutine));
- relation->rd_indam = cached;
-
- pfree(tmp);
+ oldctx = MemoryContextSwitchTo(relation->rd_indexcxt);
+ relation->rd_indam = GetIndexAmRoutine(relation->rd_amhandler);
+ MemoryContextSwitchTo(oldctx);
}
/*
diff --git a/src/include/access/amapi.h b/src/include/access/amapi.h
index 63dd41c1f21..278da36bc08 100644
--- a/src/include/access/amapi.h
+++ b/src/include/access/amapi.h
@@ -226,8 +226,8 @@ typedef void (*aminitparallelscan_function) (void *target);
typedef void (*amparallelrescan_function) (IndexScanDesc scan);
/*
- * API struct for an index AM. Note this must be stored in a single palloc'd
- * chunk of memory.
+ * API struct for an index AM. Note we expect index AMs to allocate these
+ * structs statically; the core code never copies nor frees them.
*/
typedef struct IndexAmRoutine
{
@@ -326,8 +326,8 @@ typedef struct IndexAmRoutine
/* Functions in access/index/amapi.c */
-extern IndexAmRoutine *GetIndexAmRoutine(Oid amhandler);
-extern IndexAmRoutine *GetIndexAmRoutineByAmId(Oid amoid, bool noerror);
+extern const IndexAmRoutine *GetIndexAmRoutine(Oid amhandler);
+extern const IndexAmRoutine *GetIndexAmRoutineByAmId(Oid amoid, bool noerror);
extern CompareType IndexAmTranslateStrategy(StrategyNumber strategy, Oid amoid, Oid opfamily, bool missing_ok);
extern StrategyNumber IndexAmTranslateCompareType(CompareType cmptype, Oid amoid, Oid opfamily, bool missing_ok);
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index 80286076a11..82d0e419e4b 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -203,7 +203,7 @@ typedef struct RelationData
*/
MemoryContext rd_indexcxt; /* private memory cxt for this stuff */
/* use "struct" here to avoid needing to include amapi.h: */
- struct IndexAmRoutine *rd_indam; /* index AM's API struct */
+ const struct IndexAmRoutine *rd_indam; /* index AM's API struct */
Oid *rd_opfamily; /* OIDs of op families for each index col */
Oid *rd_opcintype; /* OIDs of opclass declared input data types */
RegProcedure *rd_support; /* OIDs of support procedures */
diff --git a/src/test/modules/dummy_index_am/dummy_index_am.c b/src/test/modules/dummy_index_am/dummy_index_am.c
index a34382a5f79..d18b18106af 100644
--- a/src/test/modules/dummy_index_am/dummy_index_am.c
+++ b/src/test/modules/dummy_index_am/dummy_index_am.c
@@ -276,56 +276,57 @@ diendscan(IndexScanDesc scan)
Datum
dihandler(PG_FUNCTION_ARGS)
{
- IndexAmRoutine *amroutine = makeNode(IndexAmRoutine);
-
- amroutine->amstrategies = 0;
- amroutine->amsupport = 1;
- amroutine->amcanorder = false;
- amroutine->amcanorderbyop = false;
- amroutine->amcanhash = false;
- amroutine->amconsistentequality = false;
- amroutine->amconsistentordering = false;
- amroutine->amcanbackward = false;
- amroutine->amcanunique = false;
- amroutine->amcanmulticol = false;
- amroutine->amoptionalkey = false;
- amroutine->amsearcharray = false;
- amroutine->amsearchnulls = false;
- amroutine->amstorage = false;
- amroutine->amclusterable = false;
- amroutine->ampredlocks = false;
- amroutine->amcanparallel = false;
- amroutine->amcanbuildparallel = false;
- amroutine->amcaninclude = false;
- amroutine->amusemaintenanceworkmem = false;
- amroutine->amsummarizing = false;
- amroutine->amparallelvacuumoptions = VACUUM_OPTION_NO_PARALLEL;
- amroutine->amkeytype = InvalidOid;
-
- amroutine->ambuild = dibuild;
- amroutine->ambuildempty = dibuildempty;
- amroutine->aminsert = diinsert;
- amroutine->ambulkdelete = dibulkdelete;
- amroutine->amvacuumcleanup = divacuumcleanup;
- amroutine->amcanreturn = NULL;
- amroutine->amcostestimate = dicostestimate;
- amroutine->amgettreeheight = NULL;
- amroutine->amoptions = dioptions;
- amroutine->amproperty = NULL;
- amroutine->ambuildphasename = NULL;
- amroutine->amvalidate = divalidate;
- amroutine->ambeginscan = dibeginscan;
- amroutine->amrescan = direscan;
- amroutine->amgettuple = NULL;
- amroutine->amgetbitmap = NULL;
- amroutine->amendscan = diendscan;
- amroutine->ammarkpos = NULL;
- amroutine->amrestrpos = NULL;
- amroutine->amestimateparallelscan = NULL;
- amroutine->aminitparallelscan = NULL;
- amroutine->amparallelrescan = NULL;
-
- PG_RETURN_POINTER(amroutine);
+ static const IndexAmRoutine amroutine = {
+ .type = T_IndexAmRoutine,
+ .amstrategies = 0,
+ .amsupport = 1,
+ .amcanorder = false,
+ .amcanorderbyop = false,
+ .amcanhash = false,
+ .amconsistentequality = false,
+ .amconsistentordering = false,
+ .amcanbackward = false,
+ .amcanunique = false,
+ .amcanmulticol = false,
+ .amoptionalkey = false,
+ .amsearcharray = false,
+ .amsearchnulls = false,
+ .amstorage = false,
+ .amclusterable = false,
+ .ampredlocks = false,
+ .amcanparallel = false,
+ .amcanbuildparallel = false,
+ .amcaninclude = false,
+ .amusemaintenanceworkmem = false,
+ .amsummarizing = false,
+ .amparallelvacuumoptions = VACUUM_OPTION_NO_PARALLEL,
+ .amkeytype = InvalidOid,
+
+ .ambuild = dibuild,
+ .ambuildempty = dibuildempty,
+ .aminsert = diinsert,
+ .ambulkdelete = dibulkdelete,
+ .amvacuumcleanup = divacuumcleanup,
+ .amcanreturn = NULL,
+ .amcostestimate = dicostestimate,
+ .amgettreeheight = NULL,
+ .amoptions = dioptions,
+ .amproperty = NULL,
+ .ambuildphasename = NULL,
+ .amvalidate = divalidate,
+ .ambeginscan = dibeginscan,
+ .amrescan = direscan,
+ .amgettuple = NULL,
+ .amgetbitmap = NULL,
+ .amendscan = diendscan,
+ .ammarkpos = NULL,
+ .amrestrpos = NULL,
+ .amestimateparallelscan = NULL,
+ .aminitparallelscan = NULL,
+ .amparallelrescan = NULL,
+ };
+
+ PG_RETURN_POINTER(&amroutine);
}
void
--
2.43.7
> On Dec 30, 2025, at 04:04, Matthias van de Meent <boekewurm+postgres@gmail.com> wrote: > > On Mon, 29 Dec 2025 at 20:01, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> >> Matthias van de Meent <boekewurm+postgres@gmail.com> writes >>> 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. >> >> Cool, I forgot you'd already been poking at this. The patch >> is kinda long, but not as bad as I feared, and it all looks >> quite mechanical. It lacks documentation updates though. > > Attached v2, in which I've updated the one place where I could find > IndexAmRoutine's allocation policy being described, and in which I've > also updated InitIndexAmRoutine with the suggested changes of your > other mail. Thanks! > > Kind regards, > > Matthias van de Meent > Databricks (https://www.databricks.com) > <v2-0001-Stop-allocating-one-IndexAmRoutine-for-every-inde.patch> I’m glad that my finding helped move this patch forward. After reviewing v2, I think my patch can be completely supersededby yours, which is fine with me. I have a couple of comments on v2. 1 - amapi.c ``` /* * GetIndexAmRoutine - call the specified access method handler routine to get * its IndexAmRoutine struct, which will be palloc'd in the caller's context. * * Note that if the amhandler function is built-in, this will not involve * any catalog access. It's therefore safe to use this while bootstrapping * indexes for the system catalogs. relcache.c relies on that. */ const IndexAmRoutine * GetIndexAmRoutine(Oid amhandler) { Datum datum; IndexAmRoutine *routine; ``` * The function header comment needs an update, it still talks about “palloc”, now it should say something like “returnedstructure should not be free-ed”. * The local variable “routine” now can be “const” as well. 2 - relcache.c ``` InitIndexAmRoutine(Relation relation) { - IndexAmRoutine *cached, - *tmp; + MemoryContext oldctx; /* - * Call the amhandler in current, short-lived memory context, just in case - * it leaks anything (it probably won't, but let's be paranoid). + * We formerly specified that the amhandler should return a + * palloc'd struct. That's now deprecated in favor of returning + * a pointer to a static struct, but to avoid completely breaking + * old external AMs, run the amhandler in the relation's rd_indexcxt. */ - tmp = GetIndexAmRoutine(relation->rd_amhandler); - - /* OK, now transfer the data into relation's rd_indexcxt. */ - cached = (IndexAmRoutine *) MemoryContextAlloc(relation->rd_indexcxt, - sizeof(IndexAmRoutine)); - memcpy(cached, tmp, sizeof(IndexAmRoutine)); - relation->rd_indam = cached; - - pfree(tmp); + oldctx = MemoryContextSwitchTo(relation->rd_indexcxt); + relation->rd_indam = GetIndexAmRoutine(relation->rd_amhandler); + MemoryContextSwitchTo(oldctx); } ``` I understand that switching to rd_indexcxt is intended to mitigate the risk that external index AMs might still palloc thereturned IndexAmRoutine. However, I don’t see a way to actually enforce external AMs to always return a static pointer. In particular, if an AM switchesto a different memory context internally, the MemoryContextSwitchTo() here would not help. I’m not sure whether we want to go further than the current approach, but it seems that fully eliminating the risk wouldrequire detecting dynamically allocated results and copying them into rd_indexcxt, which doesn’t appear easy or robustto implement in practice. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
On Tue, Dec 30, 2025 at 6:55 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Matthias van de Meent <boekewurm+postgres@gmail.com> writes:
> Attached v2, in which I've updated the one place where I could find
> IndexAmRoutine's allocation policy being described, and in which I've
> also updated InitIndexAmRoutine with the suggested changes of your
> other mail. Thanks!
I went through this and made some mostly-cosmetic changes.
I think the attached v3 is ready to go, but I'll wait a day
or so to see if anyone has any comments.
regards, tom lane
I just saw v3 after sending the review comments for v2. Looks like my comment 1 has been addressed in v3, and the comment 2 still stands.
Best regards,
--
Chao Li (Evan)HighGo Software Co., Ltd.
Re: lsyscache: free IndexAmRoutine objects returned by GetIndexAmRoutineByAmId()
От
Matthias van de Meent
Дата:
On Tue, 30 Dec 2025 at 00:22, Chao Li <li.evan.chao@gmail.com> wrote: > I understand that switching to rd_indexcxt is intended to mitigate the risk that external index AMs might still pallocthe returned IndexAmRoutine. > > However, I don’t see a way to actually enforce external AMs to always return a static pointer. In particular, if an AMswitches to a different memory context internally, the MemoryContextSwitchTo() here would not help. Yes, sadly it's quite difficult to determine whether something is statically allocated. Unlike with palloc'd objects, you can't rely on an always-available header in assert-enabled builds; the best we can do introspection into which OS memory area this allocation is placed; which encroaches on ASan and Valgrind's featureset --- and even that is not necessarily sufficient, as not all compilers may put `static const` -equivalent objects into knowable memory locations. > I’m not sure whether we want to go further than the current approach, but it seems that fully eliminating the risk wouldrequire detecting dynamically allocated results and copying them into rd_indexcxt, which doesn’t appear easy or robustto implement in practice. Yes, I don't think there is much more we can do here to protect index AM implementations against this change within Postgres' own tooling without introducing address space detection features more reasonably found in ASan or Valgrind. So I think this is sufficient as a best-effort approach. Kind regards, Matthias van de Meent
Chao Li <li.evan.chao@gmail.com> writes:
> I understand that switching to rd_indexcxt is intended to mitigate the risk that external index AMs might still
pallocthe returned IndexAmRoutine.
> However, I don’t see a way to actually enforce external AMs to always return a static pointer. In particular, if an
AMswitches 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
Re: lsyscache: free IndexAmRoutine objects returned by GetIndexAmRoutineByAmId()
От
Álvaro Herrera
Дата:
On 2025-Dec-29, Tom Lane wrote:
> 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.
One thing we can perhaps do is (in assert-enabled builds) to detect
whether memory usage for that context has increased during
InitIndexAmRoutine and raise a warning if so. Then extension authors
would realize this and have a chance to fix it promptly.
I tried this out and it doesn't work as well as I thought, due to how
AllocSet works: we don't get a difference in the amount of allocated
memory (thus no WARNING) unless we add some padding bytes to
IndexAmRoutine, as shown below (of course, just POC-quality -- didn't
kry to compile without assertions). Given this, I'm not terribly
optimistic about this idea. I tested this by adding
palloc(sizeof(IndexAmRoutine)) in brinhandler() and verifying that a few
regression tests fail with the added warning message.
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 88259f7c228..75f2a2f5e37 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -1421,6 +1421,7 @@ static void
InitIndexAmRoutine(Relation relation)
{
MemoryContext oldctx;
+ Size allocated PG_USED_FOR_ASSERTS_ONLY;
/*
* We formerly specified that the amhandler should return a palloc'd
@@ -1429,7 +1430,19 @@ InitIndexAmRoutine(Relation relation)
* the amhandler in the relation's rd_indexcxt.
*/
oldctx = MemoryContextSwitchTo(relation->rd_indexcxt);
+
+#ifdef USE_ASSERT_CHECKING
+ allocated = MemoryContextMemAllocated(CurrentMemoryContext, false);
+#endif
+
relation->rd_indam = GetIndexAmRoutine(relation->rd_amhandler);
+
+#ifdef USE_ASSERT_CHECKING
+ if (MemoryContextMemAllocated(CurrentMemoryContext, false) - allocated != 0)
+ elog(WARNING, "memory allocated while initializing access method %u (was %zu, now %zu)",
+ relation->rd_amhandler, allocated, MemoryContextMemAllocated(CurrentMemoryContext, false));
+#endif
+
MemoryContextSwitchTo(oldctx);
}
diff --git a/src/include/access/amapi.h b/src/include/access/amapi.h
index 278da36bc08..78fcbcffc14 100644
--- a/src/include/access/amapi.h
+++ b/src/include/access/amapi.h
@@ -322,6 +322,11 @@ typedef struct IndexAmRoutine
/* interface functions to support planning */
amtranslate_strategy_function amtranslatestrategy; /* can be NULL */
amtranslate_cmptype_function amtranslatecmptype; /* can be NULL */
+
+#ifdef USE_ASSERT_CHECKING
+ char pad[512];
+#endif
+
} IndexAmRoutine;
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Re: lsyscache: free IndexAmRoutine objects returned by GetIndexAmRoutineByAmId()
От
Matthias van de Meent
Дата:
On Tue, 30 Dec 2025 at 15:15, Álvaro Herrera <alvherre@kurilemu.de> wrote: > > On 2025-Dec-29, Tom Lane wrote: > > > 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. > > One thing we can perhaps do is (in assert-enabled builds) to detect > whether memory usage for that context has increased during > InitIndexAmRoutine and raise a warning if so. Then extension authors > would realize this and have a chance to fix it promptly. > > I tried this out and it doesn't work as well as I thought, due to how > AllocSet works: we don't get a difference in the amount of allocated > memory (thus no WARNING) unless we add some padding bytes to > IndexAmRoutine Hmm, wouldn't we be able to detect changes in MemoryContextMemConsumed(ctx, counters) with one before and one after GetIndexAmRoutine(), such as included below? It would cause false positives if amroutine() does memory-related work other than just returning an appropriate IndexAmRoutine pointer (if it does so without switching to its own MemoryContext), but I don't think that such false positives here are necessarily bad - the AM shouldn't be doing much at this point in the code. Kind regards, Matthias van de Meent Databricks (https://www.databricks.com) diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index 86b90765433..c17f9c3e53a 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -1421,6 +1421,13 @@ static void InitIndexAmRoutine(Relation relation) { MemoryContext oldctx; +#if USE_ASSERT_CHECKING + Size prevusedmem; + MemoryContextCounters usage; + + MemoryContextMemConsumed(relation->rd_indexcxt, &usage); + prevusedmem = usage.totalspace - usage.freespace; +#endif /* * We formerly specified that the amhandler should return a @@ -1431,6 +1438,12 @@ InitIndexAmRoutine(Relation relation) oldctx = MemoryContextSwitchTo(relation->rd_indexcxt); relation->rd_indam = GetIndexAmRoutine(relation->rd_amhandler); MemoryContextSwitchTo(oldctx); + +#if USE_ASSERT_CHECKING + /* ensure the index routine was not palloc'd */ + MemoryContextMemConsumed(relation->rd_indexcxt, &usage); + Assert(prevusedmem == (usage.totalspace - usage.freespace)); +#endif } /* ```
Matthias van de Meent <boekewurm+postgres@gmail.com> writes:
> On Tue, 30 Dec 2025 at 15:15, Álvaro Herrera <alvherre@kurilemu.de> wrote:
>> One thing we can perhaps do is (in assert-enabled builds) to detect
>> whether memory usage for that context has increased during
>> InitIndexAmRoutine and raise a warning if so. Then extension authors
>> would realize this and have a chance to fix it promptly.
> Hmm, wouldn't we be able to detect changes in
> MemoryContextMemConsumed(ctx, counters) with one before and one after
> GetIndexAmRoutine(), such as included below?
I don't think we can do this, because there are effects that the
amhandler doesn't have control over. In particular, if we have to
load its pg_proc row into syscache during fmgr_info, I don't think
that is positively guaranteed not to leak anything. (This isn't
a factor for built-in AMs, which will take the fast path in
fmgr_info, but it will be an issue for extensions.)
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.
regards, tom lane
Re: lsyscache: free IndexAmRoutine objects returned by GetIndexAmRoutineByAmId()
От
Matthias van de Meent
Дата:
On Tue, 30 Dec 2025 at 16:25, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Matthias van de Meent <boekewurm+postgres@gmail.com> writes: > > On Tue, 30 Dec 2025 at 15:15, Álvaro Herrera <alvherre@kurilemu.de> wrote: > >> One thing we can perhaps do is (in assert-enabled builds) to detect > >> whether memory usage for that context has increased during > >> InitIndexAmRoutine and raise a warning if so. Then extension authors > >> would realize this and have a chance to fix it promptly. > > > Hmm, wouldn't we be able to detect changes in > > MemoryContextMemConsumed(ctx, counters) with one before and one after > > GetIndexAmRoutine(), such as included below? > > I don't think we can do this, because there are effects that the > amhandler doesn't have control over. In particular, if we have to > load its pg_proc row into syscache during fmgr_info, I don't think > that is positively guaranteed not to leak anything. (This isn't > a factor for built-in AMs, which will take the fast path in > fmgr_info, but it will be an issue for extensions.) > > 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. Kind regards, Matthias van de Meent Databricks (https://www.databricks.com)
Matthias van de Meent <boekewurm+postgres@gmail.com> writes:
> On Tue, 30 Dec 2025 at 16:25, Tom Lane <tgl@sss.pgh.pa.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