Re: Align memory context level numbering in pg_log_backend_memory_contexts() - Mailing list pgsql-hackers

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.