On Tue, Mar 31, 2020 at 11:21 AM Dilip Kumar <dilipbal...@gmail.com> wrote: > > I have started reviewing this patch and I have some comments/questions.
Thanks a lot! > > 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. Agreed, fixed. > 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; Done. > 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? Yes I agree, the name was too confusing. I went with wal_num_fpw. I also used the same for pg_stat_statements. Other fields are usually named with a trailing "s" but wal_fpws just seems too weird. I can change it if consistency is preferred here. > 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? I agree with Amit's POV. For now a single counter seems like enough to diagnose many behaviors. I'll keep answering following mails before sending an updated patchset.