Re: Enhancing Memory Context Statistics Reporting

Hi Daniel, Andres,


>
> > >> +} MemoryContextState;
> > >
> > > IMO that's too generic a name for something in a header.
> > >
> > >> +} MemoryContextId;
> > >
> > > This too.  Particularly because MemoryContextData->ident exist but is
> > > something different.
> >
> > Renamed both to use MemoryContextReporting* namespace, which leaves
> > MemoryContextReportingBackendState at an unwieldly long name.  I'm
> running out
> > of ideas on how to improve and it does make purpose quite explicit at
> least.
>
> How about
>
> MemoryContextReportingBackendState -> MemoryStatsBackendState
> MemoryContextReportingId -> MemoryStatsContextId
> MemoryContextReportingSharedState -> MemoryStatsCtl
> MemoryContextReportingStatsEntry -> MemoryStatsEntry
>
>
>
Fixed accordingly.


> > >> + /* context id starts with 1 */
> > >> + entry->context_id = ++(*stats_count);
> > >
> > > Given that we don't actually do anything here relating to starting
> with 1, I
> > > find that comment confusing.
> >
> > Reworded, not sure if it's much better tbh.
>
> I'd probably just remove the comment.
>
>
Reworded to mention that we pre-increment stats_count to make sure
id starts with 1.

>
> > > Hm. First I thought we'd leak memory if this second (and subsequent)
> > > dsa_allocate failed. Then I thought we'd be ok, because the memory
> would be
> > > memory because it'd be reachable from
> memCtxState[idx].memstats_dsa_pointer.
> > >
> > > But I think it wouldn't *quite* work, because
> memCtxState[idx].total_stats is
> > > only set *after* we would have failed.
> >
> > Keeping a running total in .total_stats should make the leak window
> smaller.
>
> Why not just initialize .total_stats *before* calling any fallible code?
> Afaict it's zero-allocated, so the free function should have no problem
> dealing with the entries that haven't yet been populated/
>
>
Fixed accordingly.

PFA a v28 which passes all local and github CI tests.

Thank you,
Rahila Syed