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

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

From
Justin Pryzby
Date:
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",
 

There was some discussion about "bytes" - maybe it should instead show
kB?

(Also, I first thought that "peek" should be "peak", but eventually I
realized that's it's as intended.)

On Fri, Jan 12, 2024 at 10:53:08PM +0530, Abhijit Menon-Sen wrote:
> (Those "bytes" look slightly odd to me in the midst of all the x=y
> pieces, but that's probably not worth thinking about.)


On Mon, Jan 29, 2024 at 04:55:27PM +0000, Alvaro Herrera wrote:
> Add EXPLAIN (MEMORY) to report planner memory consumption
> 
> This adds a new "Memory:" line under the "Planning:" group (which
> currently only has "Buffers:") when the MEMORY option is specified.
> 
> In order to make the reporting reasonably accurate, we create a separate
> memory context for planner activities, to be used only when this option
> is given.  The total amount of memory allocated by that context is
> reported as "allocated"; we subtract memory in the context's freelists
> from that and report that result as "used".  We use
> MemoryContextStatsInternal() to obtain the quantities.
> 
> The code structure to show buffer usage during planning was not in
> amazing shape, so I (Álvaro) modified the patch a bit to clean that up
> in passing.
> 
> Author: Ashutosh Bapat
> Reviewed-by: David Rowley, Andrey Lepikhov, Jian He, Andy Fan
> Discussion: https://www.postgresql.org/message-id/CAExHW5sZA=5LJ_ZPpRO-w09ck8z9p7eaYAqq3Ks9GDfhrxeWBw@mail.gmail.com



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

From
Ashutosh Bapat
Date:
On Tue, Feb 6, 2024 at 3:51 AM Justin Pryzby <pryzby@telsasoft.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 am fine with this.

> There was some discussion about "bytes" - maybe it should instead show
> kB?
>

So EXPLAIN (memory) on a prepared statement may report memory less
than 1kB in which case bytes is a better unit. Planner may consume as
less as few kBs of memory, reporting which in kBs would be lossy.

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

--
Best Wishes,
Ashutosh Bapat



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

From
Alvaro Herrera
Date:
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@telsasoft.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.

> > (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);

We don't want to know what the "peak" buffer usage is, but we want to
"peek" whether buffer usage would print anything.  I did have to spent a
minute thinking what the correct spelling was, here (but my English is
almost exclusively read/written, not spoken.  Condolencies if you've had
to suffer my spoken English at some conference or whatever).  I didn't
look at the dictionary back then, but now I do:
https://www.merriam-webster.com/dictionary/peek
As Justin says, it's the right word.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"After a quick R of TFM, all I can say is HOLY CR** THAT IS COOL! PostgreSQL was
amazing when I first started using it at 7.2, and I'm continually astounded by
learning new features and techniques made available by the continuing work of
the development team."
Berend Tober, http://archives.postgresql.org/pgsql-hackers/2007-08/msg01009.php



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

From
Ashutosh Bapat
Date:
On Wed, Feb 7, 2024 at 3:43 PM 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 AM Justin Pryzby <pryzby@telsasoft.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)
MemoryUsage: %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