Re: shared-memory based stats collector

Hi,

On 2021-03-22 12:02:39 +0900, Kyotaro Horiguchi wrote:
> At Mon, 22 Mar 2021 09:55:59 +0900 (JST), Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> wrote in
> > At Thu, 18 Mar 2021 01:47:20 -0700, Andres Freund <andres(at)anarazel(dot)de> wrote in
> > > Hi,
> > >
> > > On 2021-03-18 16:56:02 +0900, Kyotaro Horiguchi wrote:
> > > > At Tue, 16 Mar 2021 10:27:55 +0900 (JST), Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> wrote in
> > > > Rebased and fixed two bugs. Not addressed received comments in this
> > > > version.
> > >
> > > Since I am heavily editing the code, could you submit "functional"
> > > changes (as opposed to fixing rebase issues) as incremental patches?
> >
> > Oh.. please wait for.. a moment, maybe.
>
> This is that.

Thanks! That change shouldn't be necessary on my branch - I did
something to fix this kind of problem too. I decided that there's no
point in doing hash table lookups for the database: It's not going to
change in the life of a backend. So there's now two static "pending"
entries: One for the current DB, one for the shared DB. There's only
one place that needed to change,
pgstat_report_checksum_failures_in_db(), which now reports the changes
directly instead of going via pending.

I suspect we should actually do that with a number of other DB specific
functions. Things like recovery conflicts, deadlocks, checksum failures
imo should really not be delayed till later. And you should never have
enough of them to make contention a concern.

You can see a somewhat sensible list of changes from your v52 at
https://github.com/anarazel/postgres/compare/master...shmstat-before-split-2021-03-22
(I did fix some of damage from rebase in a non-incremental way, of course)

My branch: https://github.com/anarazel/postgres/tree/shmstat

It would be cool if you could check if there any relevant things between
v52 and v56 that I should include.

I think a lot of the concerns I had with the patch are addressed at the
end of my series of changes. Please let me know what you think.

My next step is going to be to squash all my changes into the base
patch, and try to extract all the things that I think can be
independently committed, and to reduce unnecessary diff noise. Once
that's done I plan to post that series to the list.

TODO:

- explain the design at the top of pgstat.c

- figure out a way to deal with the different demands on stats
consistency / efficiency

- see how hard it'd be to not need collect_oids()

- split pgstat.c

- consider removing PgStatTypes and replacing it with the oid of the
table the type of stats reside in. So PGSTAT_TYPE_DB would be
DatabaseRelationId, PGSTAT_TYPE_TABLE would be RelationRelationId, ...

I think that'd make the system more cleanly extensible going forward?

- I'm not yet happy with the naming schemes in use in pgstat.c. I feel
like I improved it a bunch, but it's not yet there.

- the replication slot stuff isn't quite right in my branch

- I still don't quite like the reset_offset stuff - I wonder if we can
find something better there. And if not, whether we can deduplicate
the code between functions like pgstat_fetch_stat_checkpointer() and
pgstat_report_checkpointer().

At the very least it'll need a lot better comments.

- bunch of FIXMEs / XXXs

Greetings,

Andres Freund