Re: Report planning memory in EXPLAIN ANALYZE

On Wed, Dec 13, 2023 at 1:41 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> 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>.

To me consumption in the "... total consumption ..." part is same as
allocation and that includes allocation overhead as well as any memory
that was allocated but remained unused (see discussion [1] if you
haven't already) ultimately. Mentioning "total consumption" and
"allocation overhead" seems more confusing.

How about
+      Include information on memory consumption by the query planning phase.
+      Report the precise amount of storage used by planner in-memory
+      structures, and total memory considering allocation overhead.
+      The parameter defaults to <literal>FALSE</literal>.

Takes care of your complaint about multiple include/ing as well.

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

Done.

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

Done

>
> 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.

240 bytes are used right after creating a memory context i.e.
mem_counts_start.totalspace = 8192 and mem_counts_start.freespace =
7952. To account for that I used two counters and calculated the
difference. If no planning is involved in EXECUTE <prepared statement>
it will show 240 bytes used instead of 0. Barring that for all
practical purposes 240 bytes negligible. E.g. 240 bytes is 4% of the
amount of memory used for planning a simple query " select 'x' ". But
your suggestion simplifies the code a lot. An error of 240 bytes seems
worth the code simplification. So did that way.

>
> 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.

Done.

Also avoided creating a memory context and switching to it when
es->memory = false.

>
> 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; */

Done.

I ended up rewriting most of the code, so squashed everything into a
single patch as attached.

--
Best Wishes,
Ashutosh Bapat