Re: WAL usage calculation patch

On Tue, Mar 31, 2020 at 2:39 PM Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote:
>
> On Tue, Mar 31, 2020 at 8:53 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Mon, Mar 30, 2020 at 6:14 PM Julien Rouhaud <rjuju123(at)gmail(dot)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.
>
> As I mentioned multiple times already, I'm really not familiar with
> the WAL code, so I'll be happy to be proven wrong but my reading is
> that in XLogRecordAssemble(), there are 2 different things being done:
>
> - a FPW is optionally added, iif include_image is true, which doesn't
> take into account REGBUF_KEEP_DATA. Looking at that part of the code
> I don't see any sign of the recorded FPW being skipped or discarded if
> REGBUF_KEEP_DATA is not set, and useful variables such as total_len
> are modified
> - then data is also optionally added, iif needs_data is set.
>
> IIUC a FPW can be added even if the WAL record doesn't contain data.
> So the behavior look ok to me, as what seems to be useful it to
> distinguish 9KB WAL for 1 record of 9KB from 9KB or WAL for 1KB record
> and 1 FPW.
>

It is possible that both of us are having different meanings for below
two variables:
+typedef struct WalUsage
+{
+ long wal_records; /* # of WAL records produced */
+ long wal_fpw_records; /* # of full page write WAL records
+ * produced */

Let me clarify my understanding. Say if the record is just an FPI
(ex. XLOG_FPI) and doesn't contain any data then do we want to add one
to each of wal_fpw_records and wal_records? My understanding was in
such a case we will just increment wal_fpw_records.

>
> > 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'm sorry but I'm not following. Do you mean adding regression tests
> for that case?
>

No. I mean to say we should implement WAL usage calculation for those
two parallel commands. AFAICS, your patch doesn't cover those two
commands.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com