On Tue, Mar 31, 2020 at 11:21 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> I have started reviewing this patch and I have some comments/questions.
Thanks a lot!
>
> 1.
> @@ -22,6 +22,10 @@ static BufferUsage save_pgBufferUsage;
>
> static void BufferUsageAdd(BufferUsage *dst, const BufferUsage *add);
>
> +WalUsage pgWalUsage;
> +static WalUsage save_pgWalUsage;
> +
> +static void WalUsageAdd(WalUsage *dst, WalUsage *add);
>
> Better we move all variable declaration first along with other
> variables and then function declaration along with other function
> declaration. That is the convention we follow.
Agreed, fixed.
> 2.
> {
> bool need_buffers = (instrument_options & INSTRUMENT_BUFFERS) != 0;
> + bool need_wal = (instrument_options & INSTRUMENT_WAL) != 0;
>
> I think you need to run pgindent, we should give only one space
> between the variable name and '='.
> so we need to change like below
>
> bool need_wal = (instrument_options & INSTRUMENT_WAL) != 0;
Done.
> 3.
> +typedef struct WalUsage
> +{
> + long wal_records; /* # of WAL records produced */
> + long wal_fpw_records; /* # of full page write WAL records
> + * produced */
>
> IMHO, the name wal_fpw_records is bit confusing, First I thought it
> is counting the number of wal records which actually has FPW, then
> after seeing code, I realized that it is actually counting total FPW.
> Shouldn't we rename it to just wal_fpw? or wal_num_fpw or
> wal_fpw_count?
Yes I agree, the name was too confusing. I went with wal_num_fpw. I
also used the same for pg_stat_statements. Other fields are usually
named with a trailing "s" but wal_fpws just seems too weird. I can
change it if consistency is preferred here.
> 4. Currently, we are combining all full-page write
> force/normal/consistency checks in one category. I am not sure
> whether it will be good information to know how many are force_fpw and
> how many are normal_fpw?
I agree with Amit's POV. For now a single counter seems like enough
to diagnose many behaviors.
I'll keep answering following mails before sending an updated patchset.
