public inbox for [email protected]
help / color / mirror / Atom feedFrom: Andres Freund <[email protected]>
To: Kyotaro Horiguchi <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Subject: Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)
Date: Tue, 24 Jan 2023 14:35:12 -0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <CALDaNm2iFT5Ubx_EYUq2x7P1PgQ8jJ=SiML3h4C9rn8wfhURbw@mail.gmail.com>
<CAAKRu_b=1cSOq6Ln96GYXaeMfUnVzpjSBXWvi_RNBJbQ0WnXug@mail.gmail.com>
<CAAKRu_af0YcV9CXG9M5R06tYRUzk13+H7qOsU6etmS4zE7RAUQ@mail.gmail.com>
<[email protected]>
Hi,
On 2023-01-24 17:22:03 +0900, Kyotaro Horiguchi wrote:
> Hello.
>
> At Thu, 19 Jan 2023 21:15:34 -0500, Melanie Plageman <[email protected]> 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
view thread (36+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox