Feedback-ID: ia2694551:Fastmail
Date: Tue, 21 Nov 2023 19:24:58 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Cc: Andy Fan <zhihui.fan1213@gmail.com>, 
	Lepikhov Andrei <a.lepikhov@postgrespro.ru>, jian he <jian.universality@gmail.com>, 
	David Rowley <dgrowleyml@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Report planning memory in EXPLAIN ANALYZE
Message-ID: <202311211824.da7igma5vvrk@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="zfxm2lkelrgss33p"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAExHW5sCJX7696sF-OnugAiaXS=Ag95=-m1cSrjcmyYj8Pduuw@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/202311211824.da7igma5vvrk%40alvherre.pgsql>
Precedence: bulk


--zfxm2lkelrgss33p
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

I gave this a quick look.  I think the usefulness aspect is already
established in general terms; the bit I'm not so sure about is whether
we want it enabled by default.  For many cases it'd just be noise.
Perhaps we want it hidden behind something like "EXPLAIN (MEMORY)" or
such, particularly since things like "allocated" (which, per David,
seems to be the really useful metric) seems too much a PG-developer
value rather than an end-user value.

If EXPLAIN (MEMORY) is added, then probably auto_explain needs a
corresponding flag, and doc updates.

Some code looks to be in weird places.  Why is calc_mem_usage, which
operates on MemoryContextCounters, in explain.c instead of mcxt.c?
why is MemUsage in explain.h instead of memnodes.h?  I moved both.  I
also renamed them, to MemoryContextSizeDifference() and MemoryUsage
respectively; fixup patch attached.

I see no reason for this to be three separate patches anymore.

The EXPLAIN docs (explain.sgml) need an update to mention the new flag
and the new output, too.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

--zfxm2lkelrgss33p
Content-Type: text/plain; charset=utf-8
Content-Disposition: attachment; filename="review.txt"
Content-Transfer-Encoding: 8bit

commit 48b0c6682a9e8cf07096b979693fac09b2f7a0ba
Author:     Alvaro Herrera <alvherre@alvh.no-ip.org> [Álvaro Herrera <alvherre@alvh.no-ip.org>]
AuthorDate: Tue Nov 21 18:20:32 2023 +0100
CommitDate: Tue Nov 21 19:18:18 2023 +0100

    review

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 9cd9b577c7..8c7f27b661 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -123,7 +123,7 @@ static void show_buffer_usage(ExplainState *es, const BufferUsage *usage,
 							  bool planning);
 static void show_wal_usage(ExplainState *es, const WalUsage *usage);
 static void show_planning_memory(ExplainState *es,
-								 const MemUsage *usage);
+								 const MemoryUsage *usage);
 static void ExplainIndexScanDetails(Oid indexid, ScanDirection indexorderdir,
 									ExplainState *es);
 static void ExplainScanTarget(Scan *plan, ExplainState *es);
@@ -395,14 +395,14 @@ ExplainOneQuery(Query *query, int cursorOptions,
 	else
 	{
 		PlannedStmt *plan;
+		MemoryContext planner_ctx;
 		instr_time	planstart,
 					planduration;
 		BufferUsage bufusage_start,
 					bufusage;
 		MemoryContextCounters mem_counts_start;
 		MemoryContextCounters mem_counts_end;
-		MemUsage	mem_usage;
-		MemoryContext planner_ctx;
+		MemoryUsage	mem_usage;
 		MemoryContext saved_ctx;
 
 		/*
@@ -415,7 +415,6 @@ ExplainOneQuery(Query *query, int cursorOptions,
 		planner_ctx = AllocSetContextCreate(CurrentMemoryContext,
 											"explain analyze planner context",
 											ALLOCSET_DEFAULT_SIZES);
-
 		if (es->buffers)
 			bufusage_start = pgBufferUsage;
 		MemoryContextMemConsumed(planner_ctx, &mem_counts_start);
@@ -429,7 +428,7 @@ ExplainOneQuery(Query *query, int cursorOptions,
 		INSTR_TIME_SUBTRACT(planduration, planstart);
 		MemoryContextSwitchTo(saved_ctx);
 		MemoryContextMemConsumed(planner_ctx, &mem_counts_end);
-		calc_mem_usage(&mem_usage, &mem_counts_end, &mem_counts_start);
+		MemoryContextSizeDifference(&mem_usage, &mem_counts_start, &mem_counts_end);
 
 		/* calc differences of buffer counters. */
 		if (es->buffers)
@@ -551,7 +550,7 @@ void
 ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es,
 			   const char *queryString, ParamListInfo params,
 			   QueryEnvironment *queryEnv, const instr_time *planduration,
-			   const BufferUsage *bufusage, const MemUsage *mem_usage)
+			   const BufferUsage *bufusage, const MemoryUsage *mem_usage)
 {
 	DestReceiver *dest;
 	QueryDesc  *queryDesc;
@@ -656,9 +655,9 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es,
 
 	if (es->summary && mem_usage)
 	{
-		ExplainOpenGroup("Planning Memory", "Planning Memory", true, es);
+		ExplainOpenGroup("Planner Memory", "Planner Memory", true, es);
 		show_planning_memory(es, mem_usage);
-		ExplainCloseGroup("Planning Memory", "Planning Memory", true, es);
+		ExplainCloseGroup("Planner Memory", "Planner Memory", true, es);
 	}
 
 	/* Print info about runtime of triggers */
@@ -3801,45 +3800,22 @@ show_wal_usage(ExplainState *es, const WalUsage *usage)
  * Show planner's memory usage details.
  */
 static void
-show_planning_memory(ExplainState *es, const MemUsage *usage)
+show_planning_memory(ExplainState *es, const MemoryUsage *usage)
 {
 	if (es->format == EXPLAIN_FORMAT_TEXT)
 	{
 		appendStringInfo(es->str,
-						 "Planning Memory: used=%zu bytes allocated=%zu bytes",
-						 usage->mem_used, usage->mem_allocated);
+						 "Planner Memory: used=%zu bytes allocated=%zu bytes",
+						 usage->bytes_used, usage->bytes_allocated);
 		appendStringInfoChar(es->str, '\n');
 	}
 	else
 	{
-		ExplainPropertyInteger("Used", "bytes", usage->mem_used, es);
-		ExplainPropertyInteger("Allocated", "bytes", usage->mem_allocated, es);
+		ExplainPropertyInteger("Used", "bytes", usage->bytes_used, es);
+		ExplainPropertyInteger("Allocated", "bytes", usage->bytes_allocated, es);
 	}
 }
 
-/*
- * Compute memory usage from the start and end memory counts.
- */
-void
-calc_mem_usage(MemUsage *mem_usage, MemoryContextCounters *mem_counts_end,
-			   MemoryContextCounters *mem_counts_start)
-{
-	Size		mem_used_start;
-	Size		mem_used_end;
-
-	mem_used_start = mem_counts_start->totalspace - mem_counts_start->freespace;
-	mem_used_end = mem_counts_end->totalspace - mem_counts_end->freespace;
-
-	mem_usage->mem_used = mem_used_end - mem_used_start;
-
-	/*
-	 * The net memory used is from total memory allocated and not necessarily
-	 * the net memory allocated between the two given samples. Hence do not
-	 * compute the difference between allocated memory reported in the two
-	 * given samples.
-	 */
-	mem_usage->mem_allocated = mem_counts_end->totalspace;
-}
 
 /*
  * Add some additional details about an IndexScan or IndexOnlyScan
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index eb39823d7a..3d3d0ae6a3 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -585,7 +585,7 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es,
 				bufusage;
 	MemoryContextCounters mem_counts_start;
 	MemoryContextCounters mem_counts_end;
-	MemUsage	mem_usage;
+	MemoryUsage	mem_usage;
 	MemoryContext planner_ctx;
 	MemoryContext saved_ctx;
 
@@ -643,7 +643,7 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es,
 	INSTR_TIME_SUBTRACT(planduration, planstart);
 	MemoryContextSwitchTo(saved_ctx);
 	MemoryContextMemConsumed(planner_ctx, &mem_counts_end);
-	calc_mem_usage(&mem_usage, &mem_counts_end, &mem_counts_start);
+	MemoryContextSizeDifference(&mem_usage, &mem_counts_start, &mem_counts_end);
 
 	/* calc differences of buffer counters. */
 	if (es->buffers)
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 94159a6799..28e5f36405 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -687,6 +687,32 @@ MemoryContextMemAllocated(MemoryContext context, bool recurse)
 	return total;
 }
 
+/*
+ * Compute memory usage from the start and end memory counts.
+ */
+void
+MemoryContextSizeDifference(MemoryUsage *mem_usage,
+							MemoryContextCounters *start,
+							MemoryContextCounters *end)
+{
+	/*
+	 * We compute the memory "used" as the difference, between end situation
+	 * and start situation, of the memory that's allocated according to the
+	 * counters, excluding memory in freelists.
+	 */
+	mem_usage->bytes_used =
+		(end->totalspace - end->freespace) -
+		(start->totalspace - start->freespace);
+
+	/*
+	 * The net memory used is from total memory allocated and not necessarily
+	 * the net memory allocated between the two given samples. Hence do not
+	 * compute the difference between allocated memory reported in the two
+	 * given samples.
+	 */
+	mem_usage->bytes_allocated = end->totalspace;
+}
+
 /*
  * MemoryContextStats
  *		Print statistics about the named context and all its descendants.
@@ -752,11 +778,11 @@ MemoryContextStatsDetail(MemoryContext context, int max_children,
  */
 extern void
 MemoryContextMemConsumed(MemoryContext context,
-						 MemoryContextCounters *mem_consumed)
+						 MemoryContextCounters *consumed)
 {
-	memset(mem_consumed, 0, sizeof(*mem_consumed));
+	memset(consumed, 0, sizeof(*consumed));
 
-	MemoryContextStatsInternal(context, 0, false, 100, mem_consumed, false);
+	MemoryContextStatsInternal(context, 0, false, 0, consumed, false);
 }
 
 /*
diff --git a/src/include/commands/explain.h b/src/include/commands/explain.h
index 05dcb91c49..6947cbae8b 100644
--- a/src/include/commands/explain.h
+++ b/src/include/commands/explain.h
@@ -62,12 +62,6 @@ typedef struct ExplainState
 	ExplainWorkersState *workers_state; /* needed if parallel plan */
 } ExplainState;
 
-typedef struct MemUsage
-{
-	Size		mem_used;
-	Size		mem_allocated;
-} MemUsage;
-
 /* Hook for plugins to get control in ExplainOneQuery() */
 typedef void (*ExplainOneQuery_hook_type) (Query *query,
 										   int cursorOptions,
@@ -99,7 +93,7 @@ extern void ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into,
 						   ParamListInfo params, QueryEnvironment *queryEnv,
 						   const instr_time *planduration,
 						   const BufferUsage *bufusage,
-						   const MemUsage *mem_usage);
+						   const MemoryUsage *mem_usage);
 
 extern void ExplainPrintPlan(ExplainState *es, QueryDesc *queryDesc);
 extern void ExplainPrintTriggers(ExplainState *es, QueryDesc *queryDesc);
@@ -132,8 +126,5 @@ extern void ExplainOpenGroup(const char *objtype, const char *labelname,
 							 bool labeled, ExplainState *es);
 extern void ExplainCloseGroup(const char *objtype, const char *labelname,
 							  bool labeled, ExplainState *es);
-extern void calc_mem_usage(MemUsage *mem_usage,
-						   MemoryContextCounters *mem_counts_end,
-						   MemoryContextCounters *mem_counts_start);
 
 #endif							/* EXPLAIN_H */
diff --git a/src/include/nodes/memnodes.h b/src/include/nodes/memnodes.h
index ff6453bb7a..577d39cef4 100644
--- a/src/include/nodes/memnodes.h
+++ b/src/include/nodes/memnodes.h
@@ -34,6 +34,16 @@ typedef struct MemoryContextCounters
 	Size		freespace;		/* The unused portion of totalspace */
 } MemoryContextCounters;
 
+/*
+ * MemoryUsage
+ *		For concise reporting of memory consumption
+ */
+typedef struct MemoryUsage
+{
+	Size		bytes_used;
+	Size		bytes_allocated;
+} MemoryUsage;
+
 /*
  * MemoryContext
  *		A logical context in which memory allocations occur.
diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h
index 50b9cc06e3..24dc0e996e 100644
--- a/src/include/utils/memutils.h
+++ b/src/include/utils/memutils.h
@@ -84,13 +84,16 @@ extern Size GetMemoryChunkSpace(void *pointer);
 extern MemoryContext MemoryContextGetParent(MemoryContext context);
 extern bool MemoryContextIsEmpty(MemoryContext context);
 extern Size MemoryContextMemAllocated(MemoryContext context, bool recurse);
+extern void MemoryContextMemConsumed(MemoryContext context,
+									 MemoryContextCounters *consumed);
+extern void MemoryContextSizeDifference(MemoryUsage *mem_usage,
+										MemoryContextCounters *start,
+										MemoryContextCounters *end);
 extern void MemoryContextStats(MemoryContext context);
 extern void MemoryContextStatsDetail(MemoryContext context, int max_children,
 									 bool print_to_stderr);
 extern void MemoryContextAllowInCriticalSection(MemoryContext context,
 												bool allow);
-extern void MemoryContextMemConsumed(MemoryContext context,
-									 MemoryContextCounters *mem_consumed);
 
 #ifdef MEMORY_CONTEXT_CHECKING
 extern void MemoryContextCheck(MemoryContext context);
diff --git a/src/test/regress/expected/explain.out b/src/test/regress/expected/explain.out
index e4b1aa7fdf..48e4ad1c3f 100644
--- a/src/test/regress/expected/explain.out
+++ b/src/test/regress/expected/explain.out
@@ -65,7 +65,7 @@ select explain_filter('explain (analyze) select * from int8_tbl i8');
 -----------------------------------------------------------------------------------------------
  Seq Scan on int8_tbl i8  (cost=N.N..N.N rows=N width=N) (actual time=N.N..N.N rows=N loops=N)
  Planning Time: N.N ms
- Planning Memory: used=N bytes allocated=N bytes
+ Planner Memory: used=N bytes allocated=N bytes
  Execution Time: N.N ms
 (4 rows)
 
@@ -75,7 +75,7 @@ select explain_filter('explain (analyze, verbose) select * from int8_tbl i8');
  Seq Scan on public.int8_tbl i8  (cost=N.N..N.N rows=N width=N) (actual time=N.N..N.N rows=N loops=N)
    Output: q1, q2
  Planning Time: N.N ms
- Planning Memory: used=N bytes allocated=N bytes
+ Planner Memory: used=N bytes allocated=N bytes
  Execution Time: N.N ms
 (5 rows)
 
@@ -84,7 +84,7 @@ select explain_filter('explain (analyze, buffers, format text) select * from int
 -----------------------------------------------------------------------------------------------
  Seq Scan on int8_tbl i8  (cost=N.N..N.N rows=N width=N) (actual time=N.N..N.N rows=N loops=N)
  Planning Time: N.N ms
- Planning Memory: used=N bytes allocated=N bytes
+ Planner Memory: used=N bytes allocated=N bytes
  Execution Time: N.N ms
 (4 rows)
 
@@ -131,10 +131,10 @@ select explain_filter('explain (analyze, buffers, format xml) select * from int8
        <Temp-Written-Blocks>N</Temp-Written-Blocks>    +
      </Planning>                                       +
      <Planning-Time>N.N</Planning-Time>                +
-     <Planning-Memory>                                 +
+     <Planner-Memory>                                  +
        <Used>N</Used>                                  +
        <Allocated>N</Allocated>                        +
-     </Planning-Memory>                                +
+     </Planner-Memory>                                 +
      <Triggers>                                        +
      </Triggers>                                       +
      <Execution-Time>N.N</Execution-Time>              +
@@ -181,7 +181,7 @@ select explain_filter('explain (analyze, buffers, format yaml) select * from int
      Temp Read Blocks: N      +
      Temp Written Blocks: N   +
    Planning Time: N.N         +
-   Planning Memory:           +
+   Planner Memory:            +
      Used: N                  +
      Allocated: N             +
    Triggers:                  +
@@ -294,7 +294,7 @@ select explain_filter('explain (analyze, buffers, format json) select * from int
        "Temp I/O Write Time": N.N   +
      },                             +
      "Planning Time": N.N,          +
-     "Planning Memory": {           +
+     "Planner Memory": {            +
        "Used": N,                   +
        "Allocated": N               +
      },                             +
@@ -550,7 +550,7 @@ select jsonb_pretty(
          ],                                                 +
          "Planning Time": 0.0,                              +
          "Execution Time": 0.0,                             +
-         "Planning Memory": {                               +
+         "Planner Memory": {                                +
              "Used": 0,                                     +
              "Allocated": 0                                 +
          }                                                  +

--zfxm2lkelrgss33p--