Hi,
Thanks for the review.
On Fri, 17 Mar 2023 at 02:02, Melanie Plageman
<melanieplageman@gmail.com> wrote:
> I think you want one less L here?
> WALLSTAT_ACC_INSTR_TIME_TYPE -> WALSTAT_ACC_INSTR_TIME_TYPE
Done.
> Also, I don't quite understand why TYPE is at the end of the name. I
> think it would still be clear without it.
Done.
> I might find it clearer if the WALSTAT_ACC_INSTR_TIME_TYPE macro was
> defined before using it for those fields instead of defining it right
> after defining WALSTAT_ACC.
Since it is undefined together with WALSTAT_ACC, defining them
together makes sense to me.
> > + * 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.
> > + */
> > +typedef struct PgStat_PendingWalStats
> > +{
> > + PgStat_Counter wal_buffers_full;
> > + PgStat_Counter wal_write;
> > + PgStat_Counter wal_sync;
> > + instr_time wal_write_time;
> > + instr_time wal_sync_time;
> > +} PgStat_PendingWalStats;
> > +
>
> So, I am not a fan of having this second struct (PgStat_PendingWalStats)
> which only has a subset of the members of PgStat_WalStats. It is pretty
> confusing.
>
> It is okay to have two structs -- one that is basically "in-memory" and
> one that is a format that can be on disk, but these two structs with
> different members are confusing and don't convey why we have the two
> structs.
>
> I would either put WalUsage into PgStat_PendingWalStats (so that it has
> all the same members as PgStat_WalStats), or figure out a way to
> maintain WalUsage separately from PgStat_WalStats or something else.
> Worst case, add more comments to the struct definitions to explain why
> they have the members they have and how WalUsage relates to them.
Yes, but like Andres said this could be better done as a separate patch.
v6 is attached.
Regards,
Nazir Bilal Yavuz
Microsoft
