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: Maciek Sakrejda <m.sakrejda@gmail.com>, Lukas Fittl <lukas@fittl.com>, 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>, Thomas Munro <thomas.munro@gmail.com>
Date: 2022-12-06T01:49:20Z
Lists: pgsql-hackers

Attachments

Attached is v40.

I have addressed the feedback from Justin [1] and Maciek [2] as well.
I took all of the suggestions regarding the docs that Maciek made,
including the following:

>  +       default. Future values could include those derived from
>  +       <symbol>XLOG_BLCKSZ</symbol>, once WAL IO is tracked in this view, and
>  +       constant multipliers once non-block-oriented IO (e.g. temporary file IO)
>  +       is tracked here.
>
>
>  I know Lukas had commented that we should communicate that the goal is
>  to eventually provide relatively comprehensive I/O stats in this view
>  (you do that in the view description and I think that works), and this
>  is sort of along those lines, but I think speculative documentation
>  like this is not all that helpful. I'd drop this last sentence. Just
>  my two cents.

I have removed this and added the relevant part of this as a comment to
the view generating function pg_stat_get_io().

On Mon, Dec 5, 2022 at 2:32 PM Andres Freund <andres@anarazel.de> wrote:
> - I think it might be worth to rename IOCONTEXT_BUFFER_POOL to
>   IOCONTEXT_{NORMAL, PLAIN, DEFAULT}. I'd like at some point to track WAL IO ,
>   temporary file IO etc, and it doesn't seem useful to define a version of
>   BUFFER_POOL for each of them. And it'd make it less confusing, because all
>   the other existing contexts are also in the buffer pool (for now, can't wait
>   for "bypass" or whatever to be tracked as well).

In attached v40, I've renamed IOCONTEXT_BUFFER_POOL to IOCONTEXT_NORMAL.

> - given that IOContextForStrategy() is defined in freelist.c, and that
>   declaring it in pgstat.h requires including buf.h, I think it's probably
>   better to move IOContextForStrategy()'s declaration to freelist.h (doesn't
>   exist, but whatever the right one is)

I have moved it to buf_internals.h.

> - pgstat_backend_io_stats_assert_well_formed() doesn't seem to belong in
>   pgstat.c. Why not pgstat_io_ops.c?

I put it in pgstat.c because it is only used there -- so I made it
static. I've moved it to pg_stat_io_ops.c and declared it in
pgstat_internal.h

> - Do pgstat_io_context_ops_assert_zero(), pgstat_io_op_assert_zero() have to
>   be in pgstat.h?

They are used in pgstatfuncs.c, which I presume should not include
pgstat_internal.h. Or did you mean that I should not put them in a
header file at all?

- Melanie

[1] https://www.postgresql.org/message-id/20221130025113.GD24131%40telsasoft.com
[2] https://www.postgresql.org/message-id/CAOtHd0BfFdMqO7-zDOk%3DiJTatzSDgVcgYcaR1_wk0GS4NN%2BRUQ%40mail.gmail.com

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.