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