Re: Parent/child context relation in pg_get_backend_memory_contexts()

Hi,

Thanks for reviewing.
Attached the updated patch v3.

Andres Freund <andres(at)anarazel(dot)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(at)snowman(dot)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