Re: WAL usage calculation patch

On Tue, Apr 7, 2020 at 4:36 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Apr 6, 2020 at 7:58 PM Euler Taveira
> <euler(dot)taveira(at)2ndquadrant(dot)com> wrote:
> >
> > On Mon, 6 Apr 2020 at 10:37, Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote:
> >>
> >> On Mon, Apr 06, 2020 at 10:12:55AM -0300, Euler Taveira wrote:
> >> > On Mon, 6 Apr 2020 at 00:25, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >> >
> >> > >
> >> > > I have pushed pg_stat_statements and Explain related patches. I am
> >> > > now looking into (auto)vacuum patch and have few comments.
> >> > >
> >> > > I wasn't paying much attention to this thread. May I suggest changing
> >> > wal_num_fpw to wal_fpw? wal_records and wal_bytes does not have a prefix
> >> > 'num'. It seems inconsistent to me.
> >> >
> >>
> >> If we want to be consistent shouldn't we rename it to wal_fpws? FTR I don't
> >> like much either version.
> >
> >
> > Since FPW is an acronym, plural form reads better when you are using uppercase (such as FPWs or FPW's); thus, I prefer singular form because parameter names are lowercase. Function description will clarify that this is "number of WAL full page writes".
> >
>
> I like Euler's suggestion to change wal_num_fpw to wal_fpw. It is
> better if others who didn't like this name can also share their
> opinion now because changing multiple times the same thing is not a
> good idea.

+1

About Justin and your comments on the other thread:

On Tue, Apr 7, 2020 at 4:31 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Apr 6, 2020 at 10:04 PM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
> >
> > On Thu, Apr 02, 2020 at 08:29:31AM +0200, Julien Rouhaud wrote:
> > > > > "full page records" seems to be showing the number of full page
> > > > > images, not the record having full page images.
> > > >
> > > > I am not sure what exactly is a difference but it is the records
> > > > having full page images. Julien correct me if I am wrong.
> >
> > > Obviously previous complaints about the meaning and parsability of
> > > "full page writes" should be addressed here for consistency.
> >
> > There's a couple places that say "full page image records" which I think is
> > language you were trying to avoid. It's the number of pages, not the number of
> > records, no ? I see explain and autovacuum say what I think is wanted, but
> > these say the wrong thing? Find attached slightly larger patch.
> >
> > $ git grep 'image record'
> > contrib/pg_stat_statements/pg_stat_statements.c: int64 wal_num_fpw; /* # of WAL full page image records generated */
> > doc/src/sgml/ref/explain.sgml: number of records, number of full page image records and amount of WAL
> >
>
> Few comments:
> 1.
> - int64 wal_num_fpw; /* # of WAL full page image records generated */
> + int64 wal_num_fpw; /* # of WAL full page images generated */
>
> Let's change comment as " /* # of WAL full page writes generated */"
> to be consistent with other places like instrument.h. Also, make a
> similar change at other places if required.

Agreed. That's pg_stat_statements.c and instrument.h. I'll send a
patch once we reach consensus with the rest of the comments.

> 2.
> <entry>
> - Total amount of WAL bytes generated by the statement
> + Total number of WAL bytes generated by the statement
> </entry>
>
> I feel the previous text was better as this field can give us the size
> of WAL with which we can answer "how much WAL data is generated by a
> particular statement?". Julien, do you have any thoughts on this?

I also prefer "amount" as it feels more natural. I'm not a native
english speaker though, so maybe I'm just biased.