Horton

Re: WAL usage calculation patch

Julien Rouhaud <rjuju123@gmail.com>

View thread
From: Julien Rouhaud <rjuju123@gmail.com>
To: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, Fujii Masao <masao.fujii@oss.nttdata.com>, Kirill Bychik <kirill.bychik@gmail.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>, Thomas Munro <thomas.munro@gmail.com>
Date: 2020-03-31T13:51:58Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Change the display of WAL usage statistics in Explain.

  2. Cosmetic fixups for WAL usage work.

  3. Allow parallel create index to accumulate buffer usage stats.

  4. Allow autovacuum to log WAL usage statistics.

  5. Add the option to report WAL usage in EXPLAIN and auto_explain.

  6. Allow pg_stat_statements to track WAL usage statistics.

  7. Add infrastructure to track WAL usage.

  8. Include information on buffer usage during planning phase, in EXPLAIN output, take two.

  9. Include information on buffer usage during planning phase, in EXPLAIN output.

  10. Allow parallel vacuum to accumulate buffer usage.

  11. Allow pg_stat_statements to track planning statistics.

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.