Hi Team,
As a follow-up to the skip scan regression discussion, I tested a small
patch that introduces *static allocation/caching of `IndexAmRoutine` *objects
in `amapi.c`, removing the malloc/free overhead.
*Test setup :*
- Baseline: PG17 (commit before skip scan)
- After: PG18 build with skip scan (patched)
- pgbench scale=1, 100 partitions
- Query: `select count(*) from pgbench_accounts where bid = 0`
- Clients: 1, 4, 32
- Protocols: simple, prepared
*Results (tps, 10s runs) :*
Mode Clients Before (PG17) After (PG18 w/ static fix)
simple 1 23856 20332 (~15% lower)
simple 4 55299 53184 (~4% lower)
simple 32 79779 78347 (~2% lower)
prepared 1 26364 26615 (no regression)
prepared 4 55784 54437 (~2% lower)
prepared 32 84687 80374 (~5% lower)
This shows the static fix eliminates the severe ~50% regression previously
observed by Tomas, leaving only a small residual slowdown (*~2-15%*).
*Patch summary :*
- Cache `IndexAmRoutine` instances per AM OID instead of malloc/free per
call.
- Avoid `pfree(amroutine)` in hot paths.
- Keeps allocations stable across lookups, reducing malloc churn.
*Proposal :*
I suggest adopting this static allocation approach for PG18 to prevent
performance cliffs. Longer term, we can explore lighter-weight caching
mechanisms or further executor tuning.
*Patch attached for discussion.*
Thanks & Regards,
Athiyaman M
On Sat, Aug 30, 2025 at 4:37 AM Tomas Vondra <to...@vondra.me> wrote:
> On 8/29/25 21:03, Peter Geoghegan wrote:
> > On Fri, Aug 29, 2025 at 9:10 AM Tomas Vondra <to...@vondra.me> wrote:
> >> Peter, any thoughts on this. Do you think it's reasonable / feasible to
> >> push the fix?
> >
> > I don't feel comfortable pushing that fix today.
> >
>
> Understood.
>
> > Honestly, I'm still not sure what to do. My proposal was to just
> > remove the totally unused options support function, which is probably
> > fine. But since I don't really know why Alexander ever added the
> > "options" support function in the first place (I don't even see a
> > theoretical benefit), I'm not quite prepared to say that I know that
> > it's okay to remove it now.
> >
>
> Right. I think removing the "options" is the only feasible solution for
> PG18 at this point. Either that or nothing. The other patch is far too
> invasive.
>
> As for why the support procedure was added to existing index AMs, I
> don't know. I suppose it as mostly for consistency, so that custom
> oclasses could opclasses could use that. I have no idea if there are
> plausible custom opclasses using this.
>
> I'm not sure how I feel about removing the support proc. It feels pretty
> arbitrary and fragile, and IIRC it doesn't even address the perf issue
> (add a couple partitions and it'll hit the same issue). It just restores
> the "threshold" to where it was for PG17. And it's fragile, because we
> have no protections about hitting this glibc-specific behavior again. It
> takes one new flag added somewhere, and we'll not even notice it.
>
> So after thinking about this a bit more, and refreshing the context, I
> think the right solution for PG18 is to do nothing.
>
> regards
>
> --
> Tomas Vondra
>
>
>
>
From c80b2a07733112e23b70ed5b309f177f6bf79ef6 Mon Sep 17 00:00:00 2001
From: athiyaman-m <heroathi...@gmail.com>
Date: Wed, 10 Sep 2025 12:08:56 +0530
Subject: [PATCH] Use static allocation for IndexAmRoutine in amapi.c to reduce
malloc/free overhead
This avoids repeated palloc/free cycles by keeping the routine
structure in static memory. Initial pgbench tests show that this
does not regress performance and may slightly improve efficiency
at higher client counts.
Signed-off-by: athiyaman-m <heroathi...@gmail.com>
---
src/backend/access/index/amapi.c | 81 +++++++++++++++++++++++++++++---
1 file changed, 75 insertions(+), 6 deletions(-)
diff --git a/src/backend/access/index/amapi.c b/src/backend/access/index/amapi.c
index d6b8dad4d5..d5481569d9 100644
--- a/src/backend/access/index/amapi.c
+++ b/src/backend/access/index/amapi.c
@@ -13,12 +13,19 @@
*/
#include "postgres.h"
+#include <string.h>
+
#include "access/amapi.h"
#include "access/htup_details.h"
#include "catalog/pg_am.h"
#include "catalog/pg_opclass.h"
#include "utils/fmgrprotos.h"
#include "utils/syscache.h"
+#include "utils/hsearch.h"
+#include "utils/memutils.h"
+#include "utils/elog.h"
+#include "utils/pg_locale.h"
+#include "utils/builtins.h"
/*
@@ -51,6 +58,10 @@ GetIndexAmRoutine(Oid amhandler)
*
* If the given OID isn't a valid index access method, returns NULL if
* noerror is true, else throws error.
+ *
+ * This implementation caches a copy of the IndexAmRoutine per handler OID
+ * in TopMemoryContext so callers get a stable pointer and we avoid repeated
+ * per-call allocations/freeing of the routine struct.
*/
IndexAmRoutine *
GetIndexAmRoutineByAmId(Oid amoid, bool noerror)
@@ -65,8 +76,7 @@ GetIndexAmRoutineByAmId(Oid amoid, bool noerror)
{
if (noerror)
return NULL;
- elog(ERROR, "cache lookup failed for access method %u",
- amoid);
+ elog(ERROR, "cache lookup failed for access method %u", amoid);
}
amform = (Form_pg_am) GETSTRUCT(tuple);
@@ -102,8 +112,67 @@ GetIndexAmRoutineByAmId(Oid amoid, bool noerror)
ReleaseSysCache(tuple);
- /* And finally, call the handler function to get the API struct. */
- return GetIndexAmRoutine(amhandler);
+ /*
+ * Cache management: keep a single HTAB (in TopMemoryContext) that maps
+ * amhandler OIDs to a stored copy of IndexAmRoutine.
+ *
+ * Use HASH_BLOBS and store the copy of IndexAmRoutine inline in the entry.
+ */
+ {
+ typedef struct IndexAmRoutineCacheEntry
+ {
+ IndexAmRoutine routine; /* stored inline */
+ } IndexAmRoutineCacheEntry;
+
+ static HTAB *IndexAmRoutine_cache = NULL;
+
+ if (IndexAmRoutine_cache == NULL)
+ {
+ HASHCTL ctl;
+
+ /* initialize hash control structure */
+ MemSet(&ctl, 0, sizeof(ctl));
+ ctl.keysize = sizeof(Oid);
+ ctl.entrysize = sizeof(IndexAmRoutineCacheEntry);
+ ctl.hcxt = TopMemoryContext;
+
+ /* create cache in TopMemoryContext; small fixed initial size */
+ IndexAmRoutine_cache = hash_create("IndexAmRoutine cache",
+ 8,
+ &ctl,
+ HASH_ELEM | HASH_BLOBS);
+ }
+
+ /* call handler to get the runtime-provided struct (may be palloc'd) */
+ IndexAmRoutine *runtime_routine = GetIndexAmRoutine(amhandler);
+ bool found;
+ IndexAmRoutineCacheEntry *entry;
+
+ /* find or create entry; key is the amhandler Oid */
+ entry = (IndexAmRoutineCacheEntry *) hash_search(IndexAmRoutine_cache,
+ (void *)&amhandler,
+ HASH_ENTER,
+ &found);
+ if (!found)
+ {
+ /* copy runtime struct into persistent cache entry */
+ /* ensure we don't copy past the size of IndexAmRoutine */
+ memcpy(&entry->routine, runtime_routine, sizeof(IndexAmRoutine));
+ }
+
+ /*
+ * Note: we intentionally do not pfree(runtime_routine) here. The
+ * handler may have returned a pointer into static memory or memory
+ * allocated in some context. Freeing it blindly may be unsafe. The
+ * runtime allocation (if any) will be at most one small struct per
+ * handler and is acceptable for this prototype. A more polished
+ * implementation could detect and free an allocated pointer when
+ * safe.
+ */
+
+ /* return pointer to the cached copy (in TopMemoryContext) */
+ return &entry->routine;
+ }
}
@@ -187,7 +256,7 @@ amvalidate(PG_FUNCTION_ARGS)
result = amroutine->amvalidate(opclassoid);
- pfree(amroutine);
+ /* Previously we pfree(amroutine); but routine is now cached in TopMemoryContext. */
PG_RETURN_BOOL(result);
-}
+}
\ No newline at end of file
--
2.39.5
