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

On Wed, Aug 11, 2021 at 4:11 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> On Tue, Aug 3, 2021 at 2:13 PM Andres Freund <andres@anarazel.de> wrote:
> >
> > > @@ -2895,6 +2948,20 @@ FlushBuffer(BufferDesc *buf, SMgrRelation reln)
> > >       /*
> > >        * bufToWrite is either the shared buffer or a copy, as appropriate.
> > >        */
> > > +
> > > +     /*
> > > +      * TODO: consider that if we did not need to distinguish between a buffer
> > > +      * flushed that was grabbed from the ring buffer and written out as part
> > > +      * of a strategy which was not from main Shared Buffers (and thus
> > > +      * preventable by bgwriter or checkpointer), then we could move all calls
> > > +      * to pgstat_increment_buffer_action() here except for the one for
> > > +      * extends, which would remain in ReadBuffer_common() before smgrextend()
> > > +      * (unless we decide to start counting other extends). That includes the
> > > +      * call to count buffers written by bgwriter and checkpointer which go
> > > +      * through FlushBuffer() but not BufferAlloc(). That would make it
> > > +      * simpler. Perhaps instead we can find somewhere else to indicate that
> > > +      * the buffer is from the ring of buffers to reuse.
> > > +      */
> > >       smgrwrite(reln,
> > >                         buf->tag.forkNum,
> > >                         buf->tag.blockNum,
> >
> > Can we just add a parameter to FlushBuffer indicating what the source of the
> > write is?
> >
>
> I just noticed this comment now, so I'll address that in the next
> version. I rebased today and noticed merge conflicts, so, it looks like
> v5 will be on its way soon anyway.
>

Actually, after moving the code around like you suggested, calling
pgstat_increment_buffer_action() before smgrwrite() in FlushBuffer() and
using a parameter to indicate if it is a strategy write or not would
only save us one other call to pgstat_increment_buffer_action() -- the
one in SyncOneBuffer(). We would end up moving the one in BufferAlloc()
to FlushBuffer() and removing the one in SyncOneBuffer().
Do you think it is still worth it?

Rebased v5 attached.