At Fri, 26 Mar 2021 13:17:21 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
> At Fri, 26 Mar 2021 12:49:16 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in
> > On 2021/03/26 12:26, Fujii Masao wrote:
> > > On 2021/03/26 12:01, Kyotaro Horiguchi wrote:
> > >> At Thu, 25 Mar 2021 23:45:17 +0900, torikoshia
> > >> <torikoshia@oss.nttdata.com> wrote in
> > >>> Attached new one.
> > > Regarding the argument max_children, isn't it better to set its
> > > default value,
> > > e.g., 100 like MemoryContextStats() uses?
> >
> > + mcxtLogData->maxChildrenPerContext = max_children;
> > +
> > + if(!SendProcSignal(pid, PROCSIG_LOG_MEMORY_CONTEXT,
> > proc->backendId))
> > + PG_RETURN_BOOL(true);
> >
> > Do we need memory barrier here? Otherwise, there is a race condition
> > where maxChildrenPerContext has not been set yet when the target
> > backend
> > that received signal read that shared variable. No?
> >
> > + Note that when multiple
> > + <function>pg_get_backend_memory_contexts</function> called in
> > + succession or simultaneously, <parameter>max_children</parameter>
> > can
> > + be the one of another
> > + <function>pg_get_backend_memory_contexts</function>.
> > + </para></entry>
> >
> > This might not be an issue in practice as Horiguchi-san said upthread.
> > But this looks a bit ugly and might be footgun later. The current
> > approach
> > using shared memory to pass this information to the target backends
> > might be overkill, and too complicated to polish the design at the
> > stage
> > just before feature freeze. So I'd withdraw my previous comment and
> > am OK to use the hard-coded value as max_children at the first version
> > of this feature. Thought?
>
> The dumper function silently suppresses logging when there are too
> many children. Suppressing a part of output when the user didn't told
> anything looks like just a misbehavior (even if it is written in the
> documentation..), especially when the suppressed contexts occupy the
> majority of memory usage. I think the fixed-limit of lines works if
> the lines are in descending order of memory usage.
>
> At least we need an additional line to inform the suppression.
> "some contexts are omitted"
> "n child contexts: total_bytes = ..."
Sorry I missed that is already implemented. So my opnion is I agree
with limiting with a fixed-number, and preferablly sorted in
descending order of... totalspace/nblocks?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
