Feedback-ID: ia2694551:Fastmail
Date: Tue, 12 Dec 2023 21:11:31 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Cc: jian he <jian.universality@gmail.com>, 
	Andrei Lepikhov <a.lepikhov@postgrespro.ru>, Andy Fan <zhihui.fan1213@gmail.com>, 
	David Rowley <dgrowleyml@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Report planning memory in EXPLAIN ANALYZE
Message-ID: <202312122011.glue4ur4ajb4@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAExHW5t4f0K6JrE1Yj3zzOJ3_FDb1M0eFpOVnANFUQ3e9WXwYA@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/202312122011.glue4ur4ajb4%40alvherre.pgsql>
Precedence: bulk

I would replace the text in explain.sgml with this:

+      Include information on memory consumption by the query planning phase.
+      This includes the precise amount of storage used by planner in-memory
+      structures, as well as overall total consumption of planner memory,
+      including allocation overhead.
+      This parameter defaults to </literal>FALSE</literal>.

and remove the new example you're adding; it's not really necessary.

In struct ExplainState, I'd put the new member just below summary.

If we're creating a new memory context for planner, we don't need the
"mem_counts_start" thing, and we don't need the
MemoryContextSizeDifference thing.  Just add the
MemoryContextMemConsumed() function (which ISTM should be just below
MemoryContextMemAllocated() instead of in the middle of the
MemoryContextStats implementation), and
just report the values we get there.  I think the SizeDifference stuff
increases surface damage for no good reason.

ExplainOnePlan() is given a MemoryUsage object (or, if we do as above
and no longer have a MemoryUsage struct at all in the first place, a
MemoryContextCounters object) even when the MEMORY option is false.
This means we waste computing memory usage when not needed.  Let's avoid
that useless overhead.

I'd also do away with the comment you added in explain_ExecutorEnd() and
do just this, below setting of es->summary:

+           /* No support for MEMORY option */
+           /* es->memory = false; */

We don't need to elaborate more at present.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Doing what he did amounts to sticking his fingers under the hood of the
implementation; if he gets his fingers burnt, it's his problem."  (Tom Lane)