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

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(at)gmail(dot)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