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-27T14:24:01Z
Lists: pgsql-hackers

Attachments

On Sun, Feb 26, 2023 at 1:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
> > The issue seems to be that code like this:
> > ...
> > is far too cute for its own good.
>
> Oh, there's another thing here that qualifies as too-cute: loops like
>
>     for (IOObject io_object = IOOBJECT_FIRST;
>          io_object < IOOBJECT_NUM_TYPES; io_object++)
>
> make it look like we could define these enums as 1-based rather
> than 0-based, but if we did this code would fail, because it's
> confusing "the number of values" with "1 more than the last value".
>
> Again, we could fix that with tests like "io_context <= IOCONTEXT_LAST",
> but I don't see the point of adding more macros rather than removing
> some.  We do need IOOBJECT_NUM_TYPES to declare array sizes with,
> so I think we should nuke the "xxx_FIRST" macros as being not worth
> the electrons they're written on, and write these loops like
>
>     for (int io_object = 0; io_object < IOOBJECT_NUM_TYPES; io_object++)
>
> which is not actually adding any assumptions that you don't already
> make by using io_object as a C array subscript.

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++)

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

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