Обсуждение: Align memory context level numbering in pg_log_backend_memory_contexts()

Hi,

With commit 32d3ed81, pg_backend_memory_contexts view will start 
numbering memory context levels from 1 instead of 0 in PostgreSQL 18.

For example:

   =# select name, level from pg_backend_memory_contexts;
                name           | level
   ----------------------------+-------
    TopMemoryContext           |     1
    Record information cache   |     2
    Btree proof lookup cache   |     2


However, pg_log_backend_memory_contexts() still starts its output from 
level 0:

   =# select pg_log_backend_memory_contexts(pg_backend_pid());

   LOG:  level: 0; TopMemoryContext: ...
   LOG:  level: 1; Record information cache: ...
   LOG:  level: 1; Btree proof lookup cache: ...

I understand that these view and function are intended primarily for 
one-off diagnostics and not for direct cross-comparison. So this 
discrepancy isn’t critical.
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?


-- 
Regards,

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

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

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