Hi Alvaro,
Thanks for the review and the edits. Sorry for replying late.
On Tue, Nov 21, 2023 at 11:56 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> 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.
It is not default but hidden behind "SUMMARY".Do you still think it
requires another sub-flag MEMORY?
>
> 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.
That looks better. The functions are now available outside explain.
>
> I see no reason for this to be three separate patches anymore.
Squashed into one along with your changes.
>
> The EXPLAIN docs (explain.sgml) need an update to mention the new flag
> and the new output, too.
Updated section describing "SUMMARY" flag.
PFA updated patch.
--
Best Wishes,
Ashutosh Bapat
