Do we still need parent column in pg_backend_memory_context?
Hi hackers,
After the patch [1]/messages/by-id/CAGPVpCThLyOsj3e_gYEvLoHkr5w=tadDiN_=z2OwsK3VJppeBA@mail.gmail.com that adds a path column to pg_backend_memory_context,
the parent context can also be found in the path array. Since there are
currently two ways to retrieve information related to the parent of a
context, I wonder whether we still want to keep the parent column.
The path column represents the path from TopMemoryContext to the current
memory context. There is always "level" number of elements in a path array
for any memory context. The first element in the array is TopMemoryContext,
and the last element (path[level]) is the current memory context. The
path[level-1] element will simply show us the parent context ID.
I understand that having the parent name instead of the transient parent
context ID can be easier to use in some cases. While I suspect that the
memory contexts most users are interested in are close to
TopMemoryContext—which means their context IDs are much less likely to
change with each execution—it's still not guaranteed.
I'm also unsure how common it is to use or rely on the parent column. I
quickly searched here [2]https://codesearch.debian.net/search?q=pg_backend_memory_context&literal=1&page=3 to see how pg_backend_memory_context is used.
There are a few places where the parent column is used in extensions. I
believe these places should be easy to update if we decide to remove the
parent column.
Attached is a patch to remove parent from the view.
[1]: /messages/by-id/CAGPVpCThLyOsj3e_gYEvLoHkr5w=tadDiN_=z2OwsK3VJppeBA@mail.gmail.com
/messages/by-id/CAGPVpCThLyOsj3e_gYEvLoHkr5w=tadDiN_=z2OwsK3VJppeBA@mail.gmail.com
[2]: https://codesearch.debian.net/search?q=pg_backend_memory_context&literal=1&page=3
https://codesearch.debian.net/search?q=pg_backend_memory_context&literal=1&page=3
Regards,
--
Melih Mutlu
Microsoft
Attachments:
v1-0001-Remove-parent-from-pg_backend_memory_context.patchapplication/octet-stream; name=v1-0001-Remove-parent-from-pg_backend_memory_context.patchDownload
From 17b3c57b53f53b504385ddde722a2ad0c88b2de5 Mon Sep 17 00:00:00 2001
From: Melih Mutlu <m.melihmutlu@gmail.com>
Date: Tue, 30 Jul 2024 19:20:34 +0300
Subject: [PATCH v1] Remove parent from pg_backend_memory_context
After adding path column into pg_backend_memory_context view wih commit
32d3ed8165, parent of a memory context can be found using the path
array.
---
src/backend/utils/adt/mcxtfuncs.c | 30 +++++++++--------------------
src/include/catalog/pg_proc.dat | 6 +++---
src/test/regress/expected/rules.out | 1 -
3 files changed, 12 insertions(+), 25 deletions(-)
diff --git a/src/backend/utils/adt/mcxtfuncs.c b/src/backend/utils/adt/mcxtfuncs.c
index 5905958c1f..34d70199fc 100644
--- a/src/backend/utils/adt/mcxtfuncs.c
+++ b/src/backend/utils/adt/mcxtfuncs.c
@@ -93,7 +93,7 @@ PutMemoryContextsStatsTupleStore(Tuplestorestate *tupstore,
TupleDesc tupdesc, MemoryContext context,
HTAB *context_id_lookup)
{
-#define PG_GET_BACKEND_MEMORY_CONTEXTS_COLS 11
+#define PG_GET_BACKEND_MEMORY_CONTEXTS_COLS 10
Datum values[PG_GET_BACKEND_MEMORY_CONTEXTS_COLS];
bool nulls[PG_GET_BACKEND_MEMORY_CONTEXTS_COLS];
@@ -154,18 +154,6 @@ PutMemoryContextsStatsTupleStore(Tuplestorestate *tupstore,
else
nulls[1] = true;
- if (context->parent)
- {
- const char *parent_name,
- *parent_ident;
-
- get_memory_context_name_and_ident(context->parent, &parent_name,
- &parent_ident);
- values[2] = CStringGetTextDatum(parent_name);
- }
- else
- nulls[2] = true;
-
switch (context->type)
{
case T_AllocSetContext:
@@ -185,14 +173,14 @@ PutMemoryContextsStatsTupleStore(Tuplestorestate *tupstore,
break;
}
- values[3] = CStringGetTextDatum(type);
- values[4] = Int32GetDatum(list_length(path)); /* level */
- values[5] = int_list_to_array(path);
- values[6] = Int64GetDatum(stat.totalspace);
- values[7] = Int64GetDatum(stat.nblocks);
- values[8] = Int64GetDatum(stat.freespace);
- values[9] = Int64GetDatum(stat.freechunks);
- values[10] = Int64GetDatum(stat.totalspace - stat.freespace);
+ values[2] = CStringGetTextDatum(type);
+ values[3] = Int32GetDatum(list_length(path)); /* level */
+ values[4] = int_list_to_array(path);
+ 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);
list_free(path);
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 06b2f4ba66..ee780f81fb 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -8317,9 +8317,9 @@
proname => 'pg_get_backend_memory_contexts', prorows => '100',
proretset => 't', provolatile => 'v', proparallel => 'r',
prorettype => 'record', proargtypes => '',
- proallargtypes => '{text,text,text,text,int4,_int4,int8,int8,int8,int8,int8}',
- proargmodes => '{o,o,o,o,o,o,o,o,o,o,o}',
- proargnames => '{name, ident, parent, type, level, path, total_bytes, total_nblocks, free_bytes, free_chunks, used_bytes}',
+ proallargtypes => '{text,text,text,int4,_int4,int8,int8,int8,int8,int8}',
+ proargmodes => '{o,o,o,o,o,o,o,o,o,o}',
+ proargnames => '{name, ident, type, level, path, 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 5201280669..d3aec87f1c 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1305,7 +1305,6 @@ pg_available_extensions| SELECT e.name,
LEFT JOIN pg_extension x ON ((e.name = x.extname)));
pg_backend_memory_contexts| SELECT name,
ident,
- parent,
type,
level,
path,
--
2.34.1
On Wed, 31 Jul 2024 at 05:19, Melih Mutlu <m.melihmutlu@gmail.com> wrote:
After the patch [1] that adds a path column to pg_backend_memory_context, the parent context can also be found in the path array. Since there are currently two ways to retrieve information related to the parent of a context, I wonder whether we still want to keep the parent column.
My vote is to remove it.
I think the parent column is only maybe useful as a rough visual
indication of what the parent is. It's dangerous to assume using it
is a reliable way to write a recursive query:
with recursive contexts as (
select name, ident, level, path, parent from pg_backend_memory_contexts
),
c as (
select path[level] as context_id, NULL::int as parent_id,* from
contexts where parent is null
union all
select c1.path[c1.level], c.context_id,c1.* from contexts c1 inner
join c on c.name = c1.parent
)
select count(*) as all_including_false_dups, count(distinct
context_id) as unique from c;
all_including_false_dups | unique
--------------------------+--------
159 | 150
So, with the backend in the state I had it in during this query, the
recursive query shows 9 additional contexts because the recursive
query joining parent to name found a false parent with a name matching
the actual parent because the names are not unique. Given that I
didn't do anything special to create contexts with duplicate names, it
seems duplicates are not rare.
select name,count(*) from pg_backend_memory_contexts group by 1 order
by 2 desc limit 3;
name | count
-------------+-------
index info | 94
dynahash | 15
ExprContext | 7
(3 rows)
I think the first two of the above won't have any children, but the
ExprContext ones can.
David
David Rowley <dgrowleyml@gmail.com> writes:
On Wed, 31 Jul 2024 at 05:19, Melih Mutlu <m.melihmutlu@gmail.com> wrote:
After the patch [1] that adds a path column to pg_backend_memory_context, the parent context can also be found in the path array. Since there are currently two ways to retrieve information related to the parent of a context, I wonder whether we still want to keep the parent column.
My vote is to remove it.
While it's certainly somewhat redundant now, removing it would break
any application queries that are using the column. Simply adding
a column in a system view is a much easier sell than replacing or
removing one.
Perhaps you can make an argument that nobody would be depending
on that column, but I fear that's wishful thinking. Or maybe you
can argue that any query using it is already broken --- but I
think that's only true if someone tries to do the specific sort
of recursive traversal that you illustrated.
regards, tom lane
On Wed, 31 Jul 2024 at 12:35, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Perhaps you can make an argument that nobody would be depending
on that column, but I fear that's wishful thinking. Or maybe you
can argue that any query using it is already broken --- but I
think that's only true if someone tries to do the specific sort
of recursive traversal that you illustrated.
It's true that people could be using it, I certainly don't dispute
that. It's just we don't have any rule that we can't do this sort of
thing. Take f66e8bf87, for example. It removed relhaspkey from
pg_class. It's true that doing that did upset at least one person
[1]: /messages/by-id/CANu8Fiy2RZL+uVnnrzaCTJxMgcKBDOnAR7bDx3n0P=KycbSNhA@mail.gmail.com
example query was broken.
I feel the bar is a bit lower for pg_backend_memory_contexts as it was
only added in v14, so it's not been around as long as pg_class had
been around in 2018 when we removed relhaspkey. My concern here is
that the longer we leave the parent column in, the higher the bar gets
to remove it. That's why I feel like it is worth considering this
now.
One thing we could do is remove it and see if anyone complains. If we
did that today, there's about a year-long window for people to
complain where we could still reverse the decision. Now is probably
the best time where we can consider this so I'd be sad if this
discussion ended on "someone might be using it.".
David
[1]: /messages/by-id/CANu8Fiy2RZL+uVnnrzaCTJxMgcKBDOnAR7bDx3n0P=KycbSNhA@mail.gmail.com
David Rowley <dgrowleyml@gmail.com> writes:
I feel the bar is a bit lower for pg_backend_memory_contexts as it was
only added in v14, so it's not been around as long as pg_class had
been around in 2018 when we removed relhaspkey.
Yeah, and also it's very much a developer-focused view with a limited
audience. It's certainly possible that we could remove the column
and nobody would complain. I just wanted to point out that there is
a compatibility worry here.
One thing we could do is remove it and see if anyone complains. If we
did that today, there's about a year-long window for people to
complain where we could still reverse the decision.
Seems like a plausible compromise.
regards, tom lane
On Wed, 31 Jul 2024 at 13:27, Tom Lane <tgl@sss.pgh.pa.us> wrote:
David Rowley <dgrowleyml@gmail.com> writes:
One thing we could do is remove it and see if anyone complains. If we
did that today, there's about a year-long window for people to
complain where we could still reverse the decision.Seems like a plausible compromise.
Does anyone object to making this happen? i.e. remove
pg_backend_memory_contexts.parent column and see if anyone complains?
If nobody comes up with any reasons against it, then I propose making
this happen.
David
On Tue, 6 Aug 2024 at 17:48, David Rowley <dgrowleyml@gmail.com> wrote:
Does anyone object to making this happen? i.e. remove
pg_backend_memory_contexts.parent column and see if anyone complains?If nobody comes up with any reasons against it, then I propose making
this happen.
I made a few adjustments and pushed the patch. Let's see if anyone complains.
David