Horton

Re: WAL usage calculation patch

Dilip Kumar <dilipbalaut@gmail.com>

View thread
From: Dilip Kumar <dilipbalaut@gmail.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Julien Rouhaud <rjuju123@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-31T09:21:23Z
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 12:23 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Mar 30, 2020 at 6:14 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> >
> > On Mon, Mar 30, 2020 at 03:52:38PM +0530, Amit Kapila wrote:
> > >
> > > I think the right place to compute this information is
> > > XLogRecordAssemble even though we update it at the place where you
> > > have it in the patch.  You can probably compute that in local
> > > variables and then transfer to pgWalUsage in XLogInsertRecord.  I am
> > > fine if you can think of some other way but the current patch doesn't
> > > seem correct to me.
> >
> > My previous approach was indeed totally broken.  v8 attached which hopefully
> > will be ok.
> >
>
> This is better.  Few more comments:
> 1. The point (c) from my previous email doesn't seem to be fixed
> properly.  Basically, the record data is only attached with FPW in
> some particular cases like where REGBUF_KEEP_DATA is set, but the
> patch assumes it is always set.
>
> 2.
> + /* Report a full page imsage constructed for the WAL record */
> + *num_fpw += 1;
>
> Typo. /imsage/image
>
> 3.  We need to enhance the patch to cover WAL usage for parallel
> vacuum and parallel create index based on Sawada-San's latest patch[1]
> which fixed the case for buffer usage.

I have started reviewing this patch and I have some comments/questions.

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.

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;

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?


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?


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com