Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?) - Mailing list pgsql-hackers

Hello.

At Thu, 19 Jan 2023 21:15:34 -0500, Melanie Plageman <melanieplageman@gmail.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.


-/* 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