On 2025-04-17 07:31, David Rowley wrote:
On Thu, 17 Apr 2025 at 02:20, torikoshia <torikos...@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.
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);
