Hi, On 2021-03-13 12:53:30 +0100, Magnus Hagander wrote: > On Sat, Mar 13, 2021 at 7:20 AM Andres Freund <and...@anarazel.de> wrote: > > I think before making things differently complicated with this patch, > > we need to clean this up, unfortunately. I think we should initially have > > - src/backend/postmaster/pgstat.c, for a), b) above > > - src/backend/utils/activity/backend_status.c for c) > > - src/backend/utils/activity/wait_events.c for d) > > - src/backend/utils/activity/progress.c for e) > > > > Not at all sure about the names, but something roughly like this > > would im make sense. > > +1, definitely.
Cool. I think I can introduce them without causing too much breakage in the shmstats patch. Without yet having done it, I think I'd not touch the function name prefixes during the move and leave the prototypes in the pgstat.h header? Then we could split the header off in a second step (with backward compat includes in pgstat.h), potentially combined with a rename? But I'm happy to consider different approaches. > I've been thinking in these lines more than once when poking at > differen patches around that area, but they've never been big enough > to justify the restructuring on their own. Which then of course just > helps accumulate the problem... Yea... > If anything, I'd even consider splitting (a) and (b) above out into > separate ones as well. But hey, I see you got to that in your next > paragraph :) I wondered about doing that as a preceding step as well, but it seems a bit pointless if all that code needs to be moved elsewhere, and little of it survives unchanged... > > The next thing to note is that after this whole patchseries, having the > > remaining functionality in src/backend/postmaster/pgstat.c doesn't make > > sense. The things in postmaster/ are related to postmaster > > sub-processes, not random pieces of backend infrastructure. Therefore I > > am thinking that patch 0004 should be changed so it basically adds all > > the changed code to two new files: > > - src/backend/utils/activity/stats.c - for the stats keeping > > infrastructure (shared memory hash table, pending table, etc) > > - src/backend/utils/activity/stats_report.c - for pgstat_report_*, > > pgstat_count_*, pgstat_update_*, flush_*, i.e. everything that knows > > about specific kinds of stats. > > > > The reason for that split is that imo the two pieces of code are largely > > independent. One shouldn't need to understand the way stats are stored > > in any sort of detail to add a new stats field and vice versa. > > Agreed as well. Cool. I'll give it a try. Greetings, Andres Freund
