Re: Enhancing Memory Context Statistics Reporting

Hi,

On 2025-04-08 01:17:17 +0200, Daniel Gustafsson wrote:
> > On 7 Apr 2025, at 17:43, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> >> + /*
> >> + * Hold the process lock to protect writes to process specific memory. Two
> >> + * processes publishing statistics do not block each other.
> >> + */
> >
> > s/specific/process specific/
>
> That's what it says though.. isn't it? I might be missing something obvious.

Understandable confusion, not sure what my brain was doing anymore
either...

> >> +} 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

> >> + /* 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.

> > 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/

Greetings,

Andres Freund