From 0ef536a26403973b081fd14de79e047cd0cd1a13 Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Mon, 6 Jan 2025 19:22:14 +0100 Subject: [PATCH vtomas 2/2] review --- src/backend/utils/adt/mcxtfuncs.c | 109 +++++++++++++++++++++++++----- src/backend/utils/mmgr/mcxt.c | 93 +++++++++++++++++-------- src/include/utils/memutils.h | 5 +- 3 files changed, 159 insertions(+), 48 deletions(-) diff --git a/src/backend/utils/adt/mcxtfuncs.c b/src/backend/utils/adt/mcxtfuncs.c index c067cdf8709..f3ffd6937a5 100644 --- a/src/backend/utils/adt/mcxtfuncs.c +++ b/src/backend/utils/adt/mcxtfuncs.c @@ -307,16 +307,26 @@ pg_log_backend_memory_contexts(PG_FUNCTION_ARGS) * pg_get_process_memory_contexts * Signal a backend or an auxiliary process to send its memory contexts. * + * By default, only superusers are allowed to signal to return the memory + * contexts because allowing any users to issue this request at an unbounded + * rate would cause lots of log messages and which can lead to denial of + * service. Additional roles can be permitted with GRANT. + * * On receipt of this signal, a backend or an auxiliary process sets the flag * in the signal handler, which causes the next CHECK_FOR_INTERRUPTS() - * or process-specific interrupt handler to copy the memory context statistics - * in a dynamic shared memory space. The statistics for contexts that do not fit in - * shared memory area are stored as a cumulative total of those contexts, - * at the end in the dynamic shared memory. - * Wait for the backend to send signal on the condition variable after - * writing statistics to a shared memory. - * Once condition variable comes out of sleep, check if the required - * backends statistics are available to read and display. + * or process-specific interrupt handler to copy the memory context details + * to a dynamic shared memory space. + * + * The shared memory buffer has a limited size - it the process has too many + * memory contexts, the memory contexts into that do not fit are summarized + * and represented as cumulative total at the end of the buffer. + * + * Once condition variable comes out of sleep, check if the memory context + * information is available for read and display. + * + * XXX Explain how the backends communicate through condition variables. + * + * XXX Explain what happens with timeouts, etc. */ Datum pg_get_process_memory_contexts(PG_FUNCTION_ARGS) @@ -333,6 +343,7 @@ pg_get_process_memory_contexts(PG_FUNCTION_ARGS) MemoryContext oldContext; int num_retries = 0; + /* XXX Shouldn't this be after the privilege check etc.? */ InitMaterializedSRF(fcinfo, 0); /* @@ -383,24 +394,50 @@ pg_get_process_memory_contexts(PG_FUNCTION_ARGS) /* * Return statistics of top level 1 and 2 contexts, if get_summary is * true. + * + * XXX Is this comment still accurate? Or are we returning information + * about more contexts? Or more precisely, isn't that irrelevant here? + * We just process whatever the process puts into the DSA, right? + * + * XXX I'd move this comment until after we wake up and are ready to + * process the information, close to the comment: + * + * Backend has finished publishing the stats, read them */ - LWLockAcquire(&memCtxState[procNumber].lw_lock, LW_EXCLUSIVE); - memCtxState[procNumber].get_summary = get_summary; /* * Create a DSA segment with maximum size of 16MB, send handle to the * publishing process for storing the stats. If number of contexts exceed * 16MB, a cumulative total is stored for such contexts. + * + * XXX 16MB seems like an awfully large amount of memory, particularly + * for small machines. Maybe it should be configurable as a parameter + * of the SQL function? In any case, it should not be hardcoded as a + * magic constant. Maybe add a #define constant? */ + LWLockAcquire(&memCtxState[procNumber].lw_lock, LW_EXCLUSIVE); + + memCtxState[procNumber].get_summary = get_summary; + if (memCtxState[procNumber].memstats_dsa_handle == DSA_HANDLE_INVALID) { oldContext = MemoryContextSwitchTo(TopMemoryContext); - area = dsa_create_ext(memCtxState[procNumber].lw_lock.tranche, DSA_DEFAULT_INIT_SEGMENT_SIZE, + + area = dsa_create_ext(memCtxState[procNumber].lw_lock.tranche, + DSA_DEFAULT_INIT_SEGMENT_SIZE, 16 * DSA_DEFAULT_INIT_SEGMENT_SIZE); + MemoryContextSwitchTo(oldContext); + handle = dsa_get_handle(area); + memCtxState[procNumber].memstats_dsa_handle = handle; - /* Pin the mapping so that it doesn't throw a warning */ + + /* Pin the mapping so that it doesn't throw a warning + * + * XXX We don't pin stuff "so that it doesn't throw a warning". Surely + * the warning has a reason, so maybe mention that? + */ dsa_pin(area); dsa_pin_mapping(area); } @@ -409,11 +446,20 @@ pg_get_process_memory_contexts(PG_FUNCTION_ARGS) area = dsa_attach(memCtxState[procNumber].memstats_dsa_handle); dsa_pin_mapping(area); } + LWLockRelease(&memCtxState[procNumber].lw_lock); + + /* + * Send a signal to the auxiliary process, informing it we want it to + * produce information about memory contexts. + */ if (SendProcSignal(pid, PROCSIG_GET_MEMORY_CONTEXT, procNumber) < 0) { ereport(WARNING, (errmsg("could not send signal to process %d: %m", pid))); + + /* XXX We do exactly this in a number of places. Maybe it'd be better + * to define an "error" label at the end, and goto to it? */ dsa_detach(area); PG_RETURN_NULL(); } @@ -461,13 +507,22 @@ pg_get_process_memory_contexts(PG_FUNCTION_ARGS) */ LWLockAcquire(&memCtxState[procNumber].lw_lock, LW_EXCLUSIVE); - if (memCtxState[procNumber].proc_id == pid && DsaPointerIsValid(memCtxState[procNumber].memstats_dsa_pointer)) + /* + * XXX Explain how could it happen that the PID does not match. + */ + if ((memCtxState[procNumber].proc_id == pid) && + DsaPointerIsValid(memCtxState[procNumber].memstats_dsa_pointer)) break; else LWLockRelease(&memCtxState[procNumber].lw_lock); } + if (DsaPointerIsValid(memCtxState[procNumber].memstats_dsa_pointer)) memctx_info = (MemoryContextInfo *) dsa_get_address(area, memCtxState[procNumber].memstats_dsa_pointer); + + /* XXX What if the memstats_dsa_pointer is not valid? Is it even possible? + * If it is, we have garbage in memctx_info. Maybe should be an Assert()? */ + /* Backend has finished publishing the stats, read them */ for (i = 0; i < memCtxState[procNumber].in_memory_stats; i++) { @@ -489,17 +544,21 @@ pg_get_process_memory_contexts(PG_FUNCTION_ARGS) nulls[1] = true; values[2] = CStringGetTextDatum(memctx_info[i].type); + path_length = memctx_info[i].path_length; path_array = construct_array_builtin(memctx_info[i].path, path_length, INT4OID); values[3] = PointerGetDatum(path_array); + values[4] = Int64GetDatum(memctx_info[i].totalspace); values[5] = Int64GetDatum(memctx_info[i].nblocks); values[6] = Int64GetDatum(memctx_info[i].freespace); values[7] = Int64GetDatum(memctx_info[i].freechunks); values[8] = Int64GetDatum(memctx_info[i].totalspace - memctx_info[i].freespace); values[9] = Int32GetDatum(memctx_info[i].num_contexts); + tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, values, nulls); } + /* If there are more contexts, display a cumulative total of those */ if (memCtxState[procNumber].total_stats > i) { @@ -516,29 +575,41 @@ pg_get_process_memory_contexts(PG_FUNCTION_ARGS) values[7] = Int64GetDatum(memctx_info[i].freechunks); values[8] = Int64GetDatum(memctx_info[i].totalspace - memctx_info[i].freespace); values[9] = Int32GetDatum(memctx_info[i].num_contexts); + tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, values, nulls); } + LWLockRelease(&memCtxState[procNumber].lw_lock); + ConditionVariableCancelSleep(); + dsa_detach(area); PG_RETURN_NULL(); } +/* + * Shared memory sizing for reporting memory context information. + */ static Size MemCtxShmemSize(void) { - Size size; - Size TotalProcs = add_size(MaxBackends, add_size(NUM_AUXILIARY_PROCS, max_prepared_xacts)); + Size TotalProcs = + add_size(MaxBackends, add_size(NUM_AUXILIARY_PROCS, max_prepared_xacts)); - size = TotalProcs * sizeof(MemoryContextState); - return size; + return mul_size(TotalProcs, sizeof(MemoryContextState)); } +/* + * Init shared memory for reporting memory context information. + * + * XXX Should this check IsUnderPostmaster, similarly to e.g. CommitTsShmemInit? + */ void MemCtxShmemInit(void) { bool found; - Size TotalProcs = add_size(MaxBackends, add_size(NUM_AUXILIARY_PROCS, max_prepared_xacts)); + Size TotalProcs = + add_size(MaxBackends, add_size(NUM_AUXILIARY_PROCS, max_prepared_xacts)); memCtxState = (MemoryContextState *) ShmemInitStruct("MemoryContextState", MemCtxShmemSize(), @@ -548,8 +619,10 @@ MemCtxShmemInit(void) for (int i = 0; i < TotalProcs; i++) { ConditionVariableInit(&memCtxState[i].memctx_cv); + LWLockInitialize(&memCtxState[i].lw_lock, LWLockNewTrancheId()); LWLockRegisterTranche(memCtxState[i].lw_lock.tranche, "mem_context_stats_reporting"); + memCtxState[i].memstats_dsa_handle = DSA_HANDLE_INVALID; memCtxState[i].memstats_dsa_pointer = InvalidDsaPointer; } diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c index 245aba5987c..8992a01ee32 100644 --- a/src/backend/utils/mmgr/mcxt.c +++ b/src/backend/utils/mmgr/mcxt.c @@ -178,7 +178,11 @@ static void MemoryContextStatsInternal(MemoryContext context, int level, static void MemoryContextStatsPrint(MemoryContext context, void *passthru, const char *stats_string, bool print_to_stderr); -static void PublishMemoryContext(MemoryContextInfo * memctx_infos, int curr_id, MemoryContext context, List *path, char *clipped_ident, MemoryContextCounters stat, int num_contexts); +static void PublishMemoryContext(MemoryContextInfo * memctx_infos, + int curr_id, MemoryContext context, + List *path, char *clipped_ident, + MemoryContextCounters stat, + int num_contexts); /* * You should not do memory allocations within a critical section, because @@ -1376,20 +1380,22 @@ ProcessLogMemoryContextInterrupt(void) } /* - * Run by each backend to publish their memory context - * statistics. It performs a breadth first search - * on the memory context tree, so that the parents - * get a chance to report stats before their children. + * ProcessGetMemoryContextInterrupt + * Generate information about memory contexts used by the process. * - * Statistics are shared via dynamic shared memory which - * can hold statistics of approx 6700 contexts. Remaining - * contexts statistics is captured as a cumulative total. + * Performs a breadth first search on the memory context tree, so that the + * parents get a chance to report stats before their children. + * + * Statistics are shared via dynamic shared memory which can hold statistics + * of approx 6700 contexts. Remaining contexts statistics is captured as a + * cumulative total. + * + * XXX Seems a bit fragile to mention the number of contexts here - if the + * DSA size changes (in mcxtfuncs.c), this will get stale. */ void ProcessGetMemoryContextInterrupt(void) { - /* Store the memory context details in shared memory */ - List *contexts; HASHCTL ctl; @@ -1407,21 +1413,18 @@ ProcessGetMemoryContextInterrupt(void) PublishMemoryContextPending = false; - /* - * The hash table is used for constructing "path" column of - * pg_get_process_memory_context is view, similar to its local backend - * couterpart. - */ - /* * Make a new context that will contain the hash table, to ease the - * cleanup + * cleanup. */ - stat_cxt = AllocSetContextCreate(CurrentMemoryContext, "Memory context statistics", ALLOCSET_DEFAULT_SIZES); + /* + * The hash table used for constructing "path" column of the view, similar + * to its local backend counterpart. + */ ctl.keysize = sizeof(MemoryContext); ctl.entrysize = sizeof(MemoryContextId); ctl.hcxt = stat_cxt; @@ -1431,39 +1434,65 @@ ProcessGetMemoryContextInterrupt(void) &ctl, HASH_ELEM | HASH_BLOBS | HASH_CONTEXT); + /* List of contexts to process in the next round - start at the top. */ contexts = list_make1(TopMemoryContext); /* Compute the number of stats that can fit in the DSM seg */ + /* + * XXX Don't hardcode the size in two places. Define a constant or + * something like that, so that we can change one place and it stays + * in sync. Or even better, define it just in mcxtfuncs.c, and store + * the size in the shmem. + * + * XXX The name is misleading - this is not the number of stats we're + * about to produce, it's the maximum number of entries we can fit into + * the shmem. I'd name this max_stats. + * + * XXX Also, what if we fill exactly this number of contexts? Won't we + * lose the last entry because it will be overwitten by the summary? + */ num_stats = floor(16 * DSA_DEFAULT_INIT_SEGMENT_SIZE / sizeof(MemoryContextInfo)); /* * Traverse the memory context tree to find total number of contexts. If * summary is requested find the total number of contexts at level 1 and * 2. + * + * XXX I'm confused about how this interacts with the get_summary flag. + * In fact, this always uses get_summary=false, because we only read the + * flag from shmem later. Seems like a bug. */ foreach_ptr(MemoryContextData, cur, contexts) { - MemoryContextId *entry; + MemoryContextId *entry; entry = (MemoryContextId *) hash_search(context_id_lookup, &cur, HASH_ENTER, &found); - stats_count = stats_count + 1; + Assert(!found); + /* context id starts with 1 */ - entry->context_id = stats_count; + entry->context_id = (++stats_count); - /* Append the children of the current context to the main list */ + /* Append the children of the current context to the main list. */ for (MemoryContext c = cur->firstchild; c != NULL; c = c->nextchild) { + /* XXX I don't understand why we need to check get_summary here? */ if (get_summary) { entry = (MemoryContextId *) hash_search(context_id_lookup, &c, HASH_ENTER, &found); - stats_count = stats_count + 1; - entry->context_id = stats_count; + Assert(!found); + + entry->context_id = (++stats_count); } + contexts = lappend(contexts, c); } - /* In summary only the first level contexts are displayed */ + + /* In summary only the first level contexts are displayed + * + * XXX Probably should say "first two levels"? + */ if (get_summary) break; } @@ -1474,23 +1503,30 @@ ProcessGetMemoryContextInterrupt(void) * segment, a cumulative total is written as the last record in the DSA * segment. */ - stats_count = stats_count > num_stats ? num_stats : stats_count; + stats_count = (stats_count > num_stats) ? num_stats : stats_count; /* Attach to DSA segment */ LWLockAcquire(&memCtxState[idx].lw_lock, LW_EXCLUSIVE); area = dsa_attach(memCtxState[idx].memstats_dsa_handle); + memCtxState[idx].proc_id = MyProcPid; + + /* XXX this is where we get the get_summary flag */ get_summary = memCtxState[idx].get_summary; - /* Free the memory allocated previously by the same process */ + /* Free the memory allocated previously by the same process. */ if (DsaPointerIsValid(memCtxState[idx].memstats_dsa_pointer)) { dsa_free(area, memCtxState[idx].memstats_dsa_pointer); memCtxState[idx].memstats_dsa_pointer = InvalidDsaPointer; } + memCtxState[idx].memstats_dsa_pointer = dsa_allocate0(area, stats_count * sizeof(MemoryContextInfo)); meminfo = (MemoryContextInfo *) dsa_get_address(area, memCtxState[idx].memstats_dsa_pointer); + /* XXX I find this really hard to understand, with the nested loops etc. + * I suggest breaking this up into smaller functions, and calling them + * (easier to understand) than huge lump of code. */ if (get_summary) { int ctx_id = 0; @@ -1506,7 +1542,7 @@ ProcessGetMemoryContextInterrupt(void) /* * Copy statistics for each of TopMemoryContexts children(XXX. Make it * capped at 100). This includes statistics of all of their children - * upto level 100 + * upto level 100. */ for (MemoryContext c = TopMemoryContext->firstchild; c != NULL; c = c->nextchild) { @@ -1651,6 +1687,7 @@ cleanup: dsa_detach(area); } +/* XXX this really needs some better formatting and comments */ static void PublishMemoryContext(MemoryContextInfo * memctx_info, int curr_id, MemoryContext context, List *path, char *clipped_ident, MemoryContextCounters stat, int num_contexts) { diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h index 9fac394aad3..49c7bf5c376 100644 --- a/src/include/utils/memutils.h +++ b/src/include/utils/memutils.h @@ -123,8 +123,9 @@ extern MemoryContext AllocSetContextCreateInternal(MemoryContext parent, Size maxBlockSize); /* Dynamic shared memory state for Memory Context Statistics reporting */ -typedef struct MemoryContextInfo +typedef struct MemoryContextInfo /* XXX I'd name this MemoryContextEntry */ { + /* XXX isn't 2 x 1kB for every context a bit too much? Maybe better to make it variable-length? */ char name[MEMORY_CONTEXT_IDENT_DISPLAY_SIZE]; char ident[MEMORY_CONTEXT_IDENT_DISPLAY_SIZE]; Datum path[MEM_CONTEXT_MAX_LEVEL]; @@ -135,7 +136,7 @@ typedef struct MemoryContextInfo int64 freespace; int64 freechunks; int num_contexts; -} MemoryContextInfo; +} MemoryContextInfo; /* XXX needs to be added to typedefs, so that pgindent works */ typedef struct MemoryContextState { -- 2.47.1