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.
Attachments:
v1-0001-Increment-level-in-pg_log_backend_memory_contexts.patchtext/x-diff; name=v1-0001-Increment-level-in-pg_log_backend_memory_contexts.patchDownload
From 7238f29804dc4f027a6e148f5f755d369c4d349c Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikoshi@sraoss.co.jp>
Date: Tue, 15 Apr 2025 21:55:39 +0900
Subject: [PATCH v1] Increment level in pg_log_backend_memory_contexts()
Since commit 32d3ed81, pg_backend_memory_contexts reports memory context
levels starting from 1 instead of 0. For consistency, this patch makes
pg_log_backend_memory_contexts() do the same.
These outputs are primarily used for diagnostics and are not meant to be
compared directly, but aligning them may reduce confusion.
---
src/backend/utils/mmgr/mcxt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 1f5ebf2e12..54ee8af41f 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -1113,7 +1113,7 @@ MemoryContextStatsPrint(MemoryContext context, void *passthru,
(errhidestmt(true),
errhidecontext(true),
errmsg_internal("level: %d; %s: %s%s",
- level, name, stats_string, truncated_ident)));
+ level + 1, name, stats_string, truncated_ident)));
}
/*
base-commit: c55df7c6eae5a5c6f91cd029fb91913db7f2089c
--
2.43.0
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
Attachments:
make_memory_context_levels_1_based.patchapplication/octet-stream; name=make_memory_context_levels_1_based.patchDownload
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 1f5ebf2e124..6fa3e350d1b 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -873,7 +873,7 @@ MemoryContextStatsDetail(MemoryContext context,
print_location = PRINT_STATS_TO_LOGS;
/* num_contexts report number of contexts aggregated in the output */
- MemoryContextStatsInternal(context, 0, max_level, max_children,
+ MemoryContextStatsInternal(context, 1, max_level, max_children,
&grand_totals, print_location, &num_contexts);
if (print_to_stderr)
@@ -968,7 +968,7 @@ MemoryContextStatsInternal(MemoryContext context, int level,
*/
child = context->firstchild;
ichild = 0;
- if (level < max_level && !stack_is_too_deep())
+ if (level <= max_level && !stack_is_too_deep())
{
for (; child != NULL && ichild < max_children;
child = child->nextchild, ichild++)
@@ -1003,7 +1003,7 @@ MemoryContextStatsInternal(MemoryContext context, int level,
if (print_location == PRINT_STATS_TO_STDERR)
{
- for (int i = 0; i <= level; i++)
+ for (int i = 0; i < level; i++)
fprintf(stderr, " ");
fprintf(stderr,
"%d more child contexts containing %zu total in %zu blocks; %zu free (%zu chunks); %zu used\n",
@@ -1585,12 +1585,11 @@ ProcessGetMemoryContextInterrupt(void)
{
MemoryContextCounters grand_totals;
int num_contexts = 0;
- int level = 0;
path = NIL;
memset(&grand_totals, 0, sizeof(grand_totals));
- MemoryContextStatsInternal(c, level, 100, 100, &grand_totals,
+ MemoryContextStatsInternal(c, 1, 100, 100, &grand_totals,
PRINT_STATS_NONE, &num_contexts);
path = compute_context_path(c, context_id_lookup);
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.
Attachments:
make_memory_context_levels_1_based_v2.patchtext/x-diff; name=make_memory_context_levels_1_based_v2.patchDownload
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 1f5ebf2e12..e9aab36d11 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -873,7 +873,7 @@ MemoryContextStatsDetail(MemoryContext context,
print_location = PRINT_STATS_TO_LOGS;
/* num_contexts report number of contexts aggregated in the output */
- MemoryContextStatsInternal(context, 0, max_level, max_children,
+ MemoryContextStatsInternal(context, 1, max_level, max_children,
&grand_totals, print_location, &num_contexts);
if (print_to_stderr)
@@ -968,7 +968,7 @@ MemoryContextStatsInternal(MemoryContext context, int level,
*/
child = context->firstchild;
ichild = 0;
- if (level < max_level && !stack_is_too_deep())
+ if (level <= max_level && !stack_is_too_deep())
{
for (; child != NULL && ichild < max_children;
child = child->nextchild, ichild++)
@@ -1003,7 +1003,7 @@ MemoryContextStatsInternal(MemoryContext context, int level,
if (print_location == PRINT_STATS_TO_STDERR)
{
- for (int i = 0; i <= level; i++)
+ for (int i = 0; i < level; i++)
fprintf(stderr, " ");
fprintf(stderr,
"%d more child contexts containing %zu total in %zu blocks; %zu free (%zu chunks); %zu used\n",
@@ -1104,7 +1104,7 @@ MemoryContextStatsPrint(MemoryContext context, void *passthru,
if (print_to_stderr)
{
- for (i = 0; i < level; i++)
+ for (i = 1; i < level; i++)
fprintf(stderr, " ");
fprintf(stderr, "%s: %s%s\n", name, stats_string, truncated_ident);
}
@@ -1585,12 +1585,11 @@ ProcessGetMemoryContextInterrupt(void)
{
MemoryContextCounters grand_totals;
int num_contexts = 0;
- int level = 0;
path = NIL;
memset(&grand_totals, 0, sizeof(grand_totals));
- MemoryContextStatsInternal(c, level, 100, 100, &grand_totals,
+ MemoryContextStatsInternal(c, 1, 100, 100, &grand_totals,
PRINT_STATS_NONE, &num_contexts);
path = compute_context_path(c, context_id_lookup);
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
Attachments:
v1-0001-doc-Fix-memory-context-level-in-pg_log_backend_me.patchtext/plain; charset=UTF-8; name=v1-0001-doc-Fix-memory-context-level-in-pg_log_backend_me.patchDownload
From 7b3aa9de467005e540b2246ed89c9e43fe08f6bd Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Fri, 18 Apr 2025 17:45:29 +0900
Subject: [PATCH v1] doc: Fix memory context level in
pg_log_backend_memory_contexts() example.
Commit d9e03864b6b changed the memory context level numbers shown by
pg_log_backend_memory_contexts() to be 1-based. However, the example in
the documentation was not updated and still used 0-based numbering.
This commit updates the example to match the current 1-based output.
---
doc/src/sgml/func.sgml | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 1c5cfee25d1..574a544d9fa 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -28922,16 +28922,16 @@ One message for each memory context will be logged. For example:
<screen>
LOG: logging memory contexts of PID 10377
STATEMENT: SELECT pg_log_backend_memory_contexts(pg_backend_pid());
-LOG: level: 0; TopMemoryContext: 80800 total in 6 blocks; 14432 free (5 chunks); 66368 used
-LOG: level: 1; pgstat TabStatusArray lookup hash table: 8192 total in 1 blocks; 1408 free (0 chunks); 6784 used
-LOG: level: 1; TopTransactionContext: 8192 total in 1 blocks; 7720 free (1 chunks); 472 used
-LOG: level: 1; RowDescriptionContext: 8192 total in 1 blocks; 6880 free (0 chunks); 1312 used
-LOG: level: 1; MessageContext: 16384 total in 2 blocks; 5152 free (0 chunks); 11232 used
-LOG: level: 1; Operator class cache: 8192 total in 1 blocks; 512 free (0 chunks); 7680 used
-LOG: level: 1; smgr relation table: 16384 total in 2 blocks; 4544 free (3 chunks); 11840 used
-LOG: level: 1; TransactionAbortContext: 32768 total in 1 blocks; 32504 free (0 chunks); 264 used
+LOG: level: 1; TopMemoryContext: 80800 total in 6 blocks; 14432 free (5 chunks); 66368 used
+LOG: level: 2; pgstat TabStatusArray lookup hash table: 8192 total in 1 blocks; 1408 free (0 chunks); 6784 used
+LOG: level: 2; TopTransactionContext: 8192 total in 1 blocks; 7720 free (1 chunks); 472 used
+LOG: level: 2; RowDescriptionContext: 8192 total in 1 blocks; 6880 free (0 chunks); 1312 used
+LOG: level: 2; MessageContext: 16384 total in 2 blocks; 5152 free (0 chunks); 11232 used
+LOG: level: 2; Operator class cache: 8192 total in 1 blocks; 512 free (0 chunks); 7680 used
+LOG: level: 2; smgr relation table: 16384 total in 2 blocks; 4544 free (3 chunks); 11840 used
+LOG: level: 2; TransactionAbortContext: 32768 total in 1 blocks; 32504 free (0 chunks); 264 used
...
-LOG: level: 1; ErrorContext: 8192 total in 1 blocks; 7928 free (3 chunks); 264 used
+LOG: level: 2; ErrorContext: 8192 total in 1 blocks; 7928 free (3 chunks); 264 used
LOG: Grand total: 1651920 bytes in 201 blocks; 622360 free (88 chunks); 1029560 used
</screen>
If there are more than 100 child contexts under the same parent, the first
--
2.49.0
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