Hi,
Thanks for the review.
On Wed, 22 Feb 2023 at 05:50, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
>
> PgStat_PendingStats should be included in typedefs.list.
Done.
>
> + * 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.
Done. And I think we should write why we didn't change
PgStat_WalStats's variable types and instead created a new struct.
Maybe we can explain it in the commit description?
>
> 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?
I tried a bit but it seems the required changes for additional
refactoring aren't small. So, I think we can create a separate patch
for these changes.
Regards,
Nazir Bilal Yavuz
Microsoft