On 2024-01-16 18:41, Melih Mutlu wrote:
Hi,
Thanks for reviewing.
torikoshia <torikos...@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
