Re: Parent/child context relation in pg_get_backend_memory_contexts()

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