Re: Add memory/disk usage for WindowAgg nodes in EXPLAIN - Mailing list pgsql-hackers

David,

Thank you for your review.

> Yes, I think it's a good idea to move that into a helper function. If
> you do the other node types, without that helper the could would have
> to be repeated quite a few times. Maybe show_storage_info() can be
> moved up with the other helper functions, say below
> show_sortorder_options() ?

Yeah, that makes sense. Looks less random.

> It might be a good idea to keep the "if
> (!es->analyze || tupstore == NULL)" checks in the calling function
> rather than the helper too.

I agree with this. This kind of check should be done in the calling
function.

> I thought about the location of the test for a while and read the
> "This file is concerned with testing EXPLAIN in its own right."
> comment at the top of that explain.out.  I was trying to decide if
> testing output of a specific node type met this or not. I can't pick
> out any other tests there which are specific to a node type, so I'm
> unsure if this is the location for it or not. However, to put it
> anywhere else means having to add a plpgsql function to mask out the
> unstable parts of EXPLAIN, so maybe the location is good as it saves
> from having to do that. I'm 50/50 on this, so I'm happy to let you
> decide.

Yeah. Maybe we should move the function to elsewhere so that it can be
shared by other tests. However in this case it's purpose is testing an
additional output in an explain command. I think this is not far from
"This file is concerned with testing EXPLAIN in its own right.". So I
would like to keep the test in explain.sql.

> You could also shrink that 100 rows into a smaller number for
> the generate_series without losing any coverage.

Right. I will make the change.

> Aside from that, I think the patch is good.  Thanks for working on it.

Thanks. Attached is the v4 patch. I am going push it if there's no
objection.

After this, I will work on remaining node types.

Best reagards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
From e71e697f4ec399bc19af6f1aeac614185acc38c7 Mon Sep 17 00:00:00 2001
From: Tatsuo Ishii <ishii@postgresql.org>
Date: Fri, 13 Sep 2024 14:51:45 +0900
Subject: [PATCH v4] Add memory/disk usage for Window aggregate nodes in 
 EXPLAIN.

This commit is similar to 1eff8279d and expands the idea to Window
aggregate nodes so that users can know how much memory or disk the
tuplestore used.

This commit uses newly introduced tuplestore_get_stats() to query this
information and add some additional output in EXPLAIN ANALYZE to
display the information for the Window aggregate node.
---
 src/backend/commands/explain.c        | 68 ++++++++++++++++++++-------
 src/test/regress/expected/explain.out | 11 +++++
 src/test/regress/sql/explain.sql      |  3 ++
 3 files changed, 64 insertions(+), 18 deletions(-)

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 2819e479f8..aaec439892 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -120,6 +120,7 @@ static void show_sort_group_keys(PlanState *planstate, const char *qlabel,
                                  List *ancestors, ExplainState *es);
 static void show_sortorder_options(StringInfo buf, Node *sortexpr,
                                    Oid sortOperator, Oid collation, bool nullsFirst);
+static void show_storage_info(Tuplestorestate *tupstore, ExplainState *es);
 static void show_tablesample(TableSampleClause *tsc, PlanState *planstate,
                              List *ancestors, ExplainState *es);
 static void show_sort_info(SortState *sortstate, ExplainState *es);
@@ -127,6 +128,7 @@ static void show_incremental_sort_info(IncrementalSortState *incrsortstate,
                                        ExplainState *es);
 static void show_hash_info(HashState *hashstate, ExplainState *es);
 static void show_material_info(MaterialState *mstate, ExplainState *es);
+static void show_windowagg_info(WindowAggState *winstate, ExplainState *es);
 static void show_memoize_info(MemoizeState *mstate, List *ancestors,
                               ExplainState *es);
 static void show_hashagg_info(AggState *aggstate, ExplainState *es);
@@ -2231,6 +2233,7 @@ ExplainNode(PlanState *planstate, List *ancestors,
                                            planstate, es);
             show_upper_qual(((WindowAgg *) plan)->runConditionOrig,
                             "Run Condition", planstate, ancestors, es);
+            show_windowagg_info(castNode(WindowAggState, planstate), es);
             break;
         case T_Group:
             show_group_keys(castNode(GroupState, planstate), ancestors, es);
@@ -2894,6 +2897,34 @@ show_sortorder_options(StringInfo buf, Node *sortexpr,
     }
 }
 
+/*
+ * Show information on storage method and maximum memory/disk space used.
+ */
+static void
+show_storage_info(Tuplestorestate *tupstore, ExplainState *es)
+{
+    char       *maxStorageType;
+    int64        maxSpaceUsed,
+                maxSpaceUsedKB;
+
+    tuplestore_get_stats(tupstore, &maxStorageType, &maxSpaceUsed);
+    maxSpaceUsedKB = BYTES_TO_KILOBYTES(maxSpaceUsed);
+
+    if (es->format != EXPLAIN_FORMAT_TEXT)
+    {
+        ExplainPropertyText("Storage", maxStorageType, es);
+        ExplainPropertyInteger("Maximum Storage", "kB", maxSpaceUsedKB, es);
+    }
+    else
+    {
+        ExplainIndentText(es);
+        appendStringInfo(es->str,
+                         "Storage: %s  Maximum Storage: " INT64_FORMAT "kB\n",
+                         maxStorageType,
+                         maxSpaceUsedKB);
+    }
+}
+
 /*
  * Show TABLESAMPLE properties
  */
@@ -3350,9 +3381,6 @@ static void
 show_material_info(MaterialState *mstate, ExplainState *es)
 {
     Tuplestorestate *tupstore = mstate->tuplestorestate;
-    char       *maxStorageType;
-    int64        maxSpaceUsed,
-                maxSpaceUsedKB;
 
     /*
      * Nothing to show if ANALYZE option wasn't used or if execution didn't
@@ -3361,22 +3389,26 @@ show_material_info(MaterialState *mstate, ExplainState *es)
     if (!es->analyze || tupstore == NULL)
         return;
 
-    tuplestore_get_stats(tupstore, &maxStorageType, &maxSpaceUsed);
-    maxSpaceUsedKB = BYTES_TO_KILOBYTES(maxSpaceUsed);
+    show_storage_info(tupstore, es);
+}
 
-    if (es->format != EXPLAIN_FORMAT_TEXT)
-    {
-        ExplainPropertyText("Storage", maxStorageType, es);
-        ExplainPropertyInteger("Maximum Storage", "kB", maxSpaceUsedKB, es);
-    }
-    else
-    {
-        ExplainIndentText(es);
-        appendStringInfo(es->str,
-                         "Storage: %s  Maximum Storage: " INT64_FORMAT "kB\n",
-                         maxStorageType,
-                         maxSpaceUsedKB);
-    }
+/*
+ * Show information on WindowAgg node, storage method and maximum memory/disk
+ * space used.
+ */
+static void
+show_windowagg_info(WindowAggState *winstate, ExplainState *es)
+{
+    Tuplestorestate *tupstore = winstate->buffer;
+
+    /*
+     * Nothing to show if ANALYZE option wasn't used or if execution didn't
+     * get as far as creating the tuplestore.
+     */
+    if (!es->analyze || tupstore == NULL)
+        return;
+
+    show_storage_info(tupstore, es);
 }
 
 /*
diff --git a/src/test/regress/expected/explain.out b/src/test/regress/expected/explain.out
index 6585c6a69e..88355b6a99 100644
--- a/src/test/regress/expected/explain.out
+++ b/src/test/regress/expected/explain.out
@@ -691,3 +691,14 @@ select explain_filter('explain (analyze,serialize) create temp table explain_tem
  Execution Time: N.N ms
 (4 rows)
 
+-- Test tuplestore storage usage in Window aggregate
+select explain_filter('explain (analyze) select sum(n) over() from generate_series(1,10) a(n)');
+                                                 explain_filter                                                 
+----------------------------------------------------------------------------------------------------------------
+ WindowAgg  (cost=N.N..N.N rows=N width=N) (actual time=N.N..N.N rows=N loops=N)
+   Storage: Memory  Maximum Storage: NkB
+   ->  Function Scan on generate_series a  (cost=N.N..N.N rows=N width=N) (actual time=N.N..N.N rows=N loops=N)
+ Planning Time: N.N ms
+ Execution Time: N.N ms
+(5 rows)
+
diff --git a/src/test/regress/sql/explain.sql b/src/test/regress/sql/explain.sql
index c7055f850c..3f11b559aa 100644
--- a/src/test/regress/sql/explain.sql
+++ b/src/test/regress/sql/explain.sql
@@ -169,3 +169,6 @@ select explain_filter('explain (analyze,serialize text,buffers,timing off) selec
 select explain_filter('explain (analyze,serialize binary,buffers,timing) select * from int8_tbl i8');
 -- this tests an edge case where we have no data to return
 select explain_filter('explain (analyze,serialize) create temp table explain_temp as select * from int8_tbl i8');
+
+-- Test tuplestore storage usage in Window aggregate
+select explain_filter('explain (analyze) select sum(n) over() from generate_series(1,10) a(n)');
-- 
2.25.1