Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)

On Tue, Sep 14, 2021 at 9:30 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-Sep-13, Melanie Plageman wrote:
>
> > I also think it makes sense to rename the pg_stat_buffer_actions view to
> > pg_stat_buffers and to name the columns using both the buffer action
> > type and buffer type -- e.g. shared, strategy, local. This leaves open
> > the possibility of counting buffer actions done on other non-shared
> > buffers -- like those done while building indexes or those using local
> > buffers. The third patch in the set does this (I wanted to see if it
> > made sense before fixing it up into the first patch in the set).
>
> What do you think of the idea of having the "shared/strategy/local"
> attribute be a column?  So you'd have up to three rows per buffer action
> type.  Users wishing to see an aggregate can just aggregate them, just
> like they'd do with pg_buffercache.  I think that leads to an easy
> decision with regards to this point:

I have rewritten the code to implement this.

>
>
> (It's weird to have enum values that are there just to indicate what's
> the maximum value.  I think that sort of thing is better done by having
> a "#define LAST_THING" that takes the last valid value from the enum.
> That would free you from having to handle the last value in switch
> blocks, for example.  LAST_OCLASS in dependency.h is a precedent on this.)
>

I have made this change.

The attached v8 patchset is rewritten to add in an additional dimension
-- buffer type. Now, a backend keeps track of how many buffers of a
particular type (e.g. shared, local) it has accessed in a particular way
(e.g. alloc, write). It also changes the naming of various structures
and the view members.

Previously, stats reset did not work since it did not consider live
backends' counters. Now, the reset message includes the current live
backends' counters to be tracked by the stats collector and used when
the view is queried.

The reset message is one of the areas in which I still need to do some
work -- I shoved the array of PgBufferAccesses into the existing reset
message used for checkpointer, bgwriter, etc. Before making a new type
of message, I would like feedback from a reviewer about the approach.

There are various TODOs in the code which are actually questions for the
reviewer. Once I have some feedback, it will be easier to address these
items.

There a few other items which may be material for other commits that
I would also like to do:
1) write wrapper functions for smgr* functions which count buffer
accesses of the appropriate type. I wasn't sure if these should
literally just take all the parameters that the smgr* functions take +
buffer type. Once these exist, there will be less possibility for
regressions in which new code is added using smgr* functions without
counting this buffer activity. Once I add these, I was going to go
through and replace existing calls to smgr* functions and thereby start
counting currently uncounted buffer type accesses (direct, local, etc).

2) Separate checkpointer and bgwriter into two views and add additional
stats to the bgwriter view.

3) Consider adding a helper function to pgstatfuncs.c to help create the
tuplestore. These functions all have quite a few lines which are exactly
the same, and I thought it might be nice to do something about that:
  pg_stat_get_progress_info(PG_FUNCTION_ARGS)
  pg_stat_get_activity(PG_FUNCTION_ARGS)
  pg_stat_get_buffers_accesses(PG_FUNCTION_ARGS)
  pg_stat_get_slru(PG_FUNCTION_ARGS)
  pg_stat_get_progress_info(PG_FUNCTION_ARGS)
I can imagine a function that takes a Datums array, a nulls array, and a
ResultSetInfo and then makes the tuplestore -- though I think that will
use more memory. Perhaps we could make a macro which does the initial
error checking (checking if caller supports returning a tuplestore)? I'm
not sure if there is something meaningful here, but I thought I would
ask.

Finally, I haven't removed the test in pg_stats and haven't done a final
pass for comment clarity, alphabetization, etc on this version.

- Melanie