Re: Align memory context level numbering in pg_log_backend_memory_contexts()

On 2025-04-17 07:31, David Rowley wrote:
> On Thu, 17 Apr 2025 at 02:20, torikoshia <torikoshia(at)oss(dot)nttdata(dot)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.