Re: shared-memory based stats collector

At Sat, 13 Mar 2021 10:05:21 -0800, Andres Freund <andres(at)anarazel(dot)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 <andres(at)anarazel(dot)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