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