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
v28-0001-Add-function-to-get-memory-context-stats-for-process.patch
Description: Binary data
