Align memory context level numbering in pg_log_backend_memory_contexts()

On Wed, 16 Apr 2025 at 01:23, torikoshia <torikoshia@oss.nttdata.com> wrote:

=# select name, level from pg_backend_memory_contexts;
name | level
----------------------------+-------
TopMemoryContext | 1

=# select pg_log_backend_memory_contexts(pg_backend_pid());

LOG: level: 0; TopMemoryContext: ...

However, for the sake of consistency and to avoid potential confusion,
would it make sense to also start the levels from 1 in
pg_log_backend_memory_contexts() starting in v18?

That's not a very nice inconsistency.

As for which one is changed... Quite a bit of thought and discussion
occurred before 32d3ed81 to try to make the "path" and "level" columns
as easy to use as possible, and making "level" 1-based was done as a
result of trying to make queries that sum up a context and its
children as easy as possible. The example at the end of [1]https://www.postgresql.org/docs/devel/view-pg-backend-memory-contexts.html would be
a little more complex with 0-based levels as we'd have to use level+1
as the index.

My vote is to make the levels 1-based in all locations where we output
the context information.

Does anyone else have views on this?

David

[1]: https://www.postgresql.org/docs/devel/view-pg-backend-memory-contexts.html

On 15 Apr 2025, at 23:03, David Rowley <dgrowleyml@gmail.com> wrote:

My vote is to make the levels 1-based in all locations where we output
the context information.

I agree with this, pg_get_process_memory_contexts() also use 1-based levels
fwiw.

--
Daniel Gustafsson

On 2025-04-16 06:18, Daniel Gustafsson wrote:

On 15 Apr 2025, at 23:03, David Rowley <dgrowleyml@gmail.com> wrote:

My vote is to make the levels 1-based in all locations where we output
the context information.

I agree with this, pg_get_process_memory_contexts() also use 1-based
levels
fwiw.

+1.
I believe there's no particular issue with starting the level from 1 in
pg_log_backend_memory_contexts().

Regarding the implementation:
In the initial patch attached, I naïvely incremented the level just
before emitting the log line.
However, it might be cleaner to simply initialize the level variable to
1 from the start. This could help avoid unnecessary confusion when
debugging that part of the code.

Similarly, I noticed that in pg_get_process_memory_contexts(), the level
is initialized to 0 in ProcessGetMemoryContextInterrupt(void):

int level = 0;
..
MemoryContextStatsInternal(c, level, 100, 100, &grand_totals, ..

If we want to be consistent, perhaps it would make sense to start from 1
there as well.

BTW level variable has existed since before pg_backend_memory_contexts
was introduced — it was originally used for functions that help inspect
memory contexts via the debugger. Because of that, I think changing this
would affect not only these functions codes but some older ones.

--
Atsushi Torikoshi
Seconded from NTT DATA GROUP CORPORATION to SRA OSS K.K.

On Thu, 17 Apr 2025 at 02:20, torikoshia <torikoshia@oss.nttdata.com> wrote:

Regarding the implementation:
In the initial patch attached, I naïvely incremented the level just
before emitting the log line.
However, it might be cleaner to simply initialize the level variable to
1 from the start. This could help avoid unnecessary confusion when
debugging that part of the code.

I didn't look at your patch before, but have now and agree that's not
the best way.

Similarly, I noticed that in pg_get_process_memory_contexts(), the level
is initialized to 0 in ProcessGetMemoryContextInterrupt(void):

int level = 0;
..
MemoryContextStatsInternal(c, level, 100, 100, &grand_totals, ..

If we want to be consistent, perhaps it would make sense to start from 1
there as well.

Yes.

BTW level variable has existed since before pg_backend_memory_contexts
was introduced — it was originally used for functions that help inspect
memory contexts via the debugger. Because of that, I think changing this
would affect not only these functions codes but some older ones.

I get the impression that it wasn't well thought through prior to
this. If you asked for max_level of 10 it would stop at 9. Changing
these to 1-based levels means we'll now stop at level 10 without
printing any more levels than we did before.

The attached patch is how I think we should do it.

David

On 2025-04-17 07:31, David Rowley wrote:

On Thu, 17 Apr 2025 at 02:20, torikoshia <torikoshia@oss.nttdata.com>
wrote:

Regarding the implementation:
In the initial patch attached, I naïvely incremented the level just
before emitting the log line.
However, it might be cleaner to simply initialize the level variable
to
1 from the start. This could help avoid unnecessary confusion when
debugging that part of the code.

I didn't look at your patch before, but have now and agree that's not
the best way.

Similarly, I noticed that in pg_get_process_memory_contexts(), the
level
is initialized to 0 in ProcessGetMemoryContextInterrupt(void):

int level = 0;
..
MemoryContextStatsInternal(c, level, 100, 100, &grand_totals, ..

If we want to be consistent, perhaps it would make sense to start from
1
there as well.

Yes.

BTW level variable has existed since before pg_backend_memory_contexts
was introduced — it was originally used for functions that help
inspect
memory contexts via the debugger. Because of that, I think changing
this
would affect not only these functions codes but some older ones.

I get the impression that it wasn't well thought through prior to
this. If you asked for max_level of 10 it would stop at 9. Changing
these to 1-based levels means we'll now stop at level 10 without
printing any more levels than we did before.

The attached patch is how I think we should do it.

Thanks for writing the patch!

I noticed that, although it's a minor detail, applying the patch changes
the indentation of the output when printing MemoryContextStats() from
the debugger. For example:

With the patch:

TopMemoryContext: 99488 total in 5 blocks; 7800 free (12 chunks);
91688 used
RowDescriptionContext: 8192 total in 1 blocks; 6912 free (0 chunks);
1280 used
MessageContext: 8192 total in 1 blocks; 6912 free (3 chunks); 1280
used
Operator class cache: 8192 total in 1 blocks; 576 free (0 chunks);
7616 used
105 more child contexts containing 1615984 total in 212 blocks; 614936
free (204 chunks); 1001048 used
Grand total: 1740048 bytes in 220 blocks; 637136 free (219 chunks);
1102912 used

original:

TopMemoryContext: 99488 total in 5 blocks; 7368 free (9 chunks); 92120
used
RowDescriptionContext: 8192 total in 1 blocks; 6912 free (0 chunks);
1280 used
MessageContext: 8192 total in 1 blocks; 6912 free (3 chunks); 1280
used
Operator class cache: 8192 total in 1 blocks; 576 free (0 chunks);
7616 used
105 more child contexts containing 1092136 total in 211 blocks; 303016
free (226 chunks); 789120 used
Grand total: 1216200 bytes in 219 blocks; 324784 free (238 chunks);
891416 use

I guess few people would notice this difference, but I think it's better
to avoid changing it unless there's a good reason to do so.
Personally, I also feel the original formatting better -- especially
because the "xx more child contexts..." line is aligned with the other
child contexts at the same level.

Attached a v2 patch to restore the original indentation.
What do you think?

--
Regards,

--
Atsushi Torikoshi
Seconded from NTT DATA GROUP CORPORATION to SRA OSS K.K.

Hi,

torikoshia <torikoshia@oss.nttdata.com>, 17 Nis 2025 Per, 12:35 tarihinde
şunu yazdı:

I guess few people would notice this difference, but I think it's better
to avoid changing it unless there's a good reason to do so.
Personally, I also feel the original formatting better -- especially
because the "xx more child contexts..." line is aligned with the other
child contexts at the same level.

Attached a v2 patch to restore the original indentation.
What do you think?

Thanks for noticing the issue. I also agree with you all about using
1-based levels consistently in memory context outputs
including pg_log_backend_memory_contexts.

I reviewed and tested the v2 patch and LGTM.

Regards,
--
Melih

Hi,

The attached patch is how I think we should do it.

Thank you for the patch.
I tested this patch and it works fine. I agree with the changes made in it.

Regarding v2 patch,
- int level = 0;

Retaining the level variable will enhance the code readability, IMO.

Thank you,
Rahila Syed

Thanks for your review, Melih and Rahila.

On 2025-04-17 21:25, Rahila Syed wrote:

Hi,

The attached patch is how I think we should do it.

Thank you for the patch.
I tested this patch and it works fine. I agree with the changes made
in it.

Regarding v2 patch,
- int level = 0;

Retaining the level variable will enhance the code readability, IMO.

As for the level variable, this change comes from the v1 patch, and I
don't have a strong opinion about it.
However, if we decide to keep the level variable here, it might be more
consistent to also define it in MemoryContextStatsDetail().

--
Regards,

--
Atsushi Torikoshi
Seconded from NTT DATA GROUP CORPORATION to SRA OSS K.K.

On Fri, 18 Apr 2025 at 00:25, Rahila Syed <rahilasyed90@gmail.com> wrote:

Regarding v2 patch,
- int level = 0;

Retaining the level variable will enhance the code readability, IMO.

When I read that, I suspected it might have been leftover from a
refactor during the development that was forgotten about. There'd be
thousands of places in our code base that you could make the
readability argument for, including the max_level and max_children
parameters at the same call-site. But those didn't get the same
treatment.

I've now pushed the latest patch. Thanks for the reviews.

David

On 2025/04/18 6:11, David Rowley wrote:

On Fri, 18 Apr 2025 at 00:25, Rahila Syed <rahilasyed90@gmail.com> wrote:

Regarding v2 patch,
- int level = 0;

Retaining the level variable will enhance the code readability, IMO.

When I read that, I suspected it might have been leftover from a
refactor during the development that was forgotten about. There'd be
thousands of places in our code base that you could make the
readability argument for, including the max_level and max_children
parameters at the same call-site. But those didn't get the same
treatment.

I've now pushed the latest patch. Thanks for the reviews.

Shouldn't the example output of pg_log_backend_memory_contexts() in
the documentation also be updated to use 1-based numbering for consistency?
Patch attached.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Fri, 18 Apr 2025 at 20:54, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

Shouldn't the example output of pg_log_backend_memory_contexts() in
the documentation also be updated to use 1-based numbering for consistency?
Patch attached.

Yeah. I failed to notice we had an example of the output.

Want to take care of it?

David

On 2025/04/18 18:23, David Rowley wrote:

On Fri, 18 Apr 2025 at 20:54, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

Shouldn't the example output of pg_log_backend_memory_contexts() in
the documentation also be updated to use 1-based numbering for consistency?
Patch attached.

Yeah. I failed to notice we had an example of the output.

Want to take care of it?

Yeah, I will if you're okay with that!

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2025/04/18 18:45, Fujii Masao wrote:

On 2025/04/18 18:23, David Rowley wrote:

On Fri, 18 Apr 2025 at 20:54, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

Shouldn't the example output of pg_log_backend_memory_contexts() in
the documentation also be updated to use 1-based numbering for consistency?
Patch attached.

Yeah. I failed to notice we had an example of the output.

Want to take care of it?

Yeah, I will if you're okay with that!

Pushed. Thanks!

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION