From: Julien Rouhaud <rjuju123@gmail.com>
To: Kirill Bychik <kirill.bychik@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Cc: Thomas Munro <thomas.munro@gmail.com>
Subject: Re: WAL usage calculation patch
Date: Tue, 17 Mar 2020 21:32:22 +0100
Message-ID: <20200317203222.GA44781@nol> (raw)
In-Reply-To: <CAB-hujrd+8kb5o+k1dy=r3oeuxnWO_PuaQiOiPHQHc8unH5ywg@mail.gmail.com>
References: <CAB-hujrGU83zRuNxg8NBgGBt8aNmj5OXUDiW=TawvMH3-peqcQ@mail.gmail.com>
<20200304160225.GA26982@nol>
<CAB-hujqqsdeTq_eMihTamJ3dhG64EDdp-fWrLYVZ=3CfApfrfw@mail.gmail.com>
<CAB-hujr8aPN6sY--qB1bidfsxE0kKkkXDe_LeaL1CRv6KYYGpg@mail.gmail.com>
<CAOBaU_aECK1Z7Nn+x=MhvEwrJzK8wyPsPtWAafjqtZN1fYjEmg@mail.gmail.com>
<CAB-hujqEdFjZy=tZHeWm0BGPGs7DiahNNdrhKhN0o0YzDsofkg@mail.gmail.com>
<CAOBaU_Z+Z+nR5hLsAjXZhGoGosfBjrt8VWmHTWAZiR4Y5uy7oA@mail.gmail.com>
<CAB-hujoJ7dUcy=upG+3QOzrad0qWdCW+LkC10vSW2UNLVvhEbA@mail.gmail.com>
<20200317153136.GA63950@nol>
<CAB-hujrd+8kb5o+k1dy=r3oeuxnWO_PuaQiOiPHQHc8unH5ywg@mail.gmail.com>
On Tue, Mar 17, 2020 at 10:27:05PM +0300, Kirill Bychik wrote:
> > > Please feel free to work on any extension of this patch idea. I lack
> > > both time and knowledge to do it all by myself.
> >
> > I'm adding a 3rd patch on top of yours to expose the new WAL counters in
> > pg_stat_database, for vacuum and autovacuum. I'm not really enthiusiastic with
> > this approach but I didn't find better, and maybe this will raise some better
> > ideas. The only sure thing is that we're not going to add a bunch of new
> > fields in pg_stat_all_tables anyway.
> >
> > We can also drop this 3rd patch entirely if no one's happy about it without
> > impacting the first two.
>
> No objections about 3rd on my side, unless we miss the CF completely.
>
> As for the code, I believe:
> + walusage.wal_records = pgWalUsage.wal_records -
> + walusage_start.wal_records;
> + walusage.wal_fp_records = pgWalUsage.wal_fp_records -
> + walusage_start.wal_fp_records;
> + walusage.wal_bytes = pgWalUsage.wal_bytes - walusage_start.wal_bytes;
>
> Could be done much simpler via the utility:
> WalUsageAccumDiff(walusage, pgWalUsage, walusage_start);
Indeed, but this function is private to instrument.c. AFAICT
pg_stat_statements is already duplicating similar code for buffers rather than
having BufferUsageAccumDiff being exported, so I chose the same approach.
I'd be in favor of exporting both functions though.
> On a side note, I agree API to the buf/wal usage is far from perfect.
Yes clearly.
> > > Test had been reworked, and I believe it should be stable now, the
> > > part which checks WAL is written and there is a correlation between
> > > affected rows and WAL records. I still have no idea how to test
> > > full-page writes against regular updates, it seems very unstable.
> > > Please share ideas if any.
> >
> >
> > I just reviewed the patches, and it globally looks good to me. The way to
> > detect full page images looks sensible, but I'm really not familiar with that
> > code so additional review would be useful.
> >
> > I noticed that the new wal_write_fp_records field in pg_stat_statements wasn't
> > used in the test. Since I have to add all the patches to make the cfbot happy,
> > I slightly adapted the tests to reference the fp column too. There was also a
> > minor issue in the documentation, as wal_records and wal_bytes were copy/pasted
> > twice while wal_write_fp_records wasn't documented, so I also changed it.
> >
> > Let me know if you're ok with those changes.
>
> Sorry for not getting wal_fp_usage into the docs, my fault.
>
> As for the tests, please get somebody else to review this. I strongly
> believe checking full page writes here could be a source of
> instability.
I'm also a little bit dubious about it. The initial checkpoint should make
things stable (of course unless full_page_writes is disabled), and Cfbot also
seems happy about it. At least keeping it for the temporary tables test
shouldn't be a problem.
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: rjuju123@gmail.com, kirill.bychik@gmail.com, thomas.munro@gmail.com
Subject: Re: WAL usage calculation patch
In-Reply-To: <20200317203222.GA44781@nol>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox