From 8861e8a323843610d47646db9a4e4a9217924f61 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Tue, 1 Oct 2024 22:07:51 +0300 Subject: [PATCH] Fix the size of the perf counters shared memory array (#9226) MaxBackends doesn't include auxiliary processes. Whenever an aux process made IO operations that updated the counters, they would scribble over shared memory beoynd the end of the array. The relsize cache hash table comes after the array, so the symptom was an error about hash table corruption in the relsize cache hash. --- pgxn/neon/neon_perf_counters.c | 9 +++++---- pgxn/neon/neon_perf_counters.h | 8 ++++++++ pgxn/neon/pagestore_smgr.c | 14 ++++++++++++++ 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/pgxn/neon/neon_perf_counters.c b/pgxn/neon/neon_perf_counters.c index eecbfe98c6..9bce81bf2e 100644 --- a/pgxn/neon/neon_perf_counters.c +++ b/pgxn/neon/neon_perf_counters.c @@ -27,7 +27,8 @@ NeonPerfCountersShmemSize(void) { Size size = 0; - size = add_size(size, mul_size(MaxBackends, sizeof(neon_per_backend_counters))); + size = add_size(size, mul_size(NUM_NEON_PERF_COUNTER_SLOTS, + sizeof(neon_per_backend_counters))); return size; } @@ -39,7 +40,7 @@ NeonPerfCountersShmemInit(void) neon_per_backend_counters_shared = ShmemInitStruct("Neon perf counters", - mul_size(MaxBackends, + mul_size(NUM_NEON_PERF_COUNTER_SLOTS, sizeof(neon_per_backend_counters)), &found); Assert(found == IsUnderPostmaster); @@ -192,7 +193,7 @@ neon_get_backend_perf_counters(PG_FUNCTION_ARGS) /* We put all the tuples into a tuplestore in one go. */ InitMaterializedSRF(fcinfo, 0); - for (int procno = 0; procno < MaxBackends; procno++) + for (int procno = 0; procno < NUM_NEON_PERF_COUNTER_SLOTS; procno++) { PGPROC *proc = GetPGProcByNumber(procno); int pid = proc->pid; @@ -231,7 +232,7 @@ neon_get_perf_counters(PG_FUNCTION_ARGS) InitMaterializedSRF(fcinfo, 0); /* Aggregate the counters across all backends */ - for (int procno = 0; procno < MaxBackends; procno++) + for (int procno = 0; procno < NUM_NEON_PERF_COUNTER_SLOTS; procno++) { neon_per_backend_counters *counters = &neon_per_backend_counters_shared[procno]; diff --git a/pgxn/neon/neon_perf_counters.h b/pgxn/neon/neon_perf_counters.h index 02163ada55..49d477c4f8 100644 --- a/pgxn/neon/neon_perf_counters.h +++ b/pgxn/neon/neon_perf_counters.h @@ -96,6 +96,14 @@ typedef struct /* Pointer to the shared memory array of neon_per_backend_counters structs */ extern neon_per_backend_counters *neon_per_backend_counters_shared; +/* + * Size of the perf counters array in shared memory. One slot for each backend + * and aux process. IOW one for each PGPROC slot, except for slots reserved + * for prepared transactions, because they're not real processes and cannot do + * I/O. + */ +#define NUM_NEON_PERF_COUNTER_SLOTS (MaxBackends + NUM_AUXILIARY_PROCS) + #if PG_VERSION_NUM >= 170000 #define MyNeonCounters (&neon_per_backend_counters_shared[MyProcNumber]) #else diff --git a/pgxn/neon/pagestore_smgr.c b/pgxn/neon/pagestore_smgr.c index 1c87f4405c..155756f8b3 100644 --- a/pgxn/neon/pagestore_smgr.c +++ b/pgxn/neon/pagestore_smgr.c @@ -1773,6 +1773,20 @@ neon_init(void) if (MyPState != NULL) return; + /* + * Sanity check that theperf counters array is sized correctly. We got + * this wrong once, and the formula for max number of backends and aux + * processes might well change in the future, so better safe than sorry. + * This is a very cheap check so we do it even without assertions. On + * v14, this gets called before initializing MyProc, so we cannot perform + * the check here. That's OK, we don't expect the logic to change in old + * releases. + */ +#if PG_VERSION_NUM>=150000 + if (MyNeonCounters >= &neon_per_backend_counters_shared[NUM_NEON_PERF_COUNTER_SLOTS]) + elog(ERROR, "MyNeonCounters points past end of array"); +#endif + prfs_size = offsetof(PrefetchState, prf_buffer) + sizeof(PrefetchRequest) * readahead_buffer_size;