From: | Julien Rouhaud <rjuju123(at)gmail(dot)com> |
---|---|
To: | Dilip Kumar <dilipbalaut(at)gmail(dot)com> |
Cc: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com>, Kirill Bychik <kirill(dot)bychik(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com> |
Subject: | Re: WAL usage calculation patch |
Date: | 2020-03-31 13:51:58 |
Message-ID: | CAOBaU_ZO1EDNMssauG00cM00NMzwPBiFUmhOrJB38Jo9yzH5YQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Mar 31, 2020 at 11:21 AM Dilip Kumar <dilipbalaut(at)gmail(dot)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.
From | Date | Subject | |
---|---|---|---|
Next Message | Prabhat Sahu | 2020-03-31 13:59:15 | Re: [Proposal] Global temporary tables |
Previous Message | Daniel Gustafsson | 2020-03-31 13:37:10 | Random set of typos spotted |