Date: Sat, 28 Mar 2020 14:38:27 +0100
From: Julien Rouhaud <rjuju123@gmail.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Kirill Bychik <kirill.bychik@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
	Fujii Masao <masao.fujii@oss.nttdata.com>,
	Thomas Munro <thomas.munro@gmail.com>
Subject: Re: WAL usage calculation patch
Message-ID: <20200328133827.GA12854@nol>
References: <20200318171916.GA69354@nol>
 <5b71ad52-68c6-08cb-6408-31307e9e6f10@oss.nttdata.com>
 <20200319153129.GA36079@nol>
 <CAB-hujo_bnDsV7C1BNLMacC4gKTyHeR8_Fsdyv+rSoOcsk5HiA@mail.gmail.com>
 <a1452be6-9e82-59c4-3c54-b3f8919e75c0@oss.nttdata.com>
 <762d4ee8-9710-cdcd-e661-b5c79aaa9bf0@oss.nttdata.com>
 <CAOBaU_bb6gf-33qCsMiZYy6EupqAcT5xga3iM8q-=-Ap=+9w_w@mail.gmail.com>
 <CAB-hujrX8BWGYDH0h-4c0AQt0dfkNdaOPAaw5n7KWnzVe6vPOQ@mail.gmail.com>
 <CAOBaU_bRp3HmAnerZWFO00f8dx=nwU5WMUVY0woVY5kwMa_jEg@mail.gmail.com>
 <CAA4eK1+nLPguL+a1JStyx+L9xh0X3nQ3tXptorsggT6dn9oPLQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAA4eK1+nLPguL+a1JStyx+L9xh0X3nQ3tXptorsggT6dn9oPLQ@mail.gmail.com>
Precedence: bulk

On Sat, Mar 28, 2020 at 04:14:04PM +0530, Amit Kapila wrote:
> 
> I see some basic problems with the patch.  The way it tries to compute
> WAL usage for parallel stuff doesn't seem right to me.  Can you share
> or point me to any test done where we have computed WAL for parallel
> operations like Parallel Vacuum or Parallel Create Index?

Ah, that's indeed a good point and AFAICT WAL records from parallel utility
workers won't be accounted for.  That being said, I think that an argument
could be made that proper infrastructure should have been added in the original
parallel utility patches, as pg_stat_statement is already broken wrt. buffer
usage in parallel utility, unless I'm missing something.

> Basically,
> I don't know changes done in ExecInitParallelPlan and friends allow us
> to compute WAL for parallel operations.  Those will primarily cover
> parallel queries that won't write WAL.  How you have tested those
> changes?

I didn't tested those, and I'm not even sure how to properly and reliably test
that.  Do you have any advice on how to achieve that?

However the patch is mimicking the buffer instrumentation that already exists,
and the approach also looks correct to me.  Do you have a reason to believe
that the approach that works for buffer usage wouldn't work for WAL records? (I
of course agree that this should be tested anyway)