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: Tom Lane <tgl@sss.pgh.pa.us>, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, vignesh21@gmail.com, pryzby@telsasoft.com, lukas@fittl.com, alvherre@alvh.no-ip.org, magnus@hagander.net, pgsql-hackers@postgresql.org, thomas.munro@gmail.com, m.sakrejda@gmail.com
Date: 2023-02-26T21:24:40Z
Lists: pgsql-hackers 
On Sun, Feb 26, 2023 at 04:11:45PM -0500, Melanie Plageman wrote:
> On Sun, Feb 26, 2023 at 12:33:03PM -0800, Andres Freund wrote:
> > On 2023-02-26 15:08:33 -0500, Tom Lane wrote:
> > > Andres Freund <andres@anarazel.de> writes:
> > > > They're all animals for testing older LLVM versions. They're using
> > > > pretty old clang versions. phycodurus and dragonet are clang 3.9, petalura and
> > > > desmoxytes is clang 4, idiacanthus and pogona are clang 5.
> > >
> > > [ shrug ... ]  If I thought this was actually good code, I might
> > > agree with ignoring these warnings; but I think what it mostly is
> > > is misleading overcomplication.
> >
> > I don't mind removing *_FIRST et al by using 0. None of the proposals for
> > getting rid of *_NUM_* seemed a cure actually better than the disease.
> 
> I am also fine with removing *_FIRST and allowing those electrons to
> move on to bigger and better things :)
> 
> >
> > Adding a cast to int of the loop iteration variable seems to work and only
> > noticeably, not untollerably, ugly.
> >
> > One thing that's odd is that the warnings don't appear reliably. The
> > "io_op < IOOP_NUM_TYPES" comparison in pgstatfuncs.c doesn't trigger any
> > with clang-4.
> 
> Using an int and casting all over the place certainly doesn't make the
> code more attractive, but I am fine with this if it seems like the least
> bad solution.
> 
> I didn't want to write a patch with this (ints instead of enums as loop
> control variable) without being able to reproduce the warnings myself
> and confirm the patch silences them. However, I wasn't able to reproduce
> the warnings myself. I tried to do so with a minimal repro on godbolt,
> and even with
> -Wtautological-constant-out-of-range-compare -Wall -Wextra -Weverything -Werror
> I couldn't get clang 4 or 5 (or a number of other compilers I randomly
> picked from the dropdown) to produce the warnings.

Just kidding: it reproduces if the defined enum has two or less values.
Interesting...

After discovering this, tried out various solutions including one Andres
suggested:

	for (IOOp io_op = 0; (int) io_op < IOOP_NUM_TYPES; io_op++)

and it does silence the warning. What do you think?

- 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.