Add memory context type to pg_backend_memory_contexts view

Started by David Rowleyover 1 year ago7 messages

dgrowleyml@gmail.com

over 1 year ago

1 attachment(s)

In [1]/messages/by-id/20240415225749.xg7uq2hwuq2qmwpg@awork3.anarazel.de Andres mentioned that there's no way to determine the memory
context type in pg_backend_memory_contexts. This is a bit annoying as
I'd like to add a test to exercise BumpStats().

Having the context type in the test's expected output helps ensure we
are exercising BumpStats() and any future changes to the choice of
context type in tuplesort.c gets flagged up by the test breaking.

It's probably too late for PG17, but I'll leave this here for the July CF.

David

[1]: /messages/by-id/20240415225749.xg7uq2hwuq2qmwpg@awork3.anarazel.de

Attachments:

v1-0001-Add-context-type-field-to-pg_backend_memory_conte.patchtext/plain; charset=US-ASCII; name=v1-0001-Add-context-type-field-to-pg_backend_memory_conte.patchDownload

From 0a58697a4b88bc3ac80f09ed78b56ebe903a2aae Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Tue, 16 Apr 2024 13:05:34 +1200
Subject: [PATCH v1] Add context type field to pg_backend_memory_contexts

---
 doc/src/sgml/system-views.sgml         |  9 +++++
 src/backend/utils/adt/mcxtfuncs.c      | 50 ++++++++++++++++++--------
 src/include/catalog/pg_proc.dat        |  6 ++--
 src/test/regress/expected/rules.out    |  5 +--
 src/test/regress/expected/sysviews.out |  8 ++---
 src/test/regress/sql/sysviews.sql      |  2 +-
 6 files changed, 56 insertions(+), 24 deletions(-)

diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index 7ed617170f..18ae5b9138 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -463,6 +463,15 @@
     </thead>
 
     <tbody>
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>type</structfield> <type>text</type>
+      </para>
+      <para>
+       Type of the memory context
+      </para></entry>
+     </row>
+
      <row>
       <entry role="catalog_table_entry"><para role="column_definition">
        <structfield>name</structfield> <type>text</type>
diff --git a/src/backend/utils/adt/mcxtfuncs.c b/src/backend/utils/adt/mcxtfuncs.c
index 4d4a70915b..fe52d57fd4 100644
--- a/src/backend/utils/adt/mcxtfuncs.c
+++ b/src/backend/utils/adt/mcxtfuncs.c
@@ -36,12 +36,13 @@ PutMemoryContextsStatsTupleStore(Tuplestorestate *tupstore,
 								 TupleDesc tupdesc, MemoryContext context,
 								 const char *parent, int level)
 {
-#define PG_GET_BACKEND_MEMORY_CONTEXTS_COLS	9
+#define PG_GET_BACKEND_MEMORY_CONTEXTS_COLS	10
 
 	Datum		values[PG_GET_BACKEND_MEMORY_CONTEXTS_COLS];
 	bool		nulls[PG_GET_BACKEND_MEMORY_CONTEXTS_COLS];
 	MemoryContextCounters stat;
 	MemoryContext child;
+	const char *type;
 	const char *name;
 	const char *ident;
 
@@ -67,10 +68,31 @@ PutMemoryContextsStatsTupleStore(Tuplestorestate *tupstore,
 	memset(values, 0, sizeof(values));
 	memset(nulls, 0, sizeof(nulls));
 
+	switch (context->type)
+	{
+		case T_AllocSetContext:
+			type = "AllocSet";
+			break;
+		case T_GenerationContext:
+			type = "Generation";
+			break;
+		case T_SlabContext:
+			type = "Slab";
+			break;
+		case T_BumpContext:
+			type = "Bump";
+			break;
+		default:
+			type = "???";
+			break;
+	}
+
+	values[0] = CStringGetTextDatum(type);
+
 	if (name)
-		values[0] = CStringGetTextDatum(name);
+		values[1] = CStringGetTextDatum(name);
 	else
-		nulls[0] = true;
+		nulls[1] = true;
 
 	if (ident)
 	{
@@ -86,22 +108,22 @@ PutMemoryContextsStatsTupleStore(Tuplestorestate *tupstore,
 
 		memcpy(clipped_ident, ident, idlen);
 		clipped_ident[idlen] = '\0';
-		values[1] = CStringGetTextDatum(clipped_ident);
+		values[2] = CStringGetTextDatum(clipped_ident);
 	}
 	else
-		nulls[1] = true;
+		nulls[2] = true;
 
 	if (parent)
-		values[2] = CStringGetTextDatum(parent);
+		values[3] = CStringGetTextDatum(parent);
 	else
-		nulls[2] = true;
-
-	values[3] = Int32GetDatum(level);
-	values[4] = Int64GetDatum(stat.totalspace);
-	values[5] = Int64GetDatum(stat.nblocks);
-	values[6] = Int64GetDatum(stat.freespace);
-	values[7] = Int64GetDatum(stat.freechunks);
-	values[8] = Int64GetDatum(stat.totalspace - stat.freespace);
+		nulls[3] = true;
+
+	values[4] = Int32GetDatum(level);
+	values[5] = Int64GetDatum(stat.totalspace);
+	values[6] = Int64GetDatum(stat.nblocks);
+	values[7] = Int64GetDatum(stat.freespace);
+	values[8] = Int64GetDatum(stat.freechunks);
+	values[9] = Int64GetDatum(stat.totalspace - stat.freespace);
 	tuplestore_putvalues(tupstore, tupdesc, values, nulls);
 
 	for (child = context->firstchild; child != NULL; child = child->nextchild)
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 134e3b22fd..adda675887 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -8277,9 +8277,9 @@
   proname => 'pg_get_backend_memory_contexts', prorows => '100',
   proretset => 't', provolatile => 'v', proparallel => 'r',
   prorettype => 'record', proargtypes => '',
-  proallargtypes => '{text,text,text,int4,int8,int8,int8,int8,int8}',
-  proargmodes => '{o,o,o,o,o,o,o,o,o}',
-  proargnames => '{name, ident, parent, level, total_bytes, total_nblocks, free_bytes, free_chunks, used_bytes}',
+  proallargtypes => '{text,text,text,text,int4,int8,int8,int8,int8,int8}',
+  proargmodes => '{o,o,o,o,o,o,o,o,o,o}',
+  proargnames => '{type, name, ident, parent, level, total_bytes, total_nblocks, free_bytes, free_chunks, used_bytes}',
   prosrc => 'pg_get_backend_memory_contexts' },
 
 # logging memory contexts of the specified backend
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index f4a0f36377..a8ac58201d 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1303,7 +1303,8 @@ pg_available_extensions| SELECT e.name,
     e.comment
    FROM (pg_available_extensions() e(name, default_version, comment)
      LEFT JOIN pg_extension x ON ((e.name = x.extname)));
-pg_backend_memory_contexts| SELECT name,
+pg_backend_memory_contexts| SELECT type,
+    name,
     ident,
     parent,
     level,
@@ -1312,7 +1313,7 @@ pg_backend_memory_contexts| SELECT name,
     free_bytes,
     free_chunks,
     used_bytes
-   FROM pg_get_backend_memory_contexts() pg_get_backend_memory_contexts(name, ident, parent, level, total_bytes, total_nblocks, free_bytes, free_chunks, used_bytes);
+   FROM pg_get_backend_memory_contexts() pg_get_backend_memory_contexts(type, name, ident, parent, level, total_bytes, total_nblocks, free_bytes, free_chunks, used_bytes);
 pg_config| SELECT name,
     setting
    FROM pg_config() pg_config(name, setting);
diff --git a/src/test/regress/expected/sysviews.out b/src/test/regress/expected/sysviews.out
index 9be7aca2b8..85585facdc 100644
--- a/src/test/regress/expected/sysviews.out
+++ b/src/test/regress/expected/sysviews.out
@@ -21,11 +21,11 @@ select count(*) >= 0 as ok from pg_available_extensions;
 
 -- The entire output of pg_backend_memory_contexts is not stable,
 -- we test only the existence and basic condition of TopMemoryContext.
-select name, ident, parent, level, total_bytes >= free_bytes
+select type, name, ident, parent, level, total_bytes >= free_bytes
   from pg_backend_memory_contexts where level = 0;
-       name       | ident | parent | level | ?column? 
-------------------+-------+--------+-------+----------
- TopMemoryContext |       |        |     0 | t
+   type   |       name       | ident | parent | level | ?column? 
+----------+------------------+-------+--------+-------+----------
+ AllocSet | TopMemoryContext |       |        |     0 | t
 (1 row)
 
 -- At introduction, pg_config had 23 entries; it may grow
diff --git a/src/test/regress/sql/sysviews.sql b/src/test/regress/sql/sysviews.sql
index 6b4e24601d..eb4bbf01b2 100644
--- a/src/test/regress/sql/sysviews.sql
+++ b/src/test/regress/sql/sysviews.sql
@@ -14,7 +14,7 @@ select count(*) >= 0 as ok from pg_available_extensions;
 
 -- The entire output of pg_backend_memory_contexts is not stable,
 -- we test only the existence and basic condition of TopMemoryContext.
-select name, ident, parent, level, total_bytes >= free_bytes
+select type, name, ident, parent, level, total_bytes >= free_bytes
   from pg_backend_memory_contexts where level = 0;
 
 -- At introduction, pg_config had 23 entries; it may grow
-- 
2.40.1.windows.1

David Rowley

dgrowleyml@gmail.com

over 1 year ago

In reply to: David Rowley (#1)

1 attachment(s)

Re: Add memory context type to pg_backend_memory_contexts view

On Tue, 16 Apr 2024 at 13:30, David Rowley <dgrowleyml@gmail.com> wrote:

In [1] Andres mentioned that there's no way to determine the memory
context type in pg_backend_memory_contexts. This is a bit annoying as
I'd like to add a test to exercise BumpStats().

Having the context type in the test's expected output helps ensure we
are exercising BumpStats() and any future changes to the choice of
context type in tuplesort.c gets flagged up by the test breaking.

bea97cd02 added a new regression test in sysviews.sql to call
pg_backend_memory_contexts to test the BumpStats() function.

The attached updates the v1 patch to add the new type column to the
new call to pg_backend_memory_contexts() to ensure the type = "Bump"

No other changes.

David

Attachments:

v2-0001-Add-context-type-field-to-pg_backend_memory_conte.patchtext/plain; charset=US-ASCII; name=v2-0001-Add-context-type-field-to-pg_backend_memory_conte.patchDownload

From 632be6de363e8f9975722debbd620665f3a0ea71 Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Tue, 16 Apr 2024 13:05:34 +1200
Subject: [PATCH v2] Add context type field to pg_backend_memory_contexts

---
 doc/src/sgml/system-views.sgml         |  9 +++++
 src/backend/utils/adt/mcxtfuncs.c      | 50 ++++++++++++++++++--------
 src/include/catalog/pg_proc.dat        |  6 ++--
 src/test/regress/expected/rules.out    |  5 +--
 src/test/regress/expected/sysviews.out | 16 ++++-----
 src/test/regress/sql/sysviews.sql      |  4 +--
 6 files changed, 61 insertions(+), 29 deletions(-)

diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index 7ed617170f..18ae5b9138 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -463,6 +463,15 @@
     </thead>
 
     <tbody>
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>type</structfield> <type>text</type>
+      </para>
+      <para>
+       Type of the memory context
+      </para></entry>
+     </row>
+
      <row>
       <entry role="catalog_table_entry"><para role="column_definition">
        <structfield>name</structfield> <type>text</type>
diff --git a/src/backend/utils/adt/mcxtfuncs.c b/src/backend/utils/adt/mcxtfuncs.c
index 4d4a70915b..fe52d57fd4 100644
--- a/src/backend/utils/adt/mcxtfuncs.c
+++ b/src/backend/utils/adt/mcxtfuncs.c
@@ -36,12 +36,13 @@ PutMemoryContextsStatsTupleStore(Tuplestorestate *tupstore,
 								 TupleDesc tupdesc, MemoryContext context,
 								 const char *parent, int level)
 {
-#define PG_GET_BACKEND_MEMORY_CONTEXTS_COLS	9
+#define PG_GET_BACKEND_MEMORY_CONTEXTS_COLS	10
 
 	Datum		values[PG_GET_BACKEND_MEMORY_CONTEXTS_COLS];
 	bool		nulls[PG_GET_BACKEND_MEMORY_CONTEXTS_COLS];
 	MemoryContextCounters stat;
 	MemoryContext child;
+	const char *type;
 	const char *name;
 	const char *ident;
 
@@ -67,10 +68,31 @@ PutMemoryContextsStatsTupleStore(Tuplestorestate *tupstore,
 	memset(values, 0, sizeof(values));
 	memset(nulls, 0, sizeof(nulls));
 
+	switch (context->type)
+	{
+		case T_AllocSetContext:
+			type = "AllocSet";
+			break;
+		case T_GenerationContext:
+			type = "Generation";
+			break;
+		case T_SlabContext:
+			type = "Slab";
+			break;
+		case T_BumpContext:
+			type = "Bump";
+			break;
+		default:
+			type = "???";
+			break;
+	}
+
+	values[0] = CStringGetTextDatum(type);
+
 	if (name)
-		values[0] = CStringGetTextDatum(name);
+		values[1] = CStringGetTextDatum(name);
 	else
-		nulls[0] = true;
+		nulls[1] = true;
 
 	if (ident)
 	{
@@ -86,22 +108,22 @@ PutMemoryContextsStatsTupleStore(Tuplestorestate *tupstore,
 
 		memcpy(clipped_ident, ident, idlen);
 		clipped_ident[idlen] = '\0';
-		values[1] = CStringGetTextDatum(clipped_ident);
+		values[2] = CStringGetTextDatum(clipped_ident);
 	}
 	else
-		nulls[1] = true;
+		nulls[2] = true;
 
 	if (parent)
-		values[2] = CStringGetTextDatum(parent);
+		values[3] = CStringGetTextDatum(parent);
 	else
-		nulls[2] = true;
-
-	values[3] = Int32GetDatum(level);
-	values[4] = Int64GetDatum(stat.totalspace);
-	values[5] = Int64GetDatum(stat.nblocks);
-	values[6] = Int64GetDatum(stat.freespace);
-	values[7] = Int64GetDatum(stat.freechunks);
-	values[8] = Int64GetDatum(stat.totalspace - stat.freespace);
+		nulls[3] = true;
+
+	values[4] = Int32GetDatum(level);
+	values[5] = Int64GetDatum(stat.totalspace);
+	values[6] = Int64GetDatum(stat.nblocks);
+	values[7] = Int64GetDatum(stat.freespace);
+	values[8] = Int64GetDatum(stat.freechunks);
+	values[9] = Int64GetDatum(stat.totalspace - stat.freespace);
 	tuplestore_putvalues(tupstore, tupdesc, values, nulls);
 
 	for (child = context->firstchild; child != NULL; child = child->nextchild)
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 134e3b22fd..adda675887 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -8277,9 +8277,9 @@
   proname => 'pg_get_backend_memory_contexts', prorows => '100',
   proretset => 't', provolatile => 'v', proparallel => 'r',
   prorettype => 'record', proargtypes => '',
-  proallargtypes => '{text,text,text,int4,int8,int8,int8,int8,int8}',
-  proargmodes => '{o,o,o,o,o,o,o,o,o}',
-  proargnames => '{name, ident, parent, level, total_bytes, total_nblocks, free_bytes, free_chunks, used_bytes}',
+  proallargtypes => '{text,text,text,text,int4,int8,int8,int8,int8,int8}',
+  proargmodes => '{o,o,o,o,o,o,o,o,o,o}',
+  proargnames => '{type, name, ident, parent, level, total_bytes, total_nblocks, free_bytes, free_chunks, used_bytes}',
   prosrc => 'pg_get_backend_memory_contexts' },
 
 # logging memory contexts of the specified backend
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index f4a0f36377..a8ac58201d 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1303,7 +1303,8 @@ pg_available_extensions| SELECT e.name,
     e.comment
    FROM (pg_available_extensions() e(name, default_version, comment)
      LEFT JOIN pg_extension x ON ((e.name = x.extname)));
-pg_backend_memory_contexts| SELECT name,
+pg_backend_memory_contexts| SELECT type,
+    name,
     ident,
     parent,
     level,
@@ -1312,7 +1313,7 @@ pg_backend_memory_contexts| SELECT name,
     free_bytes,
     free_chunks,
     used_bytes
-   FROM pg_get_backend_memory_contexts() pg_get_backend_memory_contexts(name, ident, parent, level, total_bytes, total_nblocks, free_bytes, free_chunks, used_bytes);
+   FROM pg_get_backend_memory_contexts() pg_get_backend_memory_contexts(type, name, ident, parent, level, total_bytes, total_nblocks, free_bytes, free_chunks, used_bytes);
 pg_config| SELECT name,
     setting
    FROM pg_config() pg_config(name, setting);
diff --git a/src/test/regress/expected/sysviews.out b/src/test/regress/expected/sysviews.out
index 2f3eb4e7f1..60ff45081a 100644
--- a/src/test/regress/expected/sysviews.out
+++ b/src/test/regress/expected/sysviews.out
@@ -21,11 +21,11 @@ select count(*) >= 0 as ok from pg_available_extensions;
 
 -- The entire output of pg_backend_memory_contexts is not stable,
 -- we test only the existence and basic condition of TopMemoryContext.
-select name, ident, parent, level, total_bytes >= free_bytes
+select type, name, ident, parent, level, total_bytes >= free_bytes
   from pg_backend_memory_contexts where level = 0;
-       name       | ident | parent | level | ?column? 
-------------------+-------+--------+-------+----------
- TopMemoryContext |       |        |     0 | t
+   type   |       name       | ident | parent | level | ?column? 
+----------+------------------+-------+--------+-------+----------
+ AllocSet | TopMemoryContext |       |        |     0 | t
 (1 row)
 
 -- We can exercise some MemoryContext type stats functions.  Most of the
@@ -43,11 +43,11 @@ fetch 1 from cur;
  bbbbbbbbbb | 2
 (1 row)
 
-select name, parent, total_bytes > 0, total_nblocks, free_bytes > 0, free_chunks
+select type, name, parent, total_bytes > 0, total_nblocks, free_bytes > 0, free_chunks
 from pg_backend_memory_contexts where name = 'Caller tuples';
-     name      |     parent     | ?column? | total_nblocks | ?column? | free_chunks 
----------------+----------------+----------+---------------+----------+-------------
- Caller tuples | TupleSort sort | t        |             2 | t        |           0
+ type |     name      |     parent     | ?column? | total_nblocks | ?column? | free_chunks 
+------+---------------+----------------+----------+---------------+----------+-------------
+ Bump | Caller tuples | TupleSort sort | t        |             2 | t        |           0
 (1 row)
 
 rollback;
diff --git a/src/test/regress/sql/sysviews.sql b/src/test/regress/sql/sysviews.sql
index c4f59ddc89..b899de174f 100644
--- a/src/test/regress/sql/sysviews.sql
+++ b/src/test/regress/sql/sysviews.sql
@@ -14,7 +14,7 @@ select count(*) >= 0 as ok from pg_available_extensions;
 
 -- The entire output of pg_backend_memory_contexts is not stable,
 -- we test only the existence and basic condition of TopMemoryContext.
-select name, ident, parent, level, total_bytes >= free_bytes
+select type, name, ident, parent, level, total_bytes >= free_bytes
   from pg_backend_memory_contexts where level = 0;
 
 -- We can exercise some MemoryContext type stats functions.  Most of the
@@ -28,7 +28,7 @@ declare cur cursor for select left(a,10), b
   from (values(repeat('a', 512 * 1024),1),(repeat('b', 512),2)) v(a,b)
   order by v.a desc;
 fetch 1 from cur;
-select name, parent, total_bytes > 0, total_nblocks, free_bytes > 0, free_chunks
+select type, name, parent, total_bytes > 0, total_nblocks, free_bytes > 0, free_chunks
 from pg_backend_memory_contexts where name = 'Caller tuples';
 rollback;
 
-- 
2.40.1.windows.1

David Christensen

david+pg@pgguru.net

over 1 year ago

In reply to: David Rowley (#2)

Re: Add memory context type to pg_backend_memory_contexts view

Hi David,

Giving this a once-through, this seems straightforward and useful. I
have a slight preference for keeping "name" the first field in the
view and moving "type" to the second, but that's minor.

Just confirming that the allocator types are not extensible without a
recompile, since it's using a specific node tag to switch on, so there
are no concerns with not properly displaying the output of something
else.

The "????" text placeholder might be more appropriate as "<unknown>",
or perhaps stronger, include a WARNING in the logs, since an unknown
tag at this point would be an indication of some sort of memory
corruption.

Since there are only four possible values, I think there would be
utility in including them in the docs for this field. I also think it
would be useful to have some sort of comments at least in mmgr/README
to indicate that if a new type of allocator is introduced that you
will also need to add the node to the function for this type, since
it's not an automatic conversion. (For that matter, instead of
switching on node type and outputting a given string, is there a
generic function that could just give us the string value for node
type so we don't need to teach anything else about it anyway?)

Thanks,

David

David Rowley

dgrowleyml@gmail.com

over 1 year ago

In reply to: David Christensen (#3)

1 attachment(s)

Re: Add memory context type to pg_backend_memory_contexts view

On Fri, 31 May 2024 at 07:21, David Christensen <david+pg@pgguru.net> wrote:

Giving this a once-through, this seems straightforward and useful. I
have a slight preference for keeping "name" the first field in the
view and moving "type" to the second, but that's minor.

Not having it first make sense, but I don't think putting it between
name and ident is a good idea. I think name and ident belong next to
each other. parent likely should come after those since that also
relates to the name.

How about putting it after "parent"?

Just confirming that the allocator types are not extensible without a
recompile, since it's using a specific node tag to switch on, so there
are no concerns with not properly displaying the output of something
else.

They're not extensible.

The "????" text placeholder might be more appropriate as "<unknown>",
or perhaps stronger, include a WARNING in the logs, since an unknown
tag at this point would be an indication of some sort of memory
corruption.

This follows what we do in other places. If you look at explain.c,
you'll see lots of "???"s.

I think if you're worried about corrupted memory, then garbled output
in pg_get_backend_memory_contexts wouldn't be that high on the list of
concerns.

Since there are only four possible values, I think there would be
utility in including them in the docs for this field.

I'm not sure about this. We do try not to expose too much internal
detail in the docs. I don't know all the reasons for that, but at
least one reason is that it's easy for things to get outdated as code
evolves. I'm also unsure how much value there is in listing 4 possible
values unless we were to document the meaning of each of those values,
and doing that puts us even further down the path of detailing
Postgres internals in the documents. I don't think that's a
maintenance burden that's often worth the trouble.

I also think it
would be useful to have some sort of comments at least in mmgr/README
to indicate that if a new type of allocator is introduced that you
will also need to add the node to the function for this type, since
it's not an automatic conversion.

I don't think it's sustainable to do this. If we have to maintain
documentation that lists everything you must do in order to add some
new node types then I feel it's just going to get outdated as soon as
someone adds something new that needs to be done. I'm only one
developer, but for me, I'd not even bother looking there if I was
planning to add a new memory context type.

What I would be doing is searching the entire code base for where
special handling is done for the existing types and ensuring I
consider if I need to include a case for the new node type. In this
case, I'd probably choose to search for "T_AllocSetContext", and I'd
quickly land on PutMemoryContextsStatsTupleStore() and update it. This
method doesn't get outdated, provided you do "git pull" occasionally.

(For that matter, instead of
switching on node type and outputting a given string, is there a
generic function that could just give us the string value for node
type so we don't need to teach anything else about it anyway?)

There isn't. nodeToString() does take some node types as inputs and
serialise those to something JSON-like, but that includes serialising
each field of the node type too. The memory context types are not
handled by those functions. I think it's fine to copy what's done in
explain.c. "git grep \"???\" -- *.c | wc -l" gives me 31 occurrences,
so I'm not doing anything new.

I've attached an updated patch which changes the position of the new
column in the view.

Thank you for the review.

David

Attachments:

v3-0001-Add-context-type-field-to-pg_backend_memory_conte.patchapplication/octet-stream; name=v3-0001-Add-context-type-field-to-pg_backend_memory_conte.patchDownload

From fd01edc7fa34cb43b631e35324104cf629422ac1 Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Tue, 16 Apr 2024 13:05:34 +1200
Subject: [PATCH v3] Add context type field to pg_backend_memory_contexts

---
 doc/src/sgml/system-views.sgml         |  9 +++++++
 src/backend/utils/adt/mcxtfuncs.c      | 35 ++++++++++++++++++++------
 src/include/catalog/pg_proc.dat        |  6 ++---
 src/test/regress/expected/rules.out    |  3 ++-
 src/test/regress/expected/sysviews.out | 16 ++++++------
 src/test/regress/sql/sysviews.sql      |  4 +--
 6 files changed, 52 insertions(+), 21 deletions(-)

diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index 8c18bea902..bdc34cf94e 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -490,6 +490,15 @@
       </para></entry>
      </row>
 
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>type</structfield> <type>text</type>
+      </para>
+      <para>
+       Type of the memory context
+      </para></entry>
+     </row>
+
      <row>
       <entry role="catalog_table_entry"><para role="column_definition">
        <structfield>level</structfield> <type>int4</type>
diff --git a/src/backend/utils/adt/mcxtfuncs.c b/src/backend/utils/adt/mcxtfuncs.c
index 4d4a70915b..1085941484 100644
--- a/src/backend/utils/adt/mcxtfuncs.c
+++ b/src/backend/utils/adt/mcxtfuncs.c
@@ -36,7 +36,7 @@ PutMemoryContextsStatsTupleStore(Tuplestorestate *tupstore,
 								 TupleDesc tupdesc, MemoryContext context,
 								 const char *parent, int level)
 {
-#define PG_GET_BACKEND_MEMORY_CONTEXTS_COLS	9
+#define PG_GET_BACKEND_MEMORY_CONTEXTS_COLS	10
 
 	Datum		values[PG_GET_BACKEND_MEMORY_CONTEXTS_COLS];
 	bool		nulls[PG_GET_BACKEND_MEMORY_CONTEXTS_COLS];
@@ -44,6 +44,7 @@ PutMemoryContextsStatsTupleStore(Tuplestorestate *tupstore,
 	MemoryContext child;
 	const char *name;
 	const char *ident;
+	const char *type;
 
 	Assert(MemoryContextIsValid(context));
 
@@ -96,12 +97,32 @@ PutMemoryContextsStatsTupleStore(Tuplestorestate *tupstore,
 	else
 		nulls[2] = true;
 
-	values[3] = Int32GetDatum(level);
-	values[4] = Int64GetDatum(stat.totalspace);
-	values[5] = Int64GetDatum(stat.nblocks);
-	values[6] = Int64GetDatum(stat.freespace);
-	values[7] = Int64GetDatum(stat.freechunks);
-	values[8] = Int64GetDatum(stat.totalspace - stat.freespace);
+	switch (context->type)
+	{
+		case T_AllocSetContext:
+			type = "AllocSet";
+			break;
+		case T_GenerationContext:
+			type = "Generation";
+			break;
+		case T_SlabContext:
+			type = "Slab";
+			break;
+		case T_BumpContext:
+			type = "Bump";
+			break;
+		default:
+			type = "???";
+			break;
+	}
+
+	values[3] = CStringGetTextDatum(type);
+	values[4] = Int32GetDatum(level);
+	values[5] = Int64GetDatum(stat.totalspace);
+	values[6] = Int64GetDatum(stat.nblocks);
+	values[7] = Int64GetDatum(stat.freespace);
+	values[8] = Int64GetDatum(stat.freechunks);
+	values[9] = Int64GetDatum(stat.totalspace - stat.freespace);
 	tuplestore_putvalues(tupstore, tupdesc, values, nulls);
 
 	for (child = context->firstchild; child != NULL; child = child->nextchild)
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 6a5476d3c4..d4ac578ae6 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -8279,9 +8279,9 @@
   proname => 'pg_get_backend_memory_contexts', prorows => '100',
   proretset => 't', provolatile => 'v', proparallel => 'r',
   prorettype => 'record', proargtypes => '',
-  proallargtypes => '{text,text,text,int4,int8,int8,int8,int8,int8}',
-  proargmodes => '{o,o,o,o,o,o,o,o,o}',
-  proargnames => '{name, ident, parent, level, total_bytes, total_nblocks, free_bytes, free_chunks, used_bytes}',
+  proallargtypes => '{text,text,text,text,int4,int8,int8,int8,int8,int8}',
+  proargmodes => '{o,o,o,o,o,o,o,o,o,o}',
+  proargnames => '{name, ident, parent, type, level, total_bytes, total_nblocks, free_bytes, free_chunks, used_bytes}',
   prosrc => 'pg_get_backend_memory_contexts' },
 
 # logging memory contexts of the specified backend
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index ef658ad740..bed1d029f5 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1306,13 +1306,14 @@ pg_available_extensions| SELECT e.name,
 pg_backend_memory_contexts| SELECT name,
     ident,
     parent,
+    type,
     level,
     total_bytes,
     total_nblocks,
     free_bytes,
     free_chunks,
     used_bytes
-   FROM pg_get_backend_memory_contexts() pg_get_backend_memory_contexts(name, ident, parent, level, total_bytes, total_nblocks, free_bytes, free_chunks, used_bytes);
+   FROM pg_get_backend_memory_contexts() pg_get_backend_memory_contexts(name, ident, parent, type, level, total_bytes, total_nblocks, free_bytes, free_chunks, used_bytes);
 pg_config| SELECT name,
     setting
    FROM pg_config() pg_config(name, setting);
diff --git a/src/test/regress/expected/sysviews.out b/src/test/regress/expected/sysviews.out
index dbfd0c13d4..c8e3d3620c 100644
--- a/src/test/regress/expected/sysviews.out
+++ b/src/test/regress/expected/sysviews.out
@@ -21,11 +21,11 @@ select count(*) >= 0 as ok from pg_available_extensions;
 
 -- The entire output of pg_backend_memory_contexts is not stable,
 -- we test only the existence and basic condition of TopMemoryContext.
-select name, ident, parent, level, total_bytes >= free_bytes
+select type, name, ident, parent, level, total_bytes >= free_bytes
   from pg_backend_memory_contexts where level = 0;
-       name       | ident | parent | level | ?column? 
-------------------+-------+--------+-------+----------
- TopMemoryContext |       |        |     0 | t
+   type   |       name       | ident | parent | level | ?column? 
+----------+------------------+-------+--------+-------+----------
+ AllocSet | TopMemoryContext |       |        |     0 | t
 (1 row)
 
 -- We can exercise some MemoryContext type stats functions.  Most of the
@@ -43,11 +43,11 @@ fetch 1 from cur;
  bbbbbbbbbb | 2
 (1 row)
 
-select name, parent, total_bytes > 0, total_nblocks, free_bytes > 0, free_chunks
+select type, name, parent, total_bytes > 0, total_nblocks, free_bytes > 0, free_chunks
 from pg_backend_memory_contexts where name = 'Caller tuples';
-     name      |     parent     | ?column? | total_nblocks | ?column? | free_chunks 
----------------+----------------+----------+---------------+----------+-------------
- Caller tuples | TupleSort sort | t        |             2 | t        |           0
+ type |     name      |     parent     | ?column? | total_nblocks | ?column? | free_chunks 
+------+---------------+----------------+----------+---------------+----------+-------------
+ Bump | Caller tuples | TupleSort sort | t        |             2 | t        |           0
 (1 row)
 
 rollback;
diff --git a/src/test/regress/sql/sysviews.sql b/src/test/regress/sql/sysviews.sql
index c4f59ddc89..b899de174f 100644
--- a/src/test/regress/sql/sysviews.sql
+++ b/src/test/regress/sql/sysviews.sql
@@ -14,7 +14,7 @@ select count(*) >= 0 as ok from pg_available_extensions;
 
 -- The entire output of pg_backend_memory_contexts is not stable,
 -- we test only the existence and basic condition of TopMemoryContext.
-select name, ident, parent, level, total_bytes >= free_bytes
+select type, name, ident, parent, level, total_bytes >= free_bytes
   from pg_backend_memory_contexts where level = 0;
 
 -- We can exercise some MemoryContext type stats functions.  Most of the
@@ -28,7 +28,7 @@ declare cur cursor for select left(a,10), b
   from (values(repeat('a', 512 * 1024),1),(repeat('b', 512),2)) v(a,b)
   order by v.a desc;
 fetch 1 from cur;
-select name, parent, total_bytes > 0, total_nblocks, free_bytes > 0, free_chunks
+select type, name, parent, total_bytes > 0, total_nblocks, free_bytes > 0, free_chunks
 from pg_backend_memory_contexts where name = 'Caller tuples';
 rollback;
 
-- 
2.34.1

David Christensen

david@pgguru.net

over 1 year ago

In reply to: David Rowley (#4)

Re: Add memory context type to pg_backend_memory_contexts view

On May 30, 2024, at 5:36 PM, David Rowley <dgrowleyml@gmail.com> wrote:

On Fri, 31 May 2024 at 07:21, David Christensen <david+pg@pgguru.net> wrote:

Giving this a once-through, this seems straightforward and useful. I
have a slight preference for keeping "name" the first field in the
view and moving "type" to the second, but that's minor.

Not having it first make sense, but I don't think putting it between
name and ident is a good idea. I think name and ident belong next to
each other. parent likely should come after those since that also
relates to the name.

How about putting it after "parent"?

That works for me. I skimmed the new patch and it seems fine but on my phone so didn’t do any build tests.

Just confirming that the allocator types are not extensible without a
recompile, since it's using a specific node tag to switch on, so there
are no concerns with not properly displaying the output of something
else.

They're not extensible.

Good to confirm.

The "????" text placeholder might be more appropriate as "<unknown>",
or perhaps stronger, include a WARNING in the logs, since an unknown
tag at this point would be an indication of some sort of memory
corruption.

This follows what we do in other places. If you look at explain.c,
you'll see lots of "???"s.

I think if you're worried about corrupted memory, then garbled output
in pg_get_backend_memory_contexts wouldn't be that high on the list of
concerns.

Heh, indeed. +1 for precedent.

Since there are only four possible values, I think there would be
utility in including them in the docs for this field.

I'm not sure about this. We do try not to expose too much internal
detail in the docs. I don't know all the reasons for that, but at
least one reason is that it's easy for things to get outdated as code
evolves. I'm also unsure how much value there is in listing 4 possible
values unless we were to document the meaning of each of those values,
and doing that puts us even further down the path of detailing
Postgres internals in the documents. I don't think that's a
maintenance burden that's often worth the trouble.

I can see that and it’s consistent with what we do, just was thinking as a user that that may be useful, but if you’re using this view you likely already know what it means.

I also think it
would be useful to have some sort of comments at least in mmgr/README
to indicate that if a new type of allocator is introduced that you
will also need to add the node to the function for this type, since
it's not an automatic conversion.

I don't think it's sustainable to do this. If we have to maintain
documentation that lists everything you must do in order to add some
new node types then I feel it's just going to get outdated as soon as
someone adds something new that needs to be done. I'm only one
developer, but for me, I'd not even bother looking there if I was
planning to add a new memory context type.

What I would be doing is searching the entire code base for where
special handling is done for the existing types and ensuring I
consider if I need to include a case for the new node type. In this
case, I'd probably choose to search for "T_AllocSetContext", and I'd
quickly land on PutMemoryContextsStatsTupleStore() and update it. This
method doesn't get outdated, provided you do "git pull" occasionally.

Fair.

(For that matter, instead of
switching on node type and outputting a given string, is there a
generic function that could just give us the string value for node
type so we don't need to teach anything else about it anyway?)

There isn't. nodeToString() does take some node types as inputs and
serialise those to something JSON-like, but that includes serialising
each field of the node type too. The memory context types are not
handled by those functions. I think it's fine to copy what's done in
explain.c. "git grep \"???\" -- *.c | wc -l" gives me 31 occurrences,
so I'm not doing anything new.

I've attached an updated patch which changes the position of the new
column in the view.

Michael Paquier

michael@paquier.xyz

over 1 year ago

In reply to: David Rowley (#4)

Re: Add memory context type to pg_backend_memory_contexts view

On Fri, May 31, 2024 at 12:35:58PM +1200, David Rowley wrote:

This follows what we do in other places. If you look at explain.c,
you'll see lots of "???"s.

I think if you're worried about corrupted memory, then garbled output
in pg_get_backend_memory_contexts wouldn't be that high on the list of
concerns.

+	const char *type;
[...]
+	switch (context->type)
+	{
+		case T_AllocSetContext:
+			type = "AllocSet";
+			break;
+		case T_GenerationContext:
+			type = "Generation";
+			break;
+		case T_SlabContext:
+			type = "Slab";
+			break;
+		case T_BumpContext:
+			type = "Bump";
+			break;
+		default:
+			type = "???";
+			break;
+	}

Yeah, it's a common practice to use that as fallback. What you are
doing is OK, and it is not possible to remove the default case as
these are nodetags to generate warnings if a new value needs to be
added.

This patch looks like a good idea, so +1 from here. (PS: catversion
bump).
--
Michael

David Rowley

dgrowleyml@gmail.com

over 1 year ago

In reply to: Michael Paquier (#6)

Re: Add memory context type to pg_backend_memory_contexts view

On Sat, 1 Jun 2024 at 12:55, Michael Paquier <michael@paquier.xyz> wrote:

This patch looks like a good idea, so +1 from here.

Thank you to both of you for reviewing this. I've now pushed the patch.

David