On 2020/07/01 22:15, torikoshia wrote:
On Wed, Jul 1, 2020 at 4:43 PM Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
Thanks for reviewing!
You treat pg_stat_local_memory_contexts view as a dynamic statistics view.
But isn't it better to treat it as just system view like pg_shmem_allocations
or pg_prepared_statements because it's not statistics information? If yes,
we would need to rename the view, move the documentation from
monitoring.sgml to catalogs.sgml, and move the code from pgstat.c to
the more appropriate source file.
Agreed.
At first, I thought not only statistical but dynamic information about exactly
what is going on was OK when reading the sentence on the manual below.
PostgreSQL also supports reporting dynamic information about exactly what is
going on in the system right now, such as the exact command currently being
executed by other server processes, and which other connections exist in the
system. This facility is independent of the collector process.
However, now I feel something strange because existing pg_stats_* views seem
to be per cluster information but the view I'm adding is about per backend
information.
I'm going to do some renaming and transportations.
- view name: pg_memory_contexts
- function name: pg_get_memory_contexts()
- source file: mainly src/backend/utils/mmgr/mcxt.c
+ tupdesc = rsinfo->setDesc;
+ tupstore = rsinfo->setResult;
These seem not to be necessary.
Thanks!
+ /*
+ * It seems preferable to label dynahash contexts with just the hash
table
+ * name. Those are already unique enough, so the "dynahash" part isn't
+ * very helpful, and this way is more consistent with pre-v11 practice.
+ */
+ if (ident && strcmp(name, "dynahash") == 0)
+ {
+ name = ident;
+ ident = NULL;
+ }
IMO it seems better to report both name and ident even in the case of
dynahash than report only ident (as name). We can easily understand
the context is used for dynahash when it's reported. But you think it's
better to report NULL rather than "dynahash"?
These codes come from MemoryContextStatsPrint() and my intension is to
keep consistent with it.
Ok, understood! I agree that it's strange to display different names
for the same memory context between this view and logging.
It's helpful if the comment there refers to MemoryContextStatsPrint()
and mentions the reason that you told.
+/* ----------
+ * The max bytes for showing identifiers of MemoryContext.
+ * ----------
+ */
+#define MEMORY_CONTEXT_IDENT_SIZE 1024
Do we really need this upper size limit? Could you tell me why
this is necessary?
It also derived from MemoryContextStatsPrint().
I suppose it is for keeping readability of the log..
Understood. I may want to change the upper limit of query size to display.
But at the first step, I'm fine with limitting 1024 bytes.
I'm going to follow the discussion on the mailing list and find why
these codes were introduced.
https://www.postgresql.org/message-id/12319.1521999065%40sss.pgh.pa.us
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
