Horton

Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)

Andres Freund <andres@anarazel.de>

View thread View on postgr.es
From: Andres Freund <andres@anarazel.de>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: melanieplageman@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-01-24T22:35:12Z
Lists: pgsql-hackers 
Hi,

On 2023-01-24 17:22:03 +0900, Kyotaro Horiguchi wrote:
> Hello.
> 
> At Thu, 19 Jan 2023 21:15:34 -0500, Melanie Plageman <melanieplageman@gmail.com> wrote in 
> > Oh dear-- an extra FlushBuffer() snuck in there somehow.
> > Removed it in attached v51.
> > Also, I fixed an issue in my tablespace.sql updates
> 
> I only looked 0002 and 0004.
> (Sorry for the random order of the comment..)
> 
> 0002:
> 
> +	Assert(pgstat_bktype_io_stats_valid(bktype_shstats, MyBackendType));
> 
> This is relatively complex checking. We already asserts-out increments
> of invalid counters. Thus this is checking if some unrelated codes
> clobbered them, which we do only when consistency is critical. Is
> there any needs to do that here?  I saw another occurance of the same
> assertion.

I found it useful to find problems.


> +	no_temp_rel = bktype == B_AUTOVAC_LAUNCHER || bktype == B_BG_WRITER ||
> +		bktype == B_CHECKPOINTER || bktype == B_AUTOVAC_WORKER ||
> +		bktype == B_STANDALONE_BACKEND || bktype == B_STARTUP;
> 
> I'm not sure I like to omit parentheses for such a long Boolean
> expression on the right side.

What parens would help?


> +	write_chunk_s(fpout, &pgStatLocal.snapshot.io);
> +	if (!read_chunk_s(fpin, &shmem->io.stats))
> 
> The names of the functions hardly make sense alone to me. How about
> write_struct()/read_struct()?  (I personally prefer to use
> write_chunk() directly..)

That's not related to this patch - there's several existing callers for
it. And write_struct wouldn't be better imo, because it's not just for
structs.


> + PgStat_BktypeIO
> 
> This patch abbreviates "backend" as "bk" but "be" is used in many
> places. I think that naming should follow the predecessors.

The precedence aren't consistent unfortunately :)


> > +        Number of read operations in units of <varname>op_bytes</varname>.
> 
> I may be the only one who see the name as umbiguous between "total
> number of handled bytes" and "bytes hadled at an operation". Can't it
> be op_blocksize or just block_size?
> 
> +       b.io_object,
> +       b.io_context,

No, block wouldn't be helpful - we'd like to use this for something that isn't
uniform blocks.

Greetings,

Andres Freund

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.