vtomas-0002-review.patch

From 0ef536a26403973b081fd14de79e047cd0cd1a13 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas@vondra.me>
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