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.
+/* ----------
+ * 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..
I'm going to follow the discussion on the mailing list and find why
these codes were introduced.
If there's no important reason to do the same in our context, I'll
change them.
Regards,
--
Atsushi Torikoshi
NTT DATA CORPORATION
