Re: shared-memory based stats collector

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.

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