Date: Wed, 25 Jan 2023 16:56:17 +0900 (JST)
Message-Id: <20230125.165617.724951472557617342.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
Subject: Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and
 more?)
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
In-Reply-To: <20230124223512.xqp7tjgqvzqokaxq@awork3.anarazel.de>
References: <CAAKRu_af0YcV9CXG9M5R06tYRUzk13+H7qOsU6etmS4zE7RAUQ@mail.gmail.com>
	<20230124.172203.448918351330829200.horikyota.ntt@gmail.com>
	<20230124223512.xqp7tjgqvzqokaxq@awork3.anarazel.de>
User-Agent: Mew version 6.8 on Emacs 26.1
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/20230125.165617.724951472557617342.horikyota.ntt%40gmail.com>
Precedence: bulk

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