Re: Add statistics to pg_stat_wal view for wal related parameter tuning

At Thu, 22 Oct 2020 10:44:53 +0900, Masahiro Ikeda <ikedamsh@oss.nttdata.com> wrote in 
> On 2020-10-21 18:03, Kyotaro Horiguchi wrote:
> > At Tue, 20 Oct 2020 16:11:29 +0900, Masahiro Ikeda
> > <ikedamsh@oss.nttdata.com> wrote in
> >> On 2020-10-20 12:46, Amit Kapila wrote:
> >> > I see that we also need to add extra code to capture these stats (some
> >> > of which is in performance-critical path especially in
> >> > XLogInsertRecord) which again makes me a bit uncomfortable. It might
> >> > be that it is all fine as it is very important to collect these stats
> >> > at cluster-level in spite that the same information can be gathered at
> >> > statement-level to help customers but I don't see a very strong case
> >> > for that in your proposal.
> > We should avoid that duplication as possible even if the both number
> > are important.
> > 
> >> Also about performance, I thought there are few impacts because it
> >> increments stats in memory. If I can implement to reuse pgWalUsage's
> >> value which already collects these stats, there is no impact in
> >> XLogInsertRecord.
> >> For example, how about pg_stat_wal() calculates the accumulated
> >> value of wal_records, wal_fpi, and wal_bytes to use pgWalUsage's
> >> value?
> > I don't think that works, but it would work that pgstat_send_wal()
> > takes the difference of that values between two successive calls.
> > WalUsage prevWalUsage;
> > ...
> > pgstat_send_wal()
> > {
> > ..
> >    /* fill in some values using pgWalUsage */
> >    WalStats.m_wal_bytes = pgWalUsage.wal_bytes - prevWalUsage.wal_bytes;
> >    WalStats.m_wal_records = pgWalUsage.wal_records -
> >    prevWalUsage.wal_records;
> >    WalStats.m_wal_wal_fpi = pgWalUsage.wal_fpi - prevWalUsage.wal_fpi;
> > ...
> >    pgstat_send(&WalStats, sizeof(WalStats));
> >    /* remember the current numbers */
> >    prevWalUsage = pgWalUsage;
> 
> Thanks for your advice. This code can avoid the performance impact of
> critical code.
> 
> By the way, what do you think to add these statistics to the
> pg_stat_wal view?
> I thought to remove the above statistics because there is advice that
> PG13's features,
> for example, pg_stat_statement view, vacuum log, and so on can cover
> use-cases.


At Thu, 22 Oct 2020 10:09:21 +0900, Masahiro Ikeda <ikedamsh@oss.nttdata.com> wrote in 
> >> I agreed that the statement-level stat is important and I understood
> >> that we can
> >> know the aggregated WAL stats of pg_stat_statement view and
> >> autovacuum's
> >> log.
> >> But now, WAL stats generated by autovacuum can be output to logs and
> >> it
> >> is not
> >> easy to aggregate them. Since WAL writes impacts for the entire
> >> cluster,
> >> I thought
> >> it's natural to provide accumulated value.
> >> 
> > I think it is other way i.e if we would have accumulated stats then it
> > makes sense to provide those at statement-level because one would like
> > to know the exact cause of more WAL activity. Say it is due to an
> > autovacuum or due to the particular set of statements then it would
> > easier for users to do something about it.
> 
> OK, I'll remove them.
> Do you have any comments for other statistics?

That discussion comes from the fact that the code adds duplicate code
in a hot path.  If we that extra cost doesn't exist, we are free to
add the accumulated values in pg_stat_wal. I think they are useful for
stats-collecting tools as far as we can do that without such an extra
cost.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center