Re: shared-memory based stats collector

Hi,

On 2021-03-16 15:08:39 -0400, Robert Haas wrote:
> On Mon, Mar 15, 2021 at 10:56 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > I did roughly the first steps of the split as I had outlined. I moved:
> >
> > 1) wait event related functions into utils/activity/wait_event.c /
> > wait_event.h
> >
> > 2) "backend status" functionality (PgBackendStatus stuff) into
> > utils/activity/backend_status.c
> >
> > 3) "progress" related functionality into
> > utils/activity/backend_progress.c
>
> In general, I like this. I'm not too sure about the names. I realize
> you don't want to have functions called status.c and progress.c,
> because that's awful generic, but now you have backend_progress.c,
> backend_status.c, and wait_event.c, which makes the last one look a
> little strange. Maybe command_progress.c instead of
> backend_progress.c?

I'm not thrilled about the names I ended up with either, so happy to get
some ideas.

I did consider command_progress.c too - but that seems confusing because
there's src/include/commands/progress.h, which is imo a different layer
than what pgstat/backend_progress provide. So I thought splitting things
up so that backend_progress.[ch] provide the place to store the progress
values, and commands/progress.h defining the meaning of the values as
used for in-core postgres commands would make sense. I could see us
using the general progress infrastructure for things that'd not fit
super well into commands/* at some point...

But I'd also be ok with folding in command/progress.h.

> > I think 1 and 2 are good (albeit in need of further polish). I'm a bit
> > less sure about 3:
> > - There's dependency from backend_status.h to backend_progress.h,
> > because it PGSTAT_NUM_PROGRESS_PARAM etc.
>
> That doesn't seem like a catastrophe.
>
> > - it's a fairly small amount of code
>
> But it's not bad to have it separate.

Agreed.

> > - I'm inclined to leave pgstat_report_{activity, tmpfile, appname,
> > timestamp, ..} alone naming-wise, but to rename pgstat_bestart() to
> > something like pgbestat_start()?
>
> I'd probably rename them e.g. command_progress_start(),
> comand_progress_update_param(), etc.

Hm. There's ~250 calls to pgstat_report_*. Which are you proposing to
rename? In my mind there's at least the following groups with
"inaccurately" overlapping names:

1) "stats reporting" functions, like pgstat_report_{bgwriter,
archiver,...}(), pgstat_count_*(), pgstat_{init,
end}_function_usage(),

2) "backend activity" functions, like pgstat_report_activity()

2.1) "wait event" functions, like pgstat_report_wait_{start,end}()

3) "stats control" functions, like pgstat_report_stat()

4) "stats reporting" fetch functions like pgstat_fetch_stat_dbentry()

5) "backend activity" fetch functions like
pgstat_fetch_stat_numbackends(), pgstat_fetch_stat_beentry()

I'd not quite group the progress functions as part of that, because they
do already have a distinct namespace, even though perhaps pgstat_* isn't
a great prefix.

> > - backend_status.h needs miscadmin.h, due to BackendType. Imo that's a
> > header we should try to avoid exposing in headers if possible. But
> > right now there's no good place to move BackendType to. So I'd let
> > this slide for now.
>
> Why the concern? miscadmin.h is extremely widely-included already.

In .c files, but not from a lot of headers... There's only two places,
pgstat.h and regex/regcustom.h.

For me it a weird mix of things that should be included from very few
places, and super widely used stuff. I already feel bad including it
from .c files, but indirect includes in .h files imo should be as
narrow as possible.

> Maybe it should be broken up into pieces so that we're not including
> so MUCH stuff in a zillion places, but the header that contains the
> definition of CHECK_FOR_INTERRUPTS() is always going to be needed in a
> ton of spots. Honestly, I wonder why we don't just put that part in
> postgres.h.

I'd not be against that. I'd personally put CFI() in a separate header,
but include it from postgres.h. I don't think there's much else in
there that should be as widely used. The closest is INTERRUPTS and
CRIT_SECTION stuff, but that should be less frequent.

Greetings,

Andres Freund