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

On Fri, Jun 4, 2021 at 5:52 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-Apr-12, Melanie Plageman wrote:
>
> > As for the way I have recorded strategy writes -- it is quite inelegant,
> > but, I wanted to make sure that I only counted a strategy write as one
> > in which the backend wrote out the dirty buffer from its strategy ring
> > but did not check if there was any clean buffer in shared buffers more
> > generally (so, it is *potentially* an avoidable write). I'm not sure if
> > this distinction is useful to anyone. I haven't done enough with
> > BufferAccessStrategies to know what I'd want to know about them when
> > developing or using Postgres. However, if I don't need to be so careful,
> > it will make the code much simpler (though, I'm sure I can improve the
> > code regardless).
>
> I was bitten last year by REFRESH MATERIALIZED VIEW counting its writes
> via buffers_backend, and I was very surprised/confused about it.  So it
> seems definitely worthwhile to count writes via strategy separately.
> For a DBA tuning the server configuration it is very useful.
>
> The main thing is to *not* let these writes end up regular
> buffers_backend (or whatever you call these now).  I didn't read your
> patch, but the way you have described it seems okay to me.
>

Thanks for the feedback!

I agree it makes sense to count strategy writes separately.

I thought about this some more, and I don't know if it makes sense to
only count "avoidable" strategy writes.

This would mean that a backend writing out a buffer from the strategy
ring when no clean shared buffers (as well as no clean strategy buffers)
are available would not count that write as a strategy write (even
though it is writing out a buffer from its strategy ring). But, it
obviously doesn't make sense to count it as a regular buffer being
written out. So, I plan to change this code.

On another note, I've updated the patch with more correct concurrency
control control mechanisms (had some data races and other problems
before). Now, I am using atomics for the buffer action counters, though
the code includes several #TODO questions around the correctness of what
I have now too.

I also wrapped the buffer action types in a struct to make them easier
to work with.

The most substantial missing piece of the patch right now is persisting
the data across reboots.

The two places in the code I can see to persist the buffer action stats
data are:
1) using the stats collector code (like in
pgstat_read/write_statsfiles()
2) using a before_shmem_exit() hook which writes the data structure to a
file and then read from it when making the shared memory array initially

It feels a bit weird to me to wedge the buffer actions stats into the
stats collector code--since the stats collector isn't receiving and
aggregating the buffer action stats.

Also, I'm unsure how writing the buffer action stats out in
pgstat_write_statsfiles() will work, since I think that backends can
update their buffer action stats after we would have already persisted
the data from the BufferActionStatsArray -- causing us to lose those
updates.

And, I don't think I can use pgstat_read_statsfiles() since the
BufferActionStatsArray should have the data from the file as soon as the
view containing the buffer action stats can be queried. Thus, it seems
like I would need to read the file while initializing the array in
CreateBufferActionStatsCounters().

I am registering the patch for September commitfest but plan to update
the stats persistence before then (and docs, etc).

-- Melanie