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: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andres Freund <andres@anarazel.de>, 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-27T19:03:16Z
Lists: pgsql-hackers

Attachments

On Mon, Feb 27, 2023 at 10:30 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Melanie Plageman <melanieplageman@gmail.com> writes:
> > Attached is a patch to remove the *_FIRST macros.
> > I was going to add in code to change
>
> >     for (IOObject io_object = 0; io_object < IOOBJECT_NUM_TYPES; io_object++)
> >     to
> >     for (IOObject io_object = 0; (int) io_object < IOOBJECT_NUM_TYPES; io_object++)
>
> I don't really like that proposal.  ISTM it's just silencing the
> messenger rather than addressing the underlying problem, namely that
> there's no guarantee that an IOObject variable can hold the value
> IOOBJECT_NUM_TYPES, which it had better do if you want the loop to
> terminate.  Admittedly it's quite unlikely that these three enums would
> grow to the point that that becomes an actual hazard for them --- but
> IMO it's still bad practice and a bad precedent for future code.

That's fair. Patch attached.

> > but then I couldn't remember why we didn't just do
>
> >     for (int io_object = 0; io_object < IOOBJECT_NUM_TYPES; io_object++)
>
> > I recall that when passing that loop variable into a function I was
> > getting a compiler warning that required me to cast the value back to an
> > enum to silence it:
>
> >             pgstat_tracks_io_op(bktype, (IOObject) io_object,
> > io_context, io_op))
>
> > However, I am now unable to reproduce that warning.
> > Moreover, I see in cases like table_block_relation_size() with
> > ForkNumber, the variable i is passed with no cast to smgrnblocks().
>
> Yeah, my druthers would be to just do it the way we do comparable
> things with ForkNumber.  I don't feel like we need to invent a
> better way here.
>
> The risk of needing to cast when using the "int" loop variable
> as an enum is obviously the downside of that approach, but we have
> not seen any indication that any compilers actually do warn.
> It's interesting that you did see such a warning ... I wonder which
> compiler you were using at the time?

so, pretty much any version of clang I tried with
-Wsign-conversion produces a warning.

<source>:35:32: warning: implicit conversion changes signedness: 'int'
to 'IOOp' (aka 'enum IOOp') [-Wsign-conversion]

I didn't do the casts in the attached patch since they aren't done elsewhere.

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