Horton

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

Melanie Plageman <melanieplageman@gmail.com>

View thread View on postgr.es
From: Melanie Plageman <melanieplageman@gmail.com>
To: Andres Freund <andres@anarazel.de>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, Magnus Hagander <magnus@hagander.net>, Pg Hackers <pgsql-hackers@postgresql.org>, Lukas Fittl <lukas@fittl.com>, Thomas Munro <thomas.munro@gmail.com>
Date: 2021-08-11T22:00:40Z
Lists: pgsql-hackers

Attachments

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.

Commits

  1. Stabilize pg_stat_io writes test

  2. Fix flakey pg_stat_io test

  3. Suppress more compiler warnings in new pgstats code.

  4. Suppress compiler warnings in new pgstats code.

  5. Add tests for pg_stat_io

  6. Create regress_tblspc in test_setup

  7. Add pg_stat_io view, providing more detailed IO statistics

  8. pgstat: Track more detailed relation IO statistics

  9. pgstat: Infrastructure for more detailed IO statistics

  10. doc: Fix some issues in logical replication section

  11. Manual cleanup and pgindent of pgstat and bufmgr related code

  12. Have the planner consider Incremental Sort for DISTINCT

  13. Use actual backend IDs in pg_stat_get_backend_idset() and friends.

  14. Remove redundant call to pgstat_report_wal()

  15. Add BackendType for standalone backends

  16. Initialize backend status reporting during bootstrap.