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