Horton

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

Kyotaro Horiguchi <horikyota.ntt@gmail.com>

View thread View on postgr.es
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
To: andres@anarazel.de
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-25T07:56:17Z
Lists: pgsql-hackers 
At Tue, 24 Jan 2023 14:35:12 -0800, Andres Freund <andres@anarazel.de> wrote in 
> > 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.

Okay.

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

I thought about the following.

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


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

Hmm.  Then what the "_s" stands for?

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

Uuuummmmm.  Okay, just I like "be" there!  Anyway, I don't strongly
push that.

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

What does the field show in that case?  The mean of operation size? Or
one row per opration size?  If the former, the name looks somewhat
wrong. If the latter, block_size seems making sense.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

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.