Re: pgsql: Add EXPLAIN (MEMORY) to report planner memory consumption

On Wed, Feb 7, 2024 at 3:43 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> Many thanks, Justin, for the post-commit review.
>
> On 2024-Feb-06, Ashutosh Bapat wrote:
>
> > On Tue, Feb 6, 2024 at 3:51 AM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
> > >
> > > Up to now, the explain " " (two space) format is not mixed with "=".
> > >
> > > And, other places which show "Memory" do not use "=". David will
> > > remember prior discussions.
> > > https://www.postgresql.org/message-id/20200402054120.GC14618@telsasoft.com
> > > https://www.postgresql.org/message-id/20200407042521.GH2228@telsasoft.com
> > >
> > > "Memory: used=%lld bytes allocated=%lld bytes",
> > > vs
> > > "Buckets: %d (originally %d) Batches: %d (originally %d) Memory Usage: %ldkB\n",
> >
> > I have used = to be consistent with Buffers usage in the same Planning section.
> >
> > Are you suggesting that
> > "Memory: used=%lld bytes allocated=%lld bytes",
> > should be used instead of
> > "Memory: used=%lld bytes allocated=%lld bytes",
> > Please notice the single vs double space.
>
> I think using a single space here would be confusing. It's not a
> problem for show_wal_usage because that one doesn't print units.
> I don't know it was you (Ashutosh) or I that put the double space, but I
> considered the matter and determined that two were better.
>
> In the new line we have two different separators (: and =) because there
> are two levels of info being presented; in the show_hash_info one we
> have only one type of separator.
>
> I'm not saying this is final and definite. I'm just saying I did
> consider this whole format issue and what you see is the conclusion I
> reached. It may or may not be what Ashutosh submitted -- I don't
> remember. As committer, I almost always tweak submitted patches, and I
> won't apologize for that. Further patches to correct my mistakes and
> bad decisions always welcome.

I don't have a preference myself. But now that you explain it, two
spaces between unit and next entity does seem a better choice. I had
used ",", which faced a minor objection. Thanks for that modification.
I failed to notice it in the beginning. Sorry.

>
> > > (Also, I first thought that "peek" should be "peak", but eventually I
> > > realized that's it's as intended.)
> >
> > Don't understand the context. But probably it doesn't matter.
>
> Source code always matters. Why would people spend so much time fixing
> typos otherwise?
>
> src/backend/commands/explain.c:
> static bool peek_buffer_usage(ExplainState *es, const BufferUsage *usage);
>

Ah! Thanks for sharing the context. Without that context, I didn't
understand Justin's comment. I had reviewed this change post-commit
and knew very much that its peek and not peak. I also note that it's
better than show_planning :).

--
Best Wishes,
Ashutosh Bapat