At Tue, 21 Feb 2023 16:11:19 +0300, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote in
> I agree. The patch is updated.
Thanks, that part looks good to me. I'd like to provide some
additional comments.
PgStat_PendingStats should be included in typedefs.list.
+ * Created for accumulating wal_write_time and wal_sync_time as a instr_time
+ * but instr_time can't be used as a type where it ends up on-disk
+ * because its units may change. PgStat_WalStats type is used for
+ * in-memory/on-disk data. So, PgStat_PendingWalStats is created for
+ * accumulating intervals as a instr_time.
+ */
+typedef struct PgStat_PendingWalStats
IMHO, this comment looks somewhat off. Maybe we could try something
like the following instead?
> This struct stores wal-related durations as instr_time, which makes it
> easier to accumulate them without requiring type conversions. Then,
> during stats flush, they will be moved into shared stats with type
> conversions.
The aim of this patch is to keep using instr_time for accumulation.
So it seems like we could do the same refactoring for
pgStatBlockReadTime, pgStatBlockWriteTime, pgStatActiveTime and
pgStatTransactionIdleTime. What do you think - should we include this
additional refactoring in the same patch or make a separate one for
it?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center