On Tue, Mar 31, 2020 at 12:23 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Mon, Mar 30, 2020 at 6:14 PM Julien Rouhaud <rjuju...@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