Horton

Thread

Commits

  1. Add missing pointer dereference in pg_backend_memory_contexts view

  2. Add path column to pg_backend_memory_contexts view

  3. Allow memory contexts to have both fixed and variable ident strings.

  1. Parent/child context relation in pg_get_backend_memory_contexts()

    Melih Mutlu <m.melihmutlu@gmail.com> — 2023-06-16T14:03:14Z

    Hi hackers,

pg_get_backend_memory_contexts() (and pg_backend_memory_contexts view)
does not display parent/child relation between contexts reliably.
Current version of this function only shows the name of parent context
for each context. The issue here is that it's not guaranteed that
context names are unique. So, this makes it difficult to find the
correct parent of a context.

How can knowing the correct parent context be useful? One important
use-case can be that it would allow us to sum up all the space used by
a particular context and all other subcontexts which stem from that
context.
Calculating this sum is helpful since currently
(total/used/free)_bytes returned by this function does not include
child contexts. For this reason, only looking into the related row in
pg_backend_memory_contexts does not help us to understand how many
bytes that context is actually taking.

Simplest approach to solve this could be just adding two new fields,
id and parent_id, in pg_get_backend_memory_contexts() and ensuring
each context has a unique id. This way allows us to build a correct
memory context "tree".

Please see the attached patch which introduces those two fields.
Couldn't find an existing unique identifier to use. The patch simply
assigns an id during the execution of
pg_get_backend_memory_contexts() and does not store those id's
anywhere. This means that these id's may be different in each call.

With this change, here's a query to find how much space used by each
context including its children:

> WITH RECURSIVE cte AS (
>     SELECT id, total_bytes, id as root, name as root_name
>     FROM memory_contexts
> UNION ALL
>     SELECT r.id, r.total_bytes, cte.root, cte.root_name
>     FROM memory_contexts r
>     INNER JOIN cte ON r.parent_id = cte.id
> ),
> memory_contexts AS (
>     SELECT * FROM pg_backend_memory_contexts
> )
> SELECT root as id, root_name as name, sum(total_bytes)
> FROM cte
> GROUP BY root, root_name
> ORDER BY sum DESC;


You should see that TopMemoryContext is the one with highest allocated
space since all other contexts are simply created under
TopMemoryContext.


Also; even though having a correct link between parent/child contexts
can be useful to find out many other things as well by only writing
SQL queries, it might require complex recursive queries similar to the
one in case of total_bytes including children. Maybe, we can also
consider adding such frequently used and/or useful information as new
fields in pg_get_backend_memory_contexts() too.


I appreciate any comment/feedback on this.

Thanks,
-- 
Melih Mutlu
Microsoft

  2. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    Melih Mutlu <m.melihmutlu@gmail.com> — 2023-08-04T18:16:49Z

    Hi hackers,


Melih Mutlu <m.melihmutlu@gmail.com>, 16 Haz 2023 Cum, 17:03 tarihinde şunu
yazdı:

> With this change, here's a query to find how much space used by each
> context including its children:
>
> > WITH RECURSIVE cte AS (
> >     SELECT id, total_bytes, id as root, name as root_name
> >     FROM memory_contexts
> > UNION ALL
> >     SELECT r.id, r.total_bytes, cte.root, cte.root_name
> >     FROM memory_contexts r
> >     INNER JOIN cte ON r.parent_id = cte.id
> > ),
> > memory_contexts AS (
> >     SELECT * FROM pg_backend_memory_contexts
> > )
> > SELECT root as id, root_name as name, sum(total_bytes)
> > FROM cte
> > GROUP BY root, root_name
> > ORDER BY sum DESC;
>

Given that the above query to get total bytes including all children is
still a complex one, I decided to add an additional info in
pg_backend_memory_contexts.
The new "path" field displays an integer array that consists of ids of all
parents for the current context. This way it's easier to tell whether a
context is a child of another context, and we don't need to use recursive
queries to get this info.

Here how pg_backend_memory_contexts would look like with this patch:

postgres=# SELECT name, id, parent, parent_id, path
FROM pg_backend_memory_contexts
ORDER BY total_bytes DESC LIMIT 10;
          name           | id  |      parent      | parent_id |     path
-------------------------+-----+------------------+-----------+--------------
 CacheMemoryContext      |  27 | TopMemoryContext |         0 | {0}
 Timezones               | 124 | TopMemoryContext |         0 | {0}
 TopMemoryContext        |   0 |                  |           |
 MessageContext          |   8 | TopMemoryContext |         0 | {0}
 WAL record construction | 118 | TopMemoryContext |         0 | {0}
 ExecutorState           |  18 | PortalContext    |        17 | {0,16,17}
 TupleSort main          |  19 | ExecutorState    |        18 | {0,16,17,18}
 TransactionAbortContext |  14 | TopMemoryContext |         0 | {0}
 smgr relation table     |  10 | TopMemoryContext |         0 | {0}
 GUC hash table          | 123 | GUCMemoryContext |       122 | {0,122}
(10 rows)


An example query to calculate the total_bytes including its children for a
context (say CacheMemoryContext) would look like this:

WITH contexts AS (
SELECT * FROM pg_backend_memory_contexts
)
SELECT sum(total_bytes)
FROM contexts
WHERE ARRAY[(SELECT id FROM contexts WHERE name = 'CacheMemoryContext')] <@
path;

We still need to use cte since ids are not persisted and might change in
each run of pg_backend_memory_contexts. Materializing the result can
prevent any inconsistencies due to id change. Also it can be even good for
performance reasons as well.

Any thoughts?

Thanks,
-- 
Melih Mutlu
Microsoft

  3. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    Andres Freund <andres@anarazel.de> — 2023-10-12T16:23:09Z

    Hi,

On 2023-08-04 21:16:49 +0300, Melih Mutlu wrote:
> Melih Mutlu <m.melihmutlu@gmail.com>, 16 Haz 2023 Cum, 17:03 tarihinde şunu
> yazdı:
> 
> > With this change, here's a query to find how much space used by each
> > context including its children:
> >
> > > WITH RECURSIVE cte AS (
> > >     SELECT id, total_bytes, id as root, name as root_name
> > >     FROM memory_contexts
> > > UNION ALL
> > >     SELECT r.id, r.total_bytes, cte.root, cte.root_name
> > >     FROM memory_contexts r
> > >     INNER JOIN cte ON r.parent_id = cte.id
> > > ),
> > > memory_contexts AS (
> > >     SELECT * FROM pg_backend_memory_contexts
> > > )
> > > SELECT root as id, root_name as name, sum(total_bytes)
> > > FROM cte
> > > GROUP BY root, root_name
> > > ORDER BY sum DESC;
> >
> 
> Given that the above query to get total bytes including all children is
> still a complex one, I decided to add an additional info in
> pg_backend_memory_contexts.
> The new "path" field displays an integer array that consists of ids of all
> parents for the current context. This way it's easier to tell whether a
> context is a child of another context, and we don't need to use recursive
> queries to get this info.

I think that does make it a good bit easier. Both to understand and to use.



> Here how pg_backend_memory_contexts would look like with this patch:
> 
> postgres=# SELECT name, id, parent, parent_id, path
> FROM pg_backend_memory_contexts
> ORDER BY total_bytes DESC LIMIT 10;
>           name           | id  |      parent      | parent_id |     path
> -------------------------+-----+------------------+-----------+--------------
>  CacheMemoryContext      |  27 | TopMemoryContext |         0 | {0}
>  Timezones               | 124 | TopMemoryContext |         0 | {0}
>  TopMemoryContext        |   0 |                  |           |
>  MessageContext          |   8 | TopMemoryContext |         0 | {0}
>  WAL record construction | 118 | TopMemoryContext |         0 | {0}
>  ExecutorState           |  18 | PortalContext    |        17 | {0,16,17}
>  TupleSort main          |  19 | ExecutorState    |        18 | {0,16,17,18}
>  TransactionAbortContext |  14 | TopMemoryContext |         0 | {0}
>  smgr relation table     |  10 | TopMemoryContext |         0 | {0}
>  GUC hash table          | 123 | GUCMemoryContext |       122 | {0,122}
> (10 rows)

Would we still need the parent_id column?


> +
> +     <row>
> +      <entry role="catalog_table_entry"><para role="column_definition">
> +       <structfield>context_id</structfield> <type>int4</type>
> +      </para>
> +      <para>
> +       Current context id
> +      </para></entry>
> +     </row>

I think the docs here need to warn that the id is ephemeral and will likely
differ in the next invocation.


> +     <row>
> +      <entry role="catalog_table_entry"><para role="column_definition">
> +       <structfield>parent_id</structfield> <type>int4</type>
> +      </para>
> +      <para>
> +       Parent context id
> +      </para></entry>
> +     </row>
> +
> +     <row>
> +      <entry role="catalog_table_entry"><para role="column_definition">
> +       <structfield>path</structfield> <type>int4</type>
> +      </para>
> +      <para>
> +       Path to reach the current context from TopMemoryContext
> +      </para></entry>
> +     </row>

Perhaps we should include some hint here how it could be used?


>      </tbody>
>     </tgroup>
>    </table>
> diff --git a/src/backend/utils/adt/mcxtfuncs.c b/src/backend/utils/adt/mcxtfuncs.c
> index 92ca5b2f72..81cb35dd47 100644
> --- a/src/backend/utils/adt/mcxtfuncs.c
> +++ b/src/backend/utils/adt/mcxtfuncs.c
> @@ -20,6 +20,7 @@
>  #include "mb/pg_wchar.h"
>  #include "storage/proc.h"
>  #include "storage/procarray.h"
> +#include "utils/array.h"
>  #include "utils/builtins.h"
>  
>  /* ----------
> @@ -28,6 +29,8 @@
>   */
>  #define MEMORY_CONTEXT_IDENT_DISPLAY_SIZE	1024
>  
> +static Datum convert_path_to_datum(List *path);
> +
>  /*
>   * PutMemoryContextsStatsTupleStore
>   *		One recursion level for pg_get_backend_memory_contexts.
> @@ -35,9 +38,10 @@
>  static void
>  PutMemoryContextsStatsTupleStore(Tuplestorestate *tupstore,
>  								 TupleDesc tupdesc, MemoryContext context,
> -								 const char *parent, int level)
> +								 const char *parent, int level, int *context_id,
> +								 int parent_id, List *path)
>  {
> -#define PG_GET_BACKEND_MEMORY_CONTEXTS_COLS	9
> +#define PG_GET_BACKEND_MEMORY_CONTEXTS_COLS	12
>  
>  	Datum		values[PG_GET_BACKEND_MEMORY_CONTEXTS_COLS];
>  	bool		nulls[PG_GET_BACKEND_MEMORY_CONTEXTS_COLS];
> @@ -45,6 +49,7 @@ PutMemoryContextsStatsTupleStore(Tuplestorestate *tupstore,
>  	MemoryContext child;
>  	const char *name;
>  	const char *ident;
> +	int  current_context_id = (*context_id)++;
>  
>  	Assert(MemoryContextIsValid(context));
>  
> @@ -103,13 +108,29 @@ PutMemoryContextsStatsTupleStore(Tuplestorestate *tupstore,
>  	values[6] = Int64GetDatum(stat.freespace);
>  	values[7] = Int64GetDatum(stat.freechunks);
>  	values[8] = Int64GetDatum(stat.totalspace - stat.freespace);
> +	values[9] = Int32GetDatum(current_context_id);
> +
> +	if(parent_id < 0)
> +		/* TopMemoryContext has no parent context */
> +		nulls[10] = true;
> +	else
> +		values[10] = Int32GetDatum(parent_id);
> +
> +	if (path == NIL)
> +		nulls[11] = true;
> +	else
> +		values[11] = convert_path_to_datum(path);
> +
>  	tuplestore_putvalues(tupstore, tupdesc, values, nulls);
>  
> +	path = lappend_int(path, current_context_id);
>  	for (child = context->firstchild; child != NULL; child = child->nextchild)
>  	{
> -		PutMemoryContextsStatsTupleStore(tupstore, tupdesc,
> -										 child, name, level + 1);
> +		PutMemoryContextsStatsTupleStore(tupstore, tupdesc, child, name,
> +										 level+1, context_id,
> +										 current_context_id, path);
>  	}
> +	path = list_delete_last(path);
>  }
>  
>  /*
> @@ -120,10 +141,15 @@ Datum
>  pg_get_backend_memory_contexts(PG_FUNCTION_ARGS)
>  {
>  	ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
> +	int context_id = 0;
> +	List *path = NIL;
> +
> +	elog(LOG, "pg_get_backend_memory_contexts called");
>  
>  	InitMaterializedSRF(fcinfo, 0);
>  	PutMemoryContextsStatsTupleStore(rsinfo->setResult, rsinfo->setDesc,
> -									 TopMemoryContext, NULL, 0);
> +									 TopMemoryContext, NULL, 0, &context_id,
> +									 -1, path);
>  
>  	return (Datum) 0;
>  }
> @@ -193,3 +219,26 @@ pg_log_backend_memory_contexts(PG_FUNCTION_ARGS)
>  
>  	PG_RETURN_BOOL(true);
>  }
> +
> +/*
> + * Convert a list of context ids to a int[] Datum
> + */
> +static Datum
> +convert_path_to_datum(List *path)
> +{
> +	Datum	   *datum_array;
> +	int			length;
> +	ArrayType  *result_array;
> +	ListCell   *lc;
> +
> +	length = list_length(path);
> +	datum_array = (Datum *) palloc(length * sizeof(Datum));
> +	length = 0;
> +	foreach(lc, path)
> +	{
> +		datum_array[length++] = Int32GetDatum((int) lfirst_int(lc));

The "(int)" in front of lfirst_int() seems redundant?


I think it'd be good to have some minimal test for this. E.g. checking that
there's multiple contexts below cache memory context or such.

Greetings,

Andres Freund

  4. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    Stephen Frost <sfrost@snowman.net> — 2023-10-18T19:53:30Z

    Greetings,

* Melih Mutlu (m.melihmutlu@gmail.com) wrote:
> Melih Mutlu <m.melihmutlu@gmail.com>, 16 Haz 2023 Cum, 17:03 tarihinde şunu
> yazdı:
> 
> > With this change, here's a query to find how much space used by each
> > context including its children:
> >
> > > WITH RECURSIVE cte AS (
> > >     SELECT id, total_bytes, id as root, name as root_name
> > >     FROM memory_contexts
> > > UNION ALL
> > >     SELECT r.id, r.total_bytes, cte.root, cte.root_name
> > >     FROM memory_contexts r
> > >     INNER JOIN cte ON r.parent_id = cte.id
> > > ),
> > > memory_contexts AS (
> > >     SELECT * FROM pg_backend_memory_contexts
> > > )
> > > SELECT root as id, root_name as name, sum(total_bytes)
> > > FROM cte
> > > GROUP BY root, root_name
> > > ORDER BY sum DESC;
> 
> Given that the above query to get total bytes including all children is
> still a complex one, I decided to add an additional info in
> pg_backend_memory_contexts.
> The new "path" field displays an integer array that consists of ids of all
> parents for the current context. This way it's easier to tell whether a
> context is a child of another context, and we don't need to use recursive
> queries to get this info.

Nice, this does seem quite useful.

> Here how pg_backend_memory_contexts would look like with this patch:
> 
> postgres=# SELECT name, id, parent, parent_id, path
> FROM pg_backend_memory_contexts
> ORDER BY total_bytes DESC LIMIT 10;
>           name           | id  |      parent      | parent_id |     path
> -------------------------+-----+------------------+-----------+--------------
>  CacheMemoryContext      |  27 | TopMemoryContext |         0 | {0}
>  Timezones               | 124 | TopMemoryContext |         0 | {0}
>  TopMemoryContext        |   0 |                  |           |
>  MessageContext          |   8 | TopMemoryContext |         0 | {0}
>  WAL record construction | 118 | TopMemoryContext |         0 | {0}
>  ExecutorState           |  18 | PortalContext    |        17 | {0,16,17}
>  TupleSort main          |  19 | ExecutorState    |        18 | {0,16,17,18}
>  TransactionAbortContext |  14 | TopMemoryContext |         0 | {0}
>  smgr relation table     |  10 | TopMemoryContext |         0 | {0}
>  GUC hash table          | 123 | GUCMemoryContext |       122 | {0,122}
> (10 rows)
> 
> An example query to calculate the total_bytes including its children for a
> context (say CacheMemoryContext) would look like this:
> 
> WITH contexts AS (
> SELECT * FROM pg_backend_memory_contexts
> )
> SELECT sum(total_bytes)
> FROM contexts
> WHERE ARRAY[(SELECT id FROM contexts WHERE name = 'CacheMemoryContext')] <@
> path;

I wonder if we should perhaps just include
"total_bytes_including_children" as another column?  Certainly seems
like a very useful thing that folks would like to see.  We could do that
either with C, or even something as simple as changing the view to do
something like:

WITH contexts AS MATERIALIZED (
  SELECT * FROM pg_get_backend_memory_contexts()
)
SELECT
  *,
  coalesce
  (
    (
      (SELECT sum(total_bytes) FROM contexts WHERE ARRAY[a.id] <@ path)
      + total_bytes
    ),
    total_bytes
  ) AS total_bytes_including_children
FROM contexts a;

> We still need to use cte since ids are not persisted and might change in
> each run of pg_backend_memory_contexts. Materializing the result can
> prevent any inconsistencies due to id change. Also it can be even good for
> performance reasons as well.

I don't think we really want this to be materialized, do we?  Where this
is particularly interesting is when it's being dumped to the log ( ...
though I wish we could do better than that and hope we do in the future)
while something is ongoing in a given backend and if we do that a few
times we are able to see what's changing in terms of allocations,
whereas if we materialized it (when?  transaction start?  first time
it's asked for?) then we'd only ever get the one view from whenever the
snapshot was taken.

> Any thoughts?

Generally +1 from me for working on improving this.

Thanks!

Stephen

  5. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    Andres Freund <andres@anarazel.de> — 2023-10-19T01:17:53Z

    Hi,

On 2023-10-18 15:53:30 -0400, Stephen Frost wrote:
> > Here how pg_backend_memory_contexts would look like with this patch:
> > 
> > postgres=# SELECT name, id, parent, parent_id, path
> > FROM pg_backend_memory_contexts
> > ORDER BY total_bytes DESC LIMIT 10;
> >           name           | id  |      parent      | parent_id |     path
> > -------------------------+-----+------------------+-----------+--------------
> >  CacheMemoryContext      |  27 | TopMemoryContext |         0 | {0}
> >  Timezones               | 124 | TopMemoryContext |         0 | {0}
> >  TopMemoryContext        |   0 |                  |           |
> >  MessageContext          |   8 | TopMemoryContext |         0 | {0}
> >  WAL record construction | 118 | TopMemoryContext |         0 | {0}
> >  ExecutorState           |  18 | PortalContext    |        17 | {0,16,17}
> >  TupleSort main          |  19 | ExecutorState    |        18 | {0,16,17,18}
> >  TransactionAbortContext |  14 | TopMemoryContext |         0 | {0}
> >  smgr relation table     |  10 | TopMemoryContext |         0 | {0}
> >  GUC hash table          | 123 | GUCMemoryContext |       122 | {0,122}
> > (10 rows)
> > 
> > An example query to calculate the total_bytes including its children for a
> > context (say CacheMemoryContext) would look like this:
> > 
> > WITH contexts AS (
> > SELECT * FROM pg_backend_memory_contexts
> > )
> > SELECT sum(total_bytes)
> > FROM contexts
> > WHERE ARRAY[(SELECT id FROM contexts WHERE name = 'CacheMemoryContext')] <@
> > path;
> 
> I wonder if we should perhaps just include
> "total_bytes_including_children" as another column?  Certainly seems
> like a very useful thing that folks would like to see.

The "issue" is where to stop - should we also add that for some of the other
columns? They are a bit less important, but not that much.


> > We still need to use cte since ids are not persisted and might change in
> > each run of pg_backend_memory_contexts. Materializing the result can
> > prevent any inconsistencies due to id change. Also it can be even good for
> > performance reasons as well.
> 
> I don't think we really want this to be materialized, do we?  Where this
> is particularly interesting is when it's being dumped to the log ( ...
> though I wish we could do better than that and hope we do in the future)
> while something is ongoing in a given backend and if we do that a few
> times we are able to see what's changing in terms of allocations,
> whereas if we materialized it (when?  transaction start?  first time
> it's asked for?) then we'd only ever get the one view from whenever the
> snapshot was taken.

I think the comment was just about the need to use a CTE, because self-joining
with divergent versions of pg_backend_memory_contexts would not always work
out well.

Greetings,

Andres Freund

  6. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    Stephen Frost <sfrost@snowman.net> — 2023-10-19T22:01:23Z

    Greetings,

* Andres Freund (andres@anarazel.de) wrote:
> On 2023-10-18 15:53:30 -0400, Stephen Frost wrote:
> > > Here how pg_backend_memory_contexts would look like with this patch:
> > > 
> > > postgres=# SELECT name, id, parent, parent_id, path
> > > FROM pg_backend_memory_contexts
> > > ORDER BY total_bytes DESC LIMIT 10;
> > >           name           | id  |      parent      | parent_id |     path
> > > -------------------------+-----+------------------+-----------+--------------
> > >  CacheMemoryContext      |  27 | TopMemoryContext |         0 | {0}
> > >  Timezones               | 124 | TopMemoryContext |         0 | {0}
> > >  TopMemoryContext        |   0 |                  |           |
> > >  MessageContext          |   8 | TopMemoryContext |         0 | {0}
> > >  WAL record construction | 118 | TopMemoryContext |         0 | {0}
> > >  ExecutorState           |  18 | PortalContext    |        17 | {0,16,17}
> > >  TupleSort main          |  19 | ExecutorState    |        18 | {0,16,17,18}
> > >  TransactionAbortContext |  14 | TopMemoryContext |         0 | {0}
> > >  smgr relation table     |  10 | TopMemoryContext |         0 | {0}
> > >  GUC hash table          | 123 | GUCMemoryContext |       122 | {0,122}
> > > (10 rows)
> > > 
> > > An example query to calculate the total_bytes including its children for a
> > > context (say CacheMemoryContext) would look like this:
> > > 
> > > WITH contexts AS (
> > > SELECT * FROM pg_backend_memory_contexts
> > > )
> > > SELECT sum(total_bytes)
> > > FROM contexts
> > > WHERE ARRAY[(SELECT id FROM contexts WHERE name = 'CacheMemoryContext')] <@
> > > path;
> > 
> > I wonder if we should perhaps just include
> > "total_bytes_including_children" as another column?  Certainly seems
> > like a very useful thing that folks would like to see.
> 
> The "issue" is where to stop - should we also add that for some of the other
> columns? They are a bit less important, but not that much.

I'm not sure the others really make sense to aggregate in this way as
free space isn't able to be moved between contexts.  That said, if
someone wants it then I'm not against that.  I'm actively in support of
adding an aggregated total though as that, at least to me, seems to be
very useful to have.

Thanks,

Stephen

  7. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    Melih Mutlu <m.melihmutlu@gmail.com> — 2023-10-23T12:02:27Z

    Hi,

Thanks for reviewing.
Attached the updated patch v3.

Andres Freund <andres@anarazel.de>, 12 Eki 2023 Per, 19:23 tarihinde şunu
yazdı:

> > Here how pg_backend_memory_contexts would look like with this patch:
> >
> > postgres=# SELECT name, id, parent, parent_id, path
> > FROM pg_backend_memory_contexts
> > ORDER BY total_bytes DESC LIMIT 10;
> >           name           | id  |      parent      | parent_id |     path
> >
> -------------------------+-----+------------------+-----------+--------------
> >  CacheMemoryContext      |  27 | TopMemoryContext |         0 | {0}
> >  Timezones               | 124 | TopMemoryContext |         0 | {0}
> >  TopMemoryContext        |   0 |                  |           |
> >  MessageContext          |   8 | TopMemoryContext |         0 | {0}
> >  WAL record construction | 118 | TopMemoryContext |         0 | {0}
> >  ExecutorState           |  18 | PortalContext    |        17 | {0,16,17}
> >  TupleSort main          |  19 | ExecutorState    |        18 |
> {0,16,17,18}
> >  TransactionAbortContext |  14 | TopMemoryContext |         0 | {0}
> >  smgr relation table     |  10 | TopMemoryContext |         0 | {0}
> >  GUC hash table          | 123 | GUCMemoryContext |       122 | {0,122}
> > (10 rows)
>
> Would we still need the parent_id column?
>

I guess not. Assuming the path column is sorted from TopMemoryContext to
the parent one level above, parent_id can be found using the path column if
needed.
Removed parent_id.


> > +
> > +     <row>
> > +      <entry role="catalog_table_entry"><para role="column_definition">
> > +       <structfield>context_id</structfield> <type>int4</type>
> > +      </para>
> > +      <para>
> > +       Current context id
> > +      </para></entry>
> > +     </row>
>
> I think the docs here need to warn that the id is ephemeral and will likely
> differ in the next invocation.
>

Done.

> +     <row>
> > +      <entry role="catalog_table_entry"><para role="column_definition">
> > +       <structfield>parent_id</structfield> <type>int4</type>
> > +      </para>
> > +      <para>
> > +       Parent context id
> > +      </para></entry>
> > +     </row>
> > +
> > +     <row>
> > +      <entry role="catalog_table_entry"><para role="column_definition">
> > +       <structfield>path</structfield> <type>int4</type>
> > +      </para>
> > +      <para>
> > +       Path to reach the current context from TopMemoryContext
> > +      </para></entry>
> > +     </row>
>
> Perhaps we should include some hint here how it could be used?
>

I added more explanation but not sure if that is what you asked for. Do you
want a hint that is related to a more specific use case?

> +     length = list_length(path);
> > +     datum_array = (Datum *) palloc(length * sizeof(Datum));
> > +     length = 0;
> > +     foreach(lc, path)
> > +     {
> > +             datum_array[length++] = Int32GetDatum((int)
> lfirst_int(lc));
>
> The "(int)" in front of lfirst_int() seems redundant?
>

Removed.

I think it'd be good to have some minimal test for this. E.g. checking that
> there's multiple contexts below cache memory context or such.
>

Added new tests in sysview.sql.


Stephen Frost <sfrost@snowman.net>, 18 Eki 2023 Çar, 22:53 tarihinde şunu
yazdı:

> I wonder if we should perhaps just include
> "total_bytes_including_children" as another column?  Certainly seems
> like a very useful thing that folks would like to see.  We could do that
> either with C, or even something as simple as changing the view to do
> something like:
>
> WITH contexts AS MATERIALIZED (
>   SELECT * FROM pg_get_backend_memory_contexts()
> )
> SELECT
>   *,
>   coalesce
>   (
>     (
>       (SELECT sum(total_bytes) FROM contexts WHERE ARRAY[a.id] <@ path)
>       + total_bytes
>     ),
>     total_bytes
>   ) AS total_bytes_including_children
> FROM contexts a;
>

I added a "total_bytes_including_children" column as you suggested. Did
that with C since it seemed faster than doing it by changing the view.

-- Calculating total_bytes_including_children by modifying the view
postgres=# select * from pg_backend_memory_contexts ;
Time: 30.462 ms

-- Calculating total_bytes_including_children with C
postgres=# select * from pg_backend_memory_contexts ;
Time: 1.511 ms


Thanks,
-- 
Melih Mutlu
Microsoft

  8. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    torikoshia <torikoshia@oss.nttdata.com> — 2023-12-04T04:43:18Z

    Thanks for working on this improvement!

On 2023-10-23 21:02, Melih Mutlu wrote:
> Hi,
> 
> Thanks for reviewing.
> Attached the updated patch v3.

I reviewed v3 patch and here are some minor comments:

> +     <row>
> +      <entry role="catalog_table_entry"><para 
> role="column_definition">
> +       <structfield>path</structfield> <type>int4</type>

Should 'int4' be 'int4[]'?
Other system catalog columns such as pg_groups.grolist distinguish 
whther the type is a array or not.

> +       Path to reach the current context from TopMemoryContext. 
> Context ids in
> +       this list represents all parents of the current context. This 
> can be
> +       used to build the parent and child relation.

It seems last "." is not necessary considering other explanations for 
each field end without it.

+                                const char *parent, int level, int 
*context_id,
+                                List *path, Size 
*total_bytes_inc_chidlren)

'chidlren' -> 'children'


+   elog(LOG, "pg_get_backend_memory_contexts called");

Is this message necessary?


There was warning when applying the patch:

   % git apply 
../patch/pg_backend_memory_context_refine/v3-0001-Adding-id-parent_id-into-pg_backend_memory_contex.patch
   
../patch/pg_backend_memory_context_refine/v3-0001-Adding-id-parent_id-into-pg_backend_memory_contex.patch:282: 
trailing whitespace.
   select count(*) > 0
   
../patch/pg_backend_memory_context_refine/v3-0001-Adding-id-parent_id-into-pg_backend_memory_contex.patch:283: 
trailing whitespace.
   from contexts
   warning: 2 lines add whitespace errors.

-- 
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation

  9. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    Melih Mutlu <m.melihmutlu@gmail.com> — 2024-01-03T11:40:01Z

    Hi,

Thanks for reviewing. Please find the updated patch attached.

torikoshia <torikoshia@oss.nttdata.com>, 4 Ara 2023 Pzt, 07:43 tarihinde
şunu yazdı:

> I reviewed v3 patch and here are some minor comments:
>
> > +     <row>
> > +      <entry role="catalog_table_entry"><para
> > role="column_definition">
> > +       <structfield>path</structfield> <type>int4</type>
>
> Should 'int4' be 'int4[]'?
> Other system catalog columns such as pg_groups.grolist distinguish
> whther the type is a array or not.
>

Right! Done.


>
> > +       Path to reach the current context from TopMemoryContext.
> > Context ids in
> > +       this list represents all parents of the current context. This
> > can be
> > +       used to build the parent and child relation.
>
> It seems last "." is not necessary considering other explanations for
> each field end without it.
>

Done.


> +                                const char *parent, int level, int
> *context_id,
> +                                List *path, Size
> *total_bytes_inc_chidlren)
>
> 'chidlren' -> 'children'
>

Done.


> +   elog(LOG, "pg_get_backend_memory_contexts called");
>
> Is this message necessary?
>

I guess I added this line for debugging and then forgot to remove. Now
removed.

There was warning when applying the patch:
>
>    % git apply
>
> ../patch/pg_backend_memory_context_refine/v3-0001-Adding-id-parent_id-into-pg_backend_memory_contex.patch
>
> ../patch/pg_backend_memory_context_refine/v3-0001-Adding-id-parent_id-into-pg_backend_memory_contex.patch:282:
>
> trailing whitespace.
>    select count(*) > 0
>
> ../patch/pg_backend_memory_context_refine/v3-0001-Adding-id-parent_id-into-pg_backend_memory_contex.patch:283:
>
> trailing whitespace.
>    from contexts
>    warning: 2 lines add whitespace errors.
>

Fixed.

Thanks,
-- 
Melih Mutlu
Microsoft

  10. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    torikoshia <torikoshia@oss.nttdata.com> — 2024-01-10T06:37:01Z

    On 2024-01-03 20:40, Melih Mutlu wrote:
> Hi,
> 
> Thanks for reviewing. Please find the updated patch attached.
> 
> torikoshia <torikoshia@oss.nttdata.com>, 4 Ara 2023 Pzt, 07:43
> tarihinde şunu yazdı:
> 
>> I reviewed v3 patch and here are some minor comments:
>> 
>>> +     <row>
>>> +      <entry role="catalog_table_entry"><para
>>> role="column_definition">
>>> +       <structfield>path</structfield> <type>int4</type>
>> 
>> Should 'int4' be 'int4[]'?
>> Other system catalog columns such as pg_groups.grolist distinguish
>> whther the type is a array or not.
> 
> Right! Done.
> 
>>> +       Path to reach the current context from TopMemoryContext.
>>> Context ids in
>>> +       this list represents all parents of the current context.
>> This
>>> can be
>>> +       used to build the parent and child relation.
>> 
>> It seems last "." is not necessary considering other explanations
>> for
>> each field end without it.
> 
> Done.
> 
>> +                                const char *parent, int level, int
>> *context_id,
>> +                                List *path, Size
>> *total_bytes_inc_chidlren)
>> 
>> 'chidlren' -> 'children'
> 
> Done.
> 
>> +   elog(LOG, "pg_get_backend_memory_contexts called");
>> 
>> Is this message necessary?
> 
> I guess I added this line for debugging and then forgot to remove. Now
> removed.
> 
>> There was warning when applying the patch:
>> 
>> % git apply
>> 
> ../patch/pg_backend_memory_context_refine/v3-0001-Adding-id-parent_id-into-pg_backend_memory_contex.patch
>> 
>> 
> ../patch/pg_backend_memory_context_refine/v3-0001-Adding-id-parent_id-into-pg_backend_memory_contex.patch:282:
>> 
>> trailing whitespace.
>> select count(*) > 0
>> 
>> 
> ../patch/pg_backend_memory_context_refine/v3-0001-Adding-id-parent_id-into-pg_backend_memory_contex.patch:283:
>> 
>> trailing whitespace.
>> from contexts
>> warning: 2 lines add whitespace errors.
> 
> Fixed.
> 
> Thanks,--
> 
> Melih Mutlu
> Microsoft

Thanks for updating the patch.

> +     <row>
> +      <entry role="catalog_table_entry"><para 
> role="column_definition">
> +       <structfield>context_id</structfield> <type>int4</type>
> +      </para>
> +      <para>
> +       Current context id. Note that the context id is a temporary id 
> and may
> +       change in each invocation
> +      </para></entry>
> +     </row>
> +
> +     <row>
> +      <entry role="catalog_table_entry"><para 
> role="column_definition">
> +       <structfield>path</structfield> <type>int4[]</type>
> +      </para>
> +      <para>
> +       Path to reach the current context from TopMemoryContext. 
> Context ids in
> +       this list represents all parents of the current context. This 
> can be
> +       used to build the parent and child relation
> +      </para></entry>
> +     </row>
> +
> +     <row>
> +      <entry role="catalog_table_entry"><para 
> role="column_definition">
> +       <structfield>total_bytes_including_children</structfield> 
> <type>int8</type>
> +      </para>
> +      <para>
> +       Total bytes allocated for this memory context including its 
> children
> +      </para></entry>
> +     </row>

These columns are currently added to the bottom of the table, but it may 
be better to put semantically similar items close together and change 
the insertion position with reference to other system views. For 
example,

- In pg_group and pg_user, 'id' is placed on the line following 'name', 
so 'context_id' be placed on the line following 'name'
- 'path' is similar with 'parent' and 'level' in that these are 
information about the location of the context, 'path' be placed to next 
to them.

If we do this, orders of columns in the system view should be the same, 
I think.


> +   ListCell   *lc;
> +
> +   length = list_length(path);
> +   datum_array = (Datum *) palloc(length * sizeof(Datum));
> +   length = 0;
> +   foreach(lc, path)
> +   {
> +       datum_array[length++] = Int32GetDatum(lfirst_int(lc));
> +   }

14dd0f27d have introduced new macro foreach_int.
It seems to be able to make the code a bit simpler and the commit log 
says this macro is primarily intended for use in new code. For example:

|    int id;
|
|    length = list_length(path);
|    datum_array = (Datum *) palloc(length * sizeof(Datum));
|    length = 0;
|    foreach_int(id, path)
|    {
|        datum_array[length++] = Int32GetDatum(id);
|    }



-- 
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation

  11. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    Melih Mutlu <m.melihmutlu@gmail.com> — 2024-01-16T09:41:22Z

    Hi,

Thanks for reviewing.

torikoshia <torikoshia@oss.nttdata.com>, 10 Oca 2024 Çar, 09:37 tarihinde
şunu yazdı:

> > +     <row>
> > +      <entry role="catalog_table_entry"><para
> > role="column_definition">
> > +       <structfield>context_id</structfield> <type>int4</type>
> > +      </para>
> > +      <para>
> > +       Current context id. Note that the context id is a temporary id
> > and may
> > +       change in each invocation
> > +      </para></entry>
> > +     </row>
> > +
> > +     <row>
> > +      <entry role="catalog_table_entry"><para
> > role="column_definition">
> > +       <structfield>path</structfield> <type>int4[]</type>
> > +      </para>
> > +      <para>
> > +       Path to reach the current context from TopMemoryContext.
> > Context ids in
> > +       this list represents all parents of the current context. This
> > can be
> > +       used to build the parent and child relation
> > +      </para></entry>
> > +     </row>
> > +
> > +     <row>
> > +      <entry role="catalog_table_entry"><para
> > role="column_definition">
> > +       <structfield>total_bytes_including_children</structfield>
> > <type>int8</type>
> > +      </para>
> > +      <para>
> > +       Total bytes allocated for this memory context including its
> > children
> > +      </para></entry>
> > +     </row>
>
> These columns are currently added to the bottom of the table, but it may
> be better to put semantically similar items close together and change
> the insertion position with reference to other system views. For
> example,
>
> - In pg_group and pg_user, 'id' is placed on the line following 'name',
> so 'context_id' be placed on the line following 'name'
> - 'path' is similar with 'parent' and 'level' in that these are
> information about the location of the context, 'path' be placed to next
> to them.
>
> If we do this, orders of columns in the system view should be the same,
> I think.
>

I've done what you suggested. Also moved "total_bytes_including_children"
right after "total_bytes".


14dd0f27d have introduced new macro foreach_int.
> It seems to be able to make the code a bit simpler and the commit log
> says this macro is primarily intended for use in new code. For example:
>

Makes sense. Done.

Thanks,
-- 
Melih Mutlu
Microsoft

  12. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    torikoshia <torikoshia@oss.nttdata.com> — 2024-01-19T08:41:45Z

    On 2024-01-16 18:41, Melih Mutlu wrote:
> Hi,
> 
> Thanks for reviewing.
> 
> torikoshia <torikoshia@oss.nttdata.com>, 10 Oca 2024 Çar, 09:37
> tarihinde şunu yazdı:
> 
>>> +     <row>
>>> +      <entry role="catalog_table_entry"><para
>>> role="column_definition">
>>> +       <structfield>context_id</structfield> <type>int4</type>
>>> +      </para>
>>> +      <para>
>>> +       Current context id. Note that the context id is a
>> temporary id
>>> and may
>>> +       change in each invocation
>>> +      </para></entry>
>>> +     </row>
>>> +
>>> +     <row>
>>> +      <entry role="catalog_table_entry"><para
>>> role="column_definition">
>>> +       <structfield>path</structfield> <type>int4[]</type>
>>> +      </para>
>>> +      <para>
>>> +       Path to reach the current context from TopMemoryContext.
>>> Context ids in
>>> +       this list represents all parents of the current context.
>> This
>>> can be
>>> +       used to build the parent and child relation
>>> +      </para></entry>
>>> +     </row>
>>> +
>>> +     <row>
>>> +      <entry role="catalog_table_entry"><para
>>> role="column_definition">
>>> +       <structfield>total_bytes_including_children</structfield>
>>> <type>int8</type>
>>> +      </para>
>>> +      <para>
>>> +       Total bytes allocated for this memory context including
>> its
>>> children
>>> +      </para></entry>
>>> +     </row>
>> 
>> These columns are currently added to the bottom of the table, but it
>> may
>> be better to put semantically similar items close together and
>> change
>> the insertion position with reference to other system views. For
>> example,
>> 
>> - In pg_group and pg_user, 'id' is placed on the line following
>> 'name',
>> so 'context_id' be placed on the line following 'name'
>> - 'path' is similar with 'parent' and 'level' in that these are
>> information about the location of the context, 'path' be placed to
>> next
>> to them.
>> 
>> If we do this, orders of columns in the system view should be the
>> same,
>> I think.
> 
> I've done what you suggested. Also moved
> "total_bytes_including_children" right after "total_bytes".
> 
>> 14dd0f27d have introduced new macro foreach_int.
>> It seems to be able to make the code a bit simpler and the commit
>> log
>> says this macro is primarily intended for use in new code. For
>> example:
> 
> Makes sense. Done.

Thanks for updating the patch!

> +       Current context id. Note that the context id is a temporary id 
> and may
> +       change in each invocation
> +      </para></entry>
> +     </row>

It clearly states that the context id is temporary, but I am a little 
concerned about users who write queries that refer to this view multiple 
times without using CTE.

If you agree, how about adding some description like below you mentioned 
before?

> We still need to use cte since ids are not persisted and might change 
> in
> each run of pg_backend_memory_contexts. Materializing the result can
> prevent any inconsistencies due to id change. Also it can be even good 
> for
> performance reasons as well.

We already have additional description below the table which explains 
each column of the system view. For example pg_locks:
https://www.postgresql.org/docs/devel/view-pg-locks.html


Also giving an example query something like this might be useful.

   -- show all the parent context names of ExecutorState
   with contexts as (
     select * from pg_backend_memory_contexts
   )
   select name from contexts where array[context_id] <@ (select path from 
contexts where name = 'ExecutorState');


-- 
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation

  13. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    Michael Paquier <michael@paquier.xyz> — 2024-02-14T07:23:38Z

    On Fri, Jan 19, 2024 at 05:41:45PM +0900, torikoshia wrote:
> We already have additional description below the table which explains each
> column of the system view. For example pg_locks:
> https://www.postgresql.org/docs/devel/view-pg-locks.html

I was reading the patch, and using int[] as a representation of the
path of context IDs up to the top-most parent looks a bit strange to
me, with the relationship between each parent -> child being
preserved, visibly, based on the order of the elements in this array
made of temporary IDs compiled on-the-fly during the function
execution.  Am I the only one finding that a bit strange?  Could it be
better to use a different data type for this path and perhaps switch
to the names of the contexts involved?

It is possible to retrieve this information some WITH RECURSIVE as
well, as mentioned upthread.  Perhaps we could consider documenting
these tricks?
--
Michael

  14. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    Melih Mutlu <m.melihmutlu@gmail.com> — 2024-04-03T13:20:39Z

    Hi,

Michael Paquier <michael@paquier.xyz>, 14 Şub 2024 Çar, 10:23 tarihinde
şunu yazdı:

> On Fri, Jan 19, 2024 at 05:41:45PM +0900, torikoshia wrote:
> > We already have additional description below the table which explains
> each
> > column of the system view. For example pg_locks:
> > https://www.postgresql.org/docs/devel/view-pg-locks.html
>
> I was reading the patch, and using int[] as a representation of the
> path of context IDs up to the top-most parent looks a bit strange to
> me, with the relationship between each parent -> child being
> preserved, visibly, based on the order of the elements in this array
> made of temporary IDs compiled on-the-fly during the function
> execution.  Am I the only one finding that a bit strange?  Could it be
> better to use a different data type for this path and perhaps switch
> to the names of the contexts involved?
>

Do you find having the path column strange all together? Or only using
temporary IDs to generate that column? The reason why I avoid using context
names is because there can be multiple contexts with the same name. This
makes it difficult to figure out which context, among those with that
particular name, is actually included in the path. I couldn't find any
other information that is unique to each context.

Thanks,
-- 
Melih Mutlu
Microsoft

  15. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    Andres Freund <andres@anarazel.de> — 2024-04-03T18:56:34Z

    Hi,

On 2024-02-14 16:23:38 +0900, Michael Paquier wrote:
> It is possible to retrieve this information some WITH RECURSIVE as well, as
> mentioned upthread.  Perhaps we could consider documenting these tricks?

I think it's sufficiently hard that it's not a reasonable way to do this.

Greetings,

Andres Freund

  16. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    Michael Paquier <michael@paquier.xyz> — 2024-04-03T23:34:04Z

    On Wed, Apr 03, 2024 at 04:20:39PM +0300, Melih Mutlu wrote:
> Michael Paquier <michael@paquier.xyz>, 14 Şub 2024 Çar, 10:23 tarihinde
> şunu yazdı:
>> I was reading the patch, and using int[] as a representation of the
>> path of context IDs up to the top-most parent looks a bit strange to
>> me, with the relationship between each parent -> child being
>> preserved, visibly, based on the order of the elements in this array
>> made of temporary IDs compiled on-the-fly during the function
>> execution.  Am I the only one finding that a bit strange?  Could it be
>> better to use a different data type for this path and perhaps switch
>> to the names of the contexts involved?
> 
> Do you find having the path column strange all together? Or only using
> temporary IDs to generate that column? The reason why I avoid using context
> names is because there can be multiple contexts with the same name. This
> makes it difficult to figure out which context, among those with that
> particular name, is actually included in the path. I couldn't find any
> other information that is unique to each context.

I've been re-reading the patch again to remember what this is about,
and I'm OK with having this "path" column in the catalog.  However,
I'm somewhat confused by the choice of having a temporary number that
shows up in the catalog representation, because this may not be
constant across multiple calls so this still requires a follow-up
temporary ID <-> name mapping in any SQL querying this catalog.  A
second thing is that array does not show the hierarchy of the path;
the patch relies on the order of the elements in the output array
instead.
--
Michael

  17. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    David Rowley <dgrowleyml@gmail.com> — 2024-04-04T01:44:27Z

    On Thu, 4 Apr 2024 at 12:34, Michael Paquier <michael@paquier.xyz> wrote:
> I've been re-reading the patch again to remember what this is about,
> and I'm OK with having this "path" column in the catalog.  However,
> I'm somewhat confused by the choice of having a temporary number that
> shows up in the catalog representation, because this may not be
> constant across multiple calls so this still requires a follow-up
> temporary ID <-> name mapping in any SQL querying this catalog.  A
> second thing is that array does not show the hierarchy of the path;
> the patch relies on the order of the elements in the output array
> instead.

My view on this is that there are a couple of things with the patch
which could be considered separately:

1. Should we have a context_id in the view?
2. Should we also have an array of all parents?

My view is that we really need #1 as there's currently no reliable way
to determine a context's parent as the names are not unique.   I do
see that Melih has mentioned this is temporary in:

+      <para>
+       Current context id. Note that the context id is a temporary id and may
+       change in each invocation
+      </para></entry>

For #2, I'm a bit less sure about this. I know Andres would like to
see this array added, but equally WITH RECURSIVE would work.  Does the
array of parents completely eliminate the need for recursive queries?
I think the array works for anything that requires all parents or some
fixed (would be) recursive level, but there might be some other
condition to stop recursion other than the recursion level that
someone needs to do.    What I'm trying to get at is; do we need to
document the WITH RECURSIVE stuff anyway? and if we do, is it still
worth having the parents array?

David

  18. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    Melih Mutlu <m.melihmutlu@gmail.com> — 2024-07-02T13:08:22Z

    Hi hackers,

David Rowley <dgrowleyml@gmail.com>, 4 Nis 2024 Per, 04:44 tarihinde şunu
yazdı:

> My view on this is that there are a couple of things with the patch
> which could be considered separately:
>
> 1. Should we have a context_id in the view?
> 2. Should we also have an array of all parents?
>

I discussed the above questions with David off-list, and decided to make
some changes in the patch as a result. I'd appreciate any input.

First of all, I agree that previous versions of the patch could make things
seem a bit more complicated than they should be, by having three new
columns (context_id, path, total_bytes_including_children). Especially when
we could already get the same result with several different ways (e.g.
writing a recursive query, using the patch column, and the
total_bytes_including_children column by itself help to know total used
bytes by a contexts and all of its children)

I believe that we really need to have context IDs as it's the only unique
way to identify a context. And I'm for having a parents array as it makes
things easier and demonstrates the parent/child relation explicitly. One
idea to simplify this patch a bit is adding the ID of a context into its
own path and removing the context_id column. As those IDs are temporary, I
don't think they would be useful other than using them to find some kind of
relation by looking into path values of some other rows. So maybe not
having a separate column for IDs but only having the path can help with the
confusion which this patch might introduce. The last element of the patch
would simply be the ID of that particular context.

One nice thing which David pointed out about paths is that level
information can become useful in those arrays. Level can represent the
position of a context in the path arrays of its child contexts. For
example; TopMemoryContext will always be the first element in all paths as
it's the top-most parent, it's also the only context with level 0. So this
relation between levels and indexes in path arrays can be somewhat useful
to link this array with the overall hierarchy of memory contexts.

An example query to get total used bytes including children by using level
info would look like:

WITH contexts AS (
SELECT * FROM pg_backend_memory_contexts
)
SELECT sum(total_bytes)
FROM contexts
WHERE path[( SELECT level+1 FROM contexts WHERE name =
'CacheMemoryContext')] =
(SELECT path[level+1] FROM contexts WHERE name = 'CacheMemoryContext');

Lastly, I created a separate patch to add total_bytes_including_children
columns. I understand that sum of total_bytes of a context and its children
will likely be one of the frequently used cases, not everyone may agree
with having an _including_children column for only total_bytes. I'm open to
hear more opinions on this.

Best Regards,
-- 
Melih Mutlu
Microsoft

  19. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    David Rowley <dgrowleyml@gmail.com> — 2024-07-05T08:06:27Z

    On Wed, 3 Jul 2024 at 01:08, Melih Mutlu <m.melihmutlu@gmail.com> wrote:
> An example query to get total used bytes including children by using level info would look like:
>
> WITH contexts AS (
> SELECT * FROM pg_backend_memory_contexts
> )
> SELECT sum(total_bytes)
> FROM contexts
> WHERE path[( SELECT level+1 FROM contexts WHERE name = 'CacheMemoryContext')] =
> (SELECT path[level+1] FROM contexts WHERE name = 'CacheMemoryContext');

I've been wondering about the order of the "path" column.  When we
talked, I had in mind that the TopMemoryContext should always be at
the end of the array rather than the start, but I see you've got it
the other way around.

With the order you have it, that query could be expressed as:

WITH c AS (SELECT * FROM pg_backend_memory_contexts)
SELECT c1.*
FROM c c1, c c2
WHERE c2.name = 'CacheMemoryContext'
AND c1.path[c2.level + 1] = c2.path[c2.level + 1];

Whereas, with the way I had in mind, it would need to look like:

WITH c AS (SELECT * FROM pg_backend_memory_contexts)
SELECT c1.*
FROM c c1, c c2
WHERE c2.name = 'CacheMemoryContext'
AND c1.path[c1.level - c2.level + 1] = c2.path[1];

I kind of think the latter makes more sense, as if for some reason you
know the level and context ID of the context you're looking up, you
can do:

SELECT * FROM pg_backend_memory_contexts WHERE path[<known level> +
level + 1] = <known context id>;

I also imagined "path" would be called "context_ids". I thought that
might better indicate what the column is without consulting the
documentation.

I think it might also be easier to document what context_ids is:

"Array of transient identifiers to describe the memory context
hierarchy. The first array element contains the ID for the current
context and each subsequent ID is the parent of the previous element.
Note that these IDs are unstable between multiple invocations of the
view.  See the example query below for advice on how to use this
column effectively."

There are also a couple of white space issues with the patch.  If
you're in a branch with the patch applied directly onto master, then
"git diff master --check" should show where they are.

If you do reverse the order of the "path" column, then I think
modifying convert_path_to_datum() is the best way to do that. If you
were to do it in the calling function, changing "path =
list_delete_last(path);" to use list_delete_first() is less efficient.

David

  20. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    Melih Mutlu <m.melihmutlu@gmail.com> — 2024-07-09T10:56:32Z

    Hi David,

David Rowley <dgrowleyml@gmail.com>, 5 Tem 2024 Cum, 11:06 tarihinde şunu
yazdı:

> With the order you have it, that query could be expressed as:
>
> WITH c AS (SELECT * FROM pg_backend_memory_contexts)
> SELECT c1.*
> FROM c c1, c c2
> WHERE c2.name = 'CacheMemoryContext'
> AND c1.path[c2.level + 1] = c2.path[c2.level + 1];
>
> Whereas, with the way I had in mind, it would need to look like:
>
> WITH c AS (SELECT * FROM pg_backend_memory_contexts)
> SELECT c1.*
> FROM c c1, c c2
> WHERE c2.name = 'CacheMemoryContext'
> AND c1.path[c1.level - c2.level + 1] = c2.path[1];
>
> I kind of think the latter makes more sense, as if for some reason you
> know the level and context ID of the context you're looking up, you
> can do:
>

I liked the fact that a context would always be at the same position,
level+1, in all context_ids arrays of its children. But what you described
makes sense as well, so I changed the order.

I also imagined "path" would be called "context_ids". I thought that
> might better indicate what the column is without consulting the
> documentation.
>

Done.



> I think it might also be easier to document what context_ids is:
>
> "Array of transient identifiers to describe the memory context
> hierarchy. The first array element contains the ID for the current
> context and each subsequent ID is the parent of the previous element.
> Note that these IDs are unstable between multiple invocations of the
> view.  See the example query below for advice on how to use this
> column effectively."
>

Done.



> There are also a couple of white space issues with the patch.  If
> you're in a branch with the patch applied directly onto master, then
> "git diff master --check" should show where they are.
>

Done.

Thanks,
-- 
Melih Mutlu
Microsoft

  21. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    Robert Haas <robertmhaas@gmail.com> — 2024-07-10T21:16:23Z

    On Wed, Apr 3, 2024 at 7:34 PM Michael Paquier <michael@paquier.xyz> wrote:
> I've been re-reading the patch again to remember what this is about,
> and I'm OK with having this "path" column in the catalog.  However,
> I'm somewhat confused by the choice of having a temporary number that
> shows up in the catalog representation, because this may not be
> constant across multiple calls so this still requires a follow-up
> temporary ID <-> name mapping in any SQL querying this catalog.  A
> second thing is that array does not show the hierarchy of the path;
> the patch relies on the order of the elements in the output array
> instead.

This complaint doesn't seem reasonable to me. The point of the path,
as I understand it, is to allow the caller to make sense of the
results of a single call, which is otherwise impossible. Stability
across multiple calls would be much more difficult, particularly
because we have no unique, long-lived identifier for memory contexts,
except perhaps the address of the context. Exposing the pointer
address of the memory contexts to clients would be an extremely bad
idea from a security point of view -- and it also seems unnecessary,
because the point of this function is to get a clear snapshot of
memory usage at a particular moment, not to track changes in usage by
the same contexts over time. You could still build the latter on top
of this if you wanted to do that, but I don't think most people would,
and I don't think the transient path IDs make it any more difficult.

I feel like Melih has chosen a simple and natural representation and I
would have done pretty much the same thing. And AFAICS there's no
reasonable alternative design.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  22. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    Robert Haas <robertmhaas@gmail.com> — 2024-07-10T21:19:00Z

    On Fri, Jul 5, 2024 at 4:06 AM David Rowley <dgrowleyml@gmail.com> wrote:
> I've been wondering about the order of the "path" column.  When we
> talked, I had in mind that the TopMemoryContext should always be at
> the end of the array rather than the start, but I see you've got it
> the other way around.

FWIW, I would have done what Melih did. A path normally is listed in
root-to-leaf order, not leaf-to-root.

> I also imagined "path" would be called "context_ids". I thought that
> might better indicate what the column is without consulting the
> documentation.

The only problem I see with this is that it doesn't make it clear that
we're being shown parentage or ancestry, rather than values for the
current node. I suspect path is fairly understandable, but if you
don't like that, what about parent_ids?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  23. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    David Rowley <dgrowleyml@gmail.com> — 2024-07-11T01:16:30Z

    On Thu, 11 Jul 2024 at 09:19, Robert Haas <robertmhaas@gmail.com> wrote:
> FWIW, I would have done what Melih did. A path normally is listed in
> root-to-leaf order, not leaf-to-root.

Melih and I talked about this in a meeting yesterday evening. I think
I'm about on the fence about having the IDs in leaf-to-root or
root-to-leaf.  My main concern about which order is chosen is around
how easy it is to write hierarchical queries. I think I'd feel better
about having it in root-to-leaf order if "level" was 1-based rather
than 0-based. That would allow querying CacheMemoryContext and all of
its descendants with:

WITH c AS (SELECT * FROM pg_backend_memory_contexts)
SELECT c1.*
FROM c c1, c c2
WHERE c2.name = 'CacheMemoryContext'
AND c1.path[c2.level] = c2.path[c2.level];

(With the v6 patch, you have to do level + 1.)

Ideally, no CTE would be needed here, but unfortunately, there's no
way to know the CacheMemoryContext's ID beforehand.  We could make the
ID more stable if we did a breadth-first traversal of the context.
i.e., assign IDs in level order.  This would stop TopMemoryContext's
2nd child getting a different ID if its first child became a parent
itself.

This allows easier ad-hoc queries, for example:

select * from pg_backend_memory_contexts;
-- Observe that CacheMemoryContext has ID=22 and level=2. Get the
total of that and all of its descendants.
select sum(total_bytes) from pg_backend_memory_contexts where path[2] = 22;
-- or just it and direct children
select sum(total_bytes) from pg_backend_memory_contexts where path[2]
= 22 and level <= 3;

Without the breadth-first assignment of context IDs, the sum() would
cause another context to be created for aggregation and the 2nd query
wouldn't work. Of course, it doesn't make it 100% guaranteed to be
stable, but it's way less annoying to write ad-hoc queries. It's more
stable the closer to the root you're interested in, which seems (to
me) the most likely area of interest for most people.

> On Fri, Jul 5, 2024 at 4:06 AM David Rowley <dgrowleyml@gmail.com> wrote:
> > I also imagined "path" would be called "context_ids". I thought that
> > might better indicate what the column is without consulting the
> > documentation.
>
> The only problem I see with this is that it doesn't make it clear that
> we're being shown parentage or ancestry, rather than values for the
> current node. I suspect path is fairly understandable, but if you
> don't like that, what about parent_ids?

I did a bit more work in the attached.  I changed "level" to be
1-based and because it's the column before "path" I find it much more
intuitive (assuming no prior knowledge) that the "path" column relates
to "level" somehow as it's easy to see that "level" is the same number
as the number of elements in "path". With 0-based levels, that's not
the case.

Please see the attached patch.  I didn't update any documentation.

David

  24. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    Robert Haas <robertmhaas@gmail.com> — 2024-07-11T20:09:31Z

    On Wed, Jul 10, 2024 at 9:16 PM David Rowley <dgrowleyml@gmail.com> wrote:
> Melih and I talked about this in a meeting yesterday evening. I think
> I'm about on the fence about having the IDs in leaf-to-root or
> root-to-leaf.  My main concern about which order is chosen is around
> how easy it is to write hierarchical queries. I think I'd feel better
> about having it in root-to-leaf order if "level" was 1-based rather
> than 0-based. That would allow querying CacheMemoryContext and all of
> its descendants with:
>
> WITH c AS (SELECT * FROM pg_backend_memory_contexts)
> SELECT c1.*
> FROM c c1, c c2
> WHERE c2.name = 'CacheMemoryContext'
> AND c1.path[c2.level] = c2.path[c2.level];

I don't object to making it 1-based.

> Ideally, no CTE would be needed here, but unfortunately, there's no
> way to know the CacheMemoryContext's ID beforehand.  We could make the
> ID more stable if we did a breadth-first traversal of the context.
> i.e., assign IDs in level order.  This would stop TopMemoryContext's
> 2nd child getting a different ID if its first child became a parent
> itself.

Do we ever have contexts with the same name at the same level? Could
we just make the path an array of strings, so that you could then say
something like this...

SELECT * FROM pg_backend_memory_contexts where path[2] = 'CacheMemoryContext'

...and get all the things with that in the path?

> select * from pg_backend_memory_contexts;
> -- Observe that CacheMemoryContext has ID=22 and level=2. Get the
> total of that and all of its descendants.
> select sum(total_bytes) from pg_backend_memory_contexts where path[2] = 22;
> -- or just it and direct children
> select sum(total_bytes) from pg_backend_memory_contexts where path[2]
> = 22 and level <= 3;

I'm doubtful about this because nothing prevents the set of memory
contexts from changing between one query and the next. We should try
to make it so that it's easy to get what you want in a single query.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  25. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    Melih Mutlu <m.melihmutlu@gmail.com> — 2024-07-12T22:11:52Z

    Hi David,

Thanks for v8 patch. Please see attached v9.

David Rowley <dgrowleyml@gmail.com>, 11 Tem 2024 Per, 04:16 tarihinde şunu
yazdı:

> I did a bit more work in the attached.  I changed "level" to be
> 1-based and because it's the column before "path" I find it much more
> intuitive (assuming no prior knowledge) that the "path" column relates
> to "level" somehow as it's easy to see that "level" is the same number
> as the number of elements in "path". With 0-based levels, that's not
> the case.
>
> Please see the attached patch.  I didn't update any documentation.


I updated documentation for path and level columns and also fixed the tests
as level starts from 1.

+ while (queue != NIL)
> + {
> + List *nextQueue = NIL;
> + ListCell *lc;
> +
> + foreach(lc, queue)
> + {


I don't think we need this outer while loop. Appending to the end of a
queue naturally results in top-to-bottom order anyway, keeping two lists,
"queue" and "nextQueue", might not be necessary. I believe that it's safe
to append to a list while iterating over that list in a foreach loop. v9
removes nextQueue and appends directly into queue.

Thanks,
-- 
Melih Mutlu
Microsoft

  26. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    Melih Mutlu <m.melihmutlu@gmail.com> — 2024-07-12T22:32:55Z

    Hi Robert,

Robert Haas <robertmhaas@gmail.com>, 11 Tem 2024 Per, 23:09 tarihinde şunu
yazdı:

> > Ideally, no CTE would be needed here, but unfortunately, there's no
> > way to know the CacheMemoryContext's ID beforehand.  We could make the
> > ID more stable if we did a breadth-first traversal of the context.
> > i.e., assign IDs in level order.  This would stop TopMemoryContext's
> > 2nd child getting a different ID if its first child became a parent
> > itself.
>
> Do we ever have contexts with the same name at the same level? Could
> we just make the path an array of strings, so that you could then say
> something like this...
>
> SELECT * FROM pg_backend_memory_contexts where path[2] =
> 'CacheMemoryContext'
>
> ...and get all the things with that in the path?
>

I just ran the below  to see if we have any context with the same level and
name.

postgres=# select level, name, count(*) from pg_backend_memory_contexts
group by level, name having count(*)>1;
 level |    name     | count
-------+-------------+-------
     3 | index info  |    90
     5 | ExprContext |     5

Seems like it's a possible case. But those contexts might not be the most
interesting ones. I guess the contexts that most users would be interested
in will likely be unique on their levels and with their name. So we might
not be concerned with the contexts, like those two from the above result,
and chose using names instead of transient IDs. But I think that we can't
guarantee name-based path column would be completely reliable in all cases.


> > select * from pg_backend_memory_contexts;
> > -- Observe that CacheMemoryContext has ID=22 and level=2. Get the
> > total of that and all of its descendants.
> > select sum(total_bytes) from pg_backend_memory_contexts where path[2] =
> 22;
> > -- or just it and direct children
> > select sum(total_bytes) from pg_backend_memory_contexts where path[2]
> > = 22 and level <= 3;
>
> I'm doubtful about this because nothing prevents the set of memory
> contexts from changing between one query and the next. We should try
> to make it so that it's easy to get what you want in a single query.
>

Correct. Nothing will not prevent contexts from changing between each
execution. With David's change to use breadth-first traversal, contexts at
upper levels are less likely to change. Knowing this may be useful in some
cases. IMHO there is no harm in making those IDs slightly more "stable",
even though there is no guarantee. My concern is whether we should document
this situation. If we should, how do we explain that the IDs are transient
and can change but also may not change if they're closer to
TopMemoryContext? If it's better not to mention this in the documentation,
does it really matter since most users would not be aware?


I've been also thinking if we should still have the parent column, as
finding out the parent is also possible via looking into the path. What do
you think?

Thanks,
-- 
Melih Mutlu
Microsoft

  27. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    David Rowley <dgrowleyml@gmail.com> — 2024-07-15T10:43:56Z

    On Fri, 12 Jul 2024 at 08:09, Robert Haas <robertmhaas@gmail.com> wrote:
> Do we ever have contexts with the same name at the same level? Could
> we just make the path an array of strings, so that you could then say
> something like this...
>
> SELECT * FROM pg_backend_memory_contexts where path[2] = 'CacheMemoryContext'
>
> ...and get all the things with that in the path?

Unfortunately, this wouldn't align with the goals of the patch. Going
back to Melih's opening paragraph in the initial email, he mentions
that there's currently no *reliable* way to determine the parent/child
relationship in this view.

There's been a few different approaches to making this reliable. The
first patch had "parent_id" and "id" columns.  That required a WITH
RECURSIVE query.  To get away from having to write such complex
queries, the "path" column was born.  I'm now trying to massage that
into something that's as easy to use and intuitive as possible. I've
gotta admit, I don't love the patch. That's not Melih's fault,
however. It's just the nature of what we're working with.

> I'm doubtful about this because nothing prevents the set of memory
> contexts from changing between one query and the next. We should try
> to make it so that it's easy to get what you want in a single query.

I don't think it's ideal that the context's ID changes in ad-hoc
queries, but I don't know how to make that foolproof.  The
breadth-first ID assignment helps, but it could certainly still catch
people out when the memory context of interest is nested at some deep
level.  The breadth-first certainly assignment helped me with the
CacheMemoryContext that I'd been testing with. It allowed me to run my
aggregate query to sum the bytes without the context created in
nodeAgg.c causing the IDs to change.

I'm open to better ideas on how to make this work, but it must meet
the spec of it being a reliable way to determine the context
relationship. If names were unique at each level having those instead
of IDs might be nice, but as Melih demonstrated, they're not. I think
even if Melih's query didn't return results, it would be a bad move to
make it work the way you mentioned if we have nothing to enforce the
uniqueness of names.

David

  28. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    David Rowley <dgrowleyml@gmail.com> — 2024-07-15T10:46:15Z

    On Sat, 13 Jul 2024 at 10:33, Melih Mutlu <m.melihmutlu@gmail.com> wrote:
> I've been also thinking if we should still have the parent column, as finding out the parent is also possible via looking into the path. What do you think?

I think we should probably consider removing it. Let's think about
that later. I don't think its existence is blocking us from
progressing here.

David

  29. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    David Rowley <dgrowleyml@gmail.com> — 2024-07-15T11:38:32Z

    On Sat, 13 Jul 2024 at 10:12, Melih Mutlu <m.melihmutlu@gmail.com> wrote:
> I updated documentation for path and level columns and also fixed the tests as level starts from 1.

Thanks for updating.

+   The <structfield>path</structfield> column can be useful to build
+   parent/child relation between memory contexts. For example, the following
+   query calculates the total number of bytes used by a memory context and its
+   child contexts:

"a memory context" doesn't quite sound specific enough. Let's say what
the query is doing exactly.

+<programlisting>
+WITH memory_contexts AS (
+    SELECT *
+    FROM pg_backend_memory_contexts
+)
+SELECT SUM(total_bytes)
+FROM memory_contexts
+WHERE ARRAY[(SELECT path[array_length(path, 1)] FROM memory_contexts
WHERE name = 'CacheMemoryContext')] &lt;@ path;

I don't think that example query is the most simple example. Isn't it
better to use the most simple form possible to express that?

I think it would be nice to give an example of using "level" as an
index into "path"

WITH c AS (SELECT * FROM pg_backend_memory_contexts)
SELECT sum(c1.total_bytes)
FROM c c1, c c2
WHERE c2.name = 'CacheMemoryContext'
AND c1.path[c2.level] = c2.path[c2.level];

I think the regression test query could be done using the same method.

>> + while (queue != NIL)
>> + {
>> + List *nextQueue = NIL;
>> + ListCell *lc;
>> +
>> + foreach(lc, queue)
>> + {
>
>
> I don't think we need this outer while loop. Appending to the end of a queue naturally results in top-to-bottom order anyway, keeping two lists, "queue" and "nextQueue", might not be necessary. I believe that it's safe to append to a list while iterating over that list in a foreach loop. v9 removes nextQueue and appends directly into queue.

The foreach() macro seems to be ok with that. I am too.  The following
comment will need to be updated:

+ /*
+ * Queue up all the child contexts of this level for the next
+ * iteration of the outer loop.
+ */

That outer loop is gone.

Also, this was due to my hasty writing of the patch. I named the
function get_memory_context_name_and_indent. I meant to write "ident".
If we did get rid of the "parent" column, I'd not see any need to keep
that function. The logic could just be put in
PutMemoryContextsStatsTupleStore(). I just did it that way to avoid
the repeat.

David

  30. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    Robert Haas <robertmhaas@gmail.com> — 2024-07-15T17:56:05Z

    On Fri, Jul 12, 2024 at 6:33 PM Melih Mutlu <m.melihmutlu@gmail.com> wrote:
> I just ran the below  to see if we have any context with the same level and name.
>
> postgres=# select level, name, count(*) from pg_backend_memory_contexts group by level, name having count(*)>1;
>  level |    name     | count
> -------+-------------+-------
>      3 | index info  |    90
>      5 | ExprContext |     5
>
> Seems like it's a possible case. But those contexts might not be the most interesting ones. I guess the contexts that most users would be interested in will likely be unique on their levels and with their name. So we might not be concerned with the contexts, like those two from the above result, and chose using names instead of transient IDs. But I think that we can't guarantee name-based path column would be completely reliable in all cases.

Maybe we should just fix it so that doesn't happen. I think it's only
an issue if the whole path is the same, and I'm not sure whether
that's the case here. But notice that we have this:

        const char *name;                       /* context name (just
for debugging) */
        const char *ident;                      /* context ID if any
(just for debugging) */

I think this arrangement dates to
442accc3fe0cd556de40d9d6c776449e82254763, and the discussion thread
begins like this:

"It does look like a 182KiB has been spent for some SQL, however
there's no clear way to tell which SQL is to blame."

So the point of that commit was to find better ways of distinguishing
between similar contexts. It sounds like perhaps we're not all the way
there yet, but if we agree on the goal, maybe we can figure out how to
reach it.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  31. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    Robert Haas <robertmhaas@gmail.com> — 2024-07-15T18:19:08Z

    On Mon, Jul 15, 2024 at 6:44 AM David Rowley <dgrowleyml@gmail.com> wrote:
> Unfortunately, this wouldn't align with the goals of the patch. Going
> back to Melih's opening paragraph in the initial email, he mentions
> that there's currently no *reliable* way to determine the parent/child
> relationship in this view.
>
> There's been a few different approaches to making this reliable. The
> first patch had "parent_id" and "id" columns.  That required a WITH
> RECURSIVE query.  To get away from having to write such complex
> queries, the "path" column was born.  I'm now trying to massage that
> into something that's as easy to use and intuitive as possible. I've
> gotta admit, I don't love the patch. That's not Melih's fault,
> however. It's just the nature of what we're working with.

I'm not against what you're trying to do here, but I feel like you
might be over-engineering it. I don't think there was anything really
wrong with what Melih was doing, and I don't think there's anything
really wrong with converting the path to an array of strings, either.
Sure, it might not be perfect, but future patches could always remove
the name duplication. This is a debugging facility that will be used
by a tiny minority of users, and if some non-uniqueness gets
reintroduced in the future, it's not a critical defect and can just be
fixed when it's noticed. That said, if you want to go with the integer
IDs and want to spend more time massaging it, I also think that's
fine. I simply don't believe it's the only way forward here. YMMV, but
my opinion is that none of these approaches have such critical flaws
that we need to get stressed about it.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  32. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    David Rowley <dgrowleyml@gmail.com> — 2024-07-16T00:21:46Z

    On Tue, 16 Jul 2024 at 06:19, Robert Haas <robertmhaas@gmail.com> wrote:
> I'm not against what you're trying to do here, but I feel like you
> might be over-engineering it. I don't think there was anything really
> wrong with what Melih was doing, and I don't think there's anything
> really wrong with converting the path to an array of strings, either.
> Sure, it might not be perfect, but future patches could always remove
> the name duplication. This is a debugging facility that will be used
> by a tiny minority of users, and if some non-uniqueness gets
> reintroduced in the future, it's not a critical defect and can just be
> fixed when it's noticed.

I'm just not on board with the
query-returns-correct-results-most-of-the-time attitude and I'm
surprised you are. You can get that today if you like, just write a
WITH RECURSIVE query joining "name" to "parent". If the consensus is
that's fine because it works most of the time, then I don't see any
reason to invent a new way to get equally correct-most-of-the-time
results.

> That said, if you want to go with the integer
> IDs and want to spend more time massaging it, I also think that's
> fine. I simply don't believe it's the only way forward here. YMMV, but
> my opinion is that none of these approaches have such critical flaws
> that we need to get stressed about it.

If there are other ways forward that match the goal of having a
reliable way to determine the parent of a MemoryContext, then I'm
interested in hearing more.  I know you've mentioned about having
unique names, but I don't know how to do that. Do you have any ideas
on how we could enforce the uniqueness? I don't really like your idea
of renaming contexts when we find duplicate names as bug fixes. The
nature of our code wouldn't make it easy to determine as some reusable
code might create a context as a child of CurrentMemoryContext and
multiple callers might call that code within a different
CurrentMemoryContext.

One problem is that, if you look at MemoryContextCreate(), we require
that the name is statically allocated. We don't have the flexibility
to assign unique names when we find a conflict.  If we were to come up
with a solution that assigned a unique name, then I'd call that
"over-engineered" for the use case we need it for. I think if we did
something like that, it would undo some of the work Tom did in
442accc3f. Also, I think it was you that came up with the idea of
MemoryContext reuse (9fa6f00b1)? Going by that commit message, it
seems to be done for performance reasons. If MemoryContext.name was
dynamic, there'd be more allocation work to do when reusing a context.
That might undo some of the performance gains seen in 9fa6f00b1. I
don't really want to go through the process of verifying there's no
performance regress for a patch that aims to make
pg_backend_memory_contexts more useful.

David

  33. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    Robert Haas <robertmhaas@gmail.com> — 2024-07-16T15:59:48Z

    On Mon, Jul 15, 2024 at 8:22 PM David Rowley <dgrowleyml@gmail.com> wrote:
> > That said, if you want to go with the integer
> > IDs and want to spend more time massaging it, I also think that's
> > fine. I simply don't believe it's the only way forward here. YMMV, but
> > my opinion is that none of these approaches have such critical flaws
> > that we need to get stressed about it.
>
> If there are other ways forward that match the goal of having a
> reliable way to determine the parent of a MemoryContext, then I'm
> interested in hearing more.  I know you've mentioned about having
> unique names, but I don't know how to do that. Do you have any ideas
> on how we could enforce the uniqueness? I don't really like your idea
> of renaming contexts when we find duplicate names as bug fixes. The
> nature of our code wouldn't make it easy to determine as some reusable
> code might create a context as a child of CurrentMemoryContext and
> multiple callers might call that code within a different
> CurrentMemoryContext.

I thought the reason that we have both 'name' and 'ident' was so that
the names could be compile-time constants and the ident values could
be strings, with the idea that we would choose the strings to be
something unique.

But I think I was wrong about that, because I see that for "index
info" contexts we just use the relation name and to have it actually
be unique we'd have to use something like schema_name.relation_name.
And even that wouldn't really work cleanly because the relation could
be renamed or moved to a different schema. Plus, adding string
construction overhead here sounds unappealing.

Maybe we'll find a clever solution someday, but I think for now you're
right that integer IDs are the way to go.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  34. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    Melih Mutlu <m.melihmutlu@gmail.com> — 2024-07-23T10:14:09Z

    Hi David,

David Rowley <dgrowleyml@gmail.com>, 15 Tem 2024 Pzt, 14:38 tarihinde şunu
yazdı:

> On Sat, 13 Jul 2024 at 10:12, Melih Mutlu <m.melihmutlu@gmail.com> wrote:
> > I updated documentation for path and level columns and also fixed the
> tests as level starts from 1.
>
> Thanks for updating.
>
> +   The <structfield>path</structfield> column can be useful to build
> +   parent/child relation between memory contexts. For example, the
> following
> +   query calculates the total number of bytes used by a memory context
> and its
> +   child contexts:
>
> "a memory context" doesn't quite sound specific enough. Let's say what
> the query is doing exactly.
>

Changed "a memory context" with "CacheMemoryContext".


> +<programlisting>
> +WITH memory_contexts AS (
> +    SELECT *
> +    FROM pg_backend_memory_contexts
> +)
> +SELECT SUM(total_bytes)
> +FROM memory_contexts
> +WHERE ARRAY[(SELECT path[array_length(path, 1)] FROM memory_contexts
> WHERE name = 'CacheMemoryContext')] &lt;@ path;
>
> I don't think that example query is the most simple example. Isn't it
> better to use the most simple form possible to express that?
>
> I think it would be nice to give an example of using "level" as an
> index into "path"
>
> WITH c AS (SELECT * FROM pg_backend_memory_contexts)
> SELECT sum(c1.total_bytes)
> FROM c c1, c c2
> WHERE c2.name = 'CacheMemoryContext'
> AND c1.path[c2.level] = c2.path[c2.level];
>

I changed the queries in the documentation and regression test to the ones
similar to the above query that you shared.


+ /*
> + * Queue up all the child contexts of this level for the next
> + * iteration of the outer loop.
> + */
>
> That outer loop is gone.
>

Removed that part.



> Also, this was due to my hasty writing of the patch. I named the
> function get_memory_context_name_and_indent. I meant to write "ident".
> If we did get rid of the "parent" column, I'd not see any need to keep
> that function. The logic could just be put in
> PutMemoryContextsStatsTupleStore(). I just did it that way to avoid
> the repeat.
>

Fixed the name. Also I needed to cast parameters when calling that function
as below to get rid of some warnings.

+       get_memory_context_name_and_ident(context,
+
(const char **)&name,
+
(const char **) &ident);

Thanks,
-- 
Melih Mutlu
Microsoft

  35. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    David Rowley <dgrowleyml@gmail.com> — 2024-07-24T09:47:14Z

    On Tue, 23 Jul 2024 at 22:14, Melih Mutlu <m.melihmutlu@gmail.com> wrote:
> Fixed the name. Also I needed to cast parameters when calling that function as below to get rid of some warnings.
>
> +       get_memory_context_name_and_ident(context,
> +                                                                         (const char **)&name,
> +                                                                         (const char **) &ident);

Thanks for fixing all those.

I've only had a quick look so far, but I think the patch is now in the
right shape. Unless there's some objections to how things are being
done in v10, I plan to commit this in the next few days... modulo any
minor adjustments.

David

  36. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    David Rowley <dgrowleyml@gmail.com> — 2024-07-25T03:05:44Z

    On Wed, 24 Jul 2024 at 21:47, David Rowley <dgrowleyml@gmail.com> wrote:
> I've only had a quick look so far, but I think the patch is now in the
> right shape. Unless there's some objections to how things are being
> done in v10, I plan to commit this in the next few days... modulo any
> minor adjustments.

I reviewed v10 today. I made some adjustments and pushed the result.

David

  37. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    David Rowley <dgrowleyml@gmail.com> — 2024-07-25T03:08:47Z

    On Tue, 23 Jul 2024 at 22:14, Melih Mutlu <m.melihmutlu@gmail.com> wrote:
> Fixed the name. Also I needed to cast parameters when calling that function as below to get rid of some warnings.
>
> +       get_memory_context_name_and_ident(context,
> +                                                                         (const char **)&name,
> +                                                                         (const char **) &ident);

I ended up fixing that another way as the above seems to be casting
away the const for those variables. Instead, I changed the signature
of the function to:

static void get_memory_context_name_and_ident(MemoryContext context,
const char **const name,  const char **const ident);

which I think takes into account for the call site variables being
defined as "const char *".

David

  38. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-28T16:31:27Z

    David Rowley <dgrowleyml@gmail.com> writes:
> I ended up fixing that another way as the above seems to be casting
> away the const for those variables. Instead, I changed the signature
> of the function to:
> static void get_memory_context_name_and_ident(MemoryContext context,
> const char **const name,  const char **const ident);
> which I think takes into account for the call site variables being
> defined as "const char *".

I did not check the history to see quite what happened here,
but Coverity thinks the end result is rather confused,
and I agree:

*** CID 1615190:  Null pointer dereferences  (REVERSE_INULL)
/srv/coverity/git/pgsql-git/postgresql/src/backend/utils/adt/mcxtfuncs.c: 58 in get_memory_context_name_and_ident()
52     	*ident = context->ident;
53     
54     	/*
55     	 * To be consistent with logging output, we label dynahash contexts with
56     	 * just the hash table name as with MemoryContextStatsPrint().
57     	 */
>>>     CID 1615190:  Null pointer dereferences  (REVERSE_INULL)
>>>     Null-checking "ident" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
58     	if (ident && strcmp(*name, "dynahash") == 0)
59     	{
60     		*name = *ident;
61     		*ident = NULL;
62     	}
63     }

It is not clear to me exactly which of these pointers should be
presumed to be possibly-null, but certainly testing ident after
storing through it is pretty pointless.  Maybe what was intended
was

-     	if (ident && strcmp(*name, "dynahash") == 0)
+     	if (*name && strcmp(*name, "dynahash") == 0)

?

			regards, tom lane

  39. Re: Parent/child context relation in pg_get_backend_memory_contexts()

    David Rowley <dgrowleyml@gmail.com> — 2024-07-28T21:19:51Z

    On Mon, 29 Jul 2024 at 04:31, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> It is not clear to me exactly which of these pointers should be
> presumed to be possibly-null, but certainly testing ident after
> storing through it is pretty pointless.  Maybe what was intended
> was
>
> -       if (ident && strcmp(*name, "dynahash") == 0)
> +       if (*name && strcmp(*name, "dynahash") == 0)

It should be *ident. I just missed adding the pointer dereference when
moving that code to a function.

Thanks for the report. I'll fix shortly.

David