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

On Thu, Apr 15, 2021 at 7:59 PM Andres Freund <andres(at)anarazel(dot)de> wrote:

> Hi,
>
> On 2021-04-12 19:49:36 -0700, Melanie Plageman wrote:
> > So, I took a stab at implementing this in PgBackendStatus.
>
> Cool!
>

Just a note on v2 of the patch -- the diff for the changes I made to
pgstatfuncs.c is pretty atrocious and hard to read. I tried using a
different diff algorithm, to no avail.

>
>
> > The attached patch is not quite on top of current master, so, alas,
> > don't try and apply it. I went to rebase today and realized I needed
> > to make some changes in light of e1025044cd4, however, I wanted to
> > share this WIP so that I could pose a few questions that I imagine
> > will still be relevant after I rewrite the patch.
>

Regarding the refactor done in e1025044cd4:
Most of the functions I've added access variables in PgBackendStatus, so
I put most of them in backend_status.h/c. However, technically, these
are stats which are aggregated over time, which e1025044cd4 says should
go in pgstat.c/h. I could move some of it, but I hadn't tried to do so,
as it made a few things inconvenient, and, I wasn't sure if it was the
right thing to do anyway.

> >
> > I removed buffers_backend and buffers_backend_fsync from
> > pg_stat_bgwriter and have created a new view which tracks
> > - number of shared buffers the checkpointer and bgwriter write out
> > - number of shared buffers a regular backend is forced to flush
> > - number of extends done by a regular backend through shared buffers
> > - number of buffers flushed by a backend or autovacuum using a
> > BufferAccessStrategy which, were they not to use this strategy,
> > could perhaps have been avoided if a clean shared buffer was
> > available
> > - number of fsyncs done by a backend which could have been done by
> > checkpointer if sync queue had not been full
>
> I wonder if leaving buffers_alloc in pg_stat_bgwriter makes sense after
> this? I'm tempted to move that to pg_stat_buffers or such...
>
>
I've gone ahead and moved buffers_alloc out of pg_stat_bgwriter and into
pg_stat_buffer_actions (I've renamed it from pg_stat_buffers_written).

> I'm not quite convinced by having separate columns for checkpointer,
> bgwriter, etc. That doesn't seem to scale all that well. What if we
> instead made it a view that has one row for each BackendType?
>
>
I've changed the view to have one row for each backend type for which we
would like to report stats and one column for each buffer action type.

To make the code easier to write, I record buffer actions for all
backend types -- even if we don't have any buffer actions we care about
for that backend type. I thought it was okay because when I actually
aggregate the counters across backends, I only do so for the backend
types we care about -- thus there shouldn't be much accessing of shared
memory by multiple different processes.

Also, I copy-pasted most of the code in pg_stat_get_buffer_actions() to
set up the result tuplestore from pg_stat_get_activity() without totally
understanding all the parts of it, so I'm not sure if all of it is
required here.

>
> > In implementing this counting per backend, it is easy for all types of
> > backends to keep track of the number of writes, extends, fsyncs, and
> > strategy writes they are doing. So, as recommended upthread, I have
> > added columns in the view for the number of writes for checkpointer and
> > bgwriter and others. Thus, this view becomes more than just stats on
> > "avoidable I/O done by backends".
> >
> > So, my question is, does it makes sense to track all extends -- those to
> > extend the fsm and visimap and when making a new relation or index? Is
> > that information useful? If so, is it different than the extends done
> > through shared buffers? Should it be tracked separately?
>
> I don't fully understand what you mean with "extends done through shared
> buffers"?
>
>
By "extends done through shared buffers", I just mean when an extend of
a relation is done and the data that will be written to the new block is
written into a shared buffer (as opposed to a local one or local memory
or a strategy buffer).

Random note:
I added a length member to the BackendType enum (BACKEND_NUM_TYPES),
which led to this compiler warning:

miscinit.c: In function ‘GetBackendTypeDesc’:
miscinit.c:236:2: warning: enumeration value ‘BACKEND_NUM_TYPES’ not
handled in switch [-Wswitch]
236 | switch (backendType)
| ^~~~~~

I tried using pg_attribute_unused() for BACKEND_NUM_TYPES, but, it
didn't seem to have the desired effect. As such, I just threw a case
into GetBackendTypeDesc() which does nothing (as opposed to erroring
out), since the backendDesc already is initialized to "unknown process
type", erroring out doesn't seem to be expected.

- Melanie