At Sat, 13 Mar 2021 10:05:21 -0800, Andres Freund <and...@anarazel.de> wrote in > 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.
Previously the patch split pgstat.c into pgstat.c and besomething.c but that was rejected for large footprint. Of course I happily agree to split (a,b) and c. And also agree to split out d and e. > 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. That sounds reasonable. (I did split the header files at the time.) > > 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. FWIW, agreed, too. > Cool. I'll give it a try. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
