public inbox for [email protected]  

help / color / mirror / Atom feed

From: Kyotaro Horiguchi <[email protected]>
To: [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 17:22:03 +0900 (JST)
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAAKRu_af0YcV9CXG9M5R06tYRUzk13+H7qOsU6etmS4zE7RAUQ@mail.gmail.com>
References: <CALDaNm2iFT5Ubx_EYUq2x7P1PgQ8jJ=SiML3h4C9rn8wfhURbw@mail.gmail.com>
	<CAAKRu_b=1cSOq6Ln96GYXaeMfUnVzpjSBXWvi_RNBJbQ0WnXug@mail.gmail.com>
	<CAAKRu_af0YcV9CXG9M5R06tYRUzk13+H7qOsU6etmS4zE7RAUQ@mail.gmail.com>

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.


-/* Reset some shared cluster-wide counters */
+/*
+ * Reset some shared cluster-wide counters
+ *
+ * When adding a new reset target, ideally the name should match that in
+ * pgstat_kind_infos, if relevant.
+ */

I'm not sure the addition is useful..


+pgstat_count_io_op(IOObject io_object, IOContext io_context, IOOp io_op)
+{
+	Assert(io_object < IOOBJECT_NUM_TYPES);
+	Assert(io_context < IOCONTEXT_NUM_TYPES);
+	Assert(io_op < IOOP_NUM_TYPES);
+	Assert(pgstat_tracks_io_op(MyBackendType, io_object, io_context, io_op));

Is there any reason for not checking the value ranges at the
bottom-most functions?  They can lead to out-of-bounds access so I
don't think we need to continue execution for such invalid values.

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


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


+ PgStat_BktypeIO

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


0004:

system_views.sql:

+FROM pg_stat_get_io() b;

What does the "b" stand for? (Backend? then "s" or "i" seems
straight-forward.)


+		nulls[col_idx] = !pgstat_tracks_io_op(bktype, io_obj, io_context, io_op);
+
+		if (nulls[col_idx])
+			continue;
+
+		values[col_idx] =
+			Int64GetDatum(bktype_stats->data[io_obj][io_context][io_op]);

This is a bit hard to read since it requires to follow the condition
flow. The following is simpler and I thhink close to our standard.

if (pgstat_tacks_io_op())
   values[col_idx] =
			Int64GetDatum(bktype_stats->data[io_obj][io_context][io_op]);
else
   nulls[col_idx]  = true;


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

It's uncertain to me why only the two columns are prefixed by
"io". Don't "object_type" and just "context" work instead?


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

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