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: Justin Pryzby <pryzby@telsasoft.com>, 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: 2022-08-11T23:53:09Z
Lists: pgsql-hackers

Attachments

I've attached v27 of the patch.

I've renamed IOPATH to IOCONTEXT. I also have added assertions to
confirm that unexpected statistics are not being accumulated.

There are also assorted other cleanups and changes.

It would be good to confirm that the rows being skipped and cells that
are NULL in the view are the correct ones.
The startup process will never use a BufferAccessStrategy, right?


On Wed, Jul 20, 2022 at 12:50 PM Andres Freund <andres@anarazel.de> wrote:

>
> > Subject: [PATCH v26 3/4] Track IO operation statistics
>
> > @@ -978,8 +979,17 @@ ReadBuffer_common(SMgrRelation smgr, char
> relpersistence, ForkNumber forkNum,
> >
> >       bufBlock = isLocalBuf ? LocalBufHdrGetBlock(bufHdr) :
> BufHdrGetBlock(bufHdr);
> >
> > +     if (isLocalBuf)
> > +             io_path = IOPATH_LOCAL;
> > +     else if (strategy != NULL)
> > +             io_path = IOPATH_STRATEGY;
> > +     else
> > +             io_path = IOPATH_SHARED;
>
> Seems a bit ugly to have an if (isLocalBuf) just after an isLocalBuf ?.
>

Changed this.


>
>
> > +                     /*
> > +                      * When a strategy is in use, reused buffers from
> the strategy ring will
> > +                      * be counted as allocations for the purposes of
> IO Operation statistics
> > +                      * tracking.
> > +                      *
> > +                      * However, even when a strategy is in use, if a
> new buffer must be
> > +                      * allocated from shared buffers and added to the
> ring, this is counted
> > +                      * as a IOPATH_SHARED allocation.
> > +                      */
>
> There's a bit too much duplication between the paragraphs...
>

I actually think the two paragraphs are making separate points. I've
edited this, so see if you like it better now.


>
> > @@ -628,6 +637,9 @@ pgstat_report_stat(bool force)
> >       /* flush database / relation / function / ... stats */
> >       partial_flush |= pgstat_flush_pending_entries(nowait);
> >
> > +     /* flush IO Operations stats */
> > +     partial_flush |= pgstat_flush_io_ops(nowait);
>
> Could you either add a note to the commit message that the stats file
> version needs to be increased, or just iclude that in the patch.
>
>
Bumped the stats file version in attached patchset.

- Melanie

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.