Re: WAL usage calculation patch

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