Re: Parent/child context relation in pg_get_backend_memory_contexts()

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