On Wed, Jul 1, 2020 at 4:43 PM Fujii Masao <masao.fujii@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