MIME-Version: 1.0
References: 
 <CAExHW5v3EwH6SGwewLwr2aDnJePgHYKooOZuOu0XfoGfjths9A@mail.gmail.com>
 <202402071013.x6psffds5qpu@alvherre.pgsql>
In-Reply-To: <202402071013.x6psffds5qpu@alvherre.pgsql>
From: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Date: Wed, 7 Feb 2024 16:09:47 +0530
Message-ID: 
 <CAExHW5v9H9z1x6RX22U-OYa1jU=Ao5j3Fstc_0AUunvoESU=fQ@mail.gmail.com>
Subject: Re: pgsql: Add EXPLAIN (MEMORY) to report planner memory consumption
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Justin Pryzby <pryzby@telsasoft.com>, pgsql-hackers@lists.postgresql.org,
	David Rowley <dgrowleyml@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAExHW5v9H9z1x6RX22U-OYa1jU%3DAo5j3Fstc_0AUunvoESU%3DfQ%40mail.gmail.com>
Precedence: bulk

On Wed, Feb 7, 2024 at 3:43=E2=80=AFPM Alvaro Herrera <alvherre@alvh.no-ip.=
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=E2=80=AFAM Justin Pryzby <pryzby@telsasoft.=
com> wrote:
> > >
> > > Up to now, the explain "  " (two space) format is not mixed with "=3D=
".
> > >
> > > And, other places which show "Memory" do not use "=3D".  David will
> > > remember prior discussions.
> > > https://www.postgresql.org/message-id/20200402054120.GC14618@telsasof=
t.com
> > > https://www.postgresql.org/message-id/20200407042521.GH2228@telsasoft=
.com
> > >
> > >                                                  "Memory: used=3D%lld=
 bytes  allocated=3D%lld bytes",
> > > vs
> > >                                                  "Buckets: %d (origin=
ally %d)  Batches: %d (originally %d)  Memory Usage: %ldkB\n",
> >
> > I have used =3D to be consistent with Buffers usage in the same Plannin=
g section.
> >
> > Are you suggesting that
> > "Memory: used=3D%lld bytes allocated=3D%lld bytes",
> > should be used instead of
> > "Memory: used=3D%lld bytes  allocated=3D%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 =3D) because ther=
e
> 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 :).

--=20
Best Wishes,
Ashutosh Bapat