Subject: Re: Align memory context level numbering in pg_log_backend_memory_contexts()

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 15 Apr 2025 23:18:16 +0200

> On 15 Apr 2025, at 23:03, David Rowley <[email-masked]> 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

---

From: torikoshia <[email-masked]>
Date: Wed, 16 Apr 2025 23:19:59 +0900

> I agree with this, pg_get_process_memory_contexts() also use 1-based 

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

---

From: torikoshia <[email-masked]>
Date: Thu, 17 Apr 2025 23:27:42 +0900

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,

---

From: Fujii Masao <[email-masked]>
Date: Fri, 18 Apr 2025 18:45:39 +0900

On 2025/04/18 18:23, David Rowley wrote:
> On Fri, 18 Apr 2025 at 20:54, Fujii Masao <[email-masked]> 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