Re: New statistics for tuning WAL buffer size

At Sat, 26 Sep 2020 15:48:49 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in 
> On Fri, Sep 25, 2020 at 11:06 PM Fujii Masao
> <masao.fujii@oss.nttdata.com> wrote:
> >
> > On 2020/09/25 12:06, Masahiro Ikeda wrote:
> > > On 2020-09-18 11:11, Kyotaro Horiguchi wrote:
> > >> At Fri, 18 Sep 2020 09:40:11 +0900, Masahiro Ikeda
> > >> <ikedamsh@oss.nttdata.com> wrote in
> > >>> Thanks. I confirmed that it causes HOT pruning or killing of
> > >>> dead index tuple if DecodeCommit() is called.
> > >>>
> > >>> As you said, DecodeCommit() may access the system table.
> > >> ...
> > >>> The wals are generated only when logical replication is performed.
> > >>> So, I added pgstat_send_wal() in XLogSendLogical().
> > >>>
> > >>> But, I concerned that it causes poor performance
> > >>> since pgstat_send_wal() is called per wal record,
> > >>
> > >> I think that's too frequent.  If we want to send any stats to the
> > >> collector, it is usually done at commit time using
> > >> pgstat_report_stat(), and the function avoids sending stats too
> > >> frequently. For logrep-worker, apply_handle_commit() is calling it. It
> > >> seems to be the place if we want to send the wal stats.  Or it may be
> > >> better to call pgstat_send_wal() via pgstat_report_stat(), like
> > >> pg_stat_slru().
> > >
> > > Thanks for your comments.
> > > Since I changed to use pgstat_report_stat() and DecodeCommit() is calling it,
> > > the frequency to send statistics is not so high.
> >
> > On second thought, it's strange to include this change in pg_stat_wal patch.
> > Because pgstat_report_stat() sends various stats and that change would
> > affect not only pg_stat_wal but also other stats views. That is, if we really
> > want to make some processes call pgstat_report_stat() newly, which
> > should be implemented as a separate patch. But I'm not sure how useful
> > this change is because probably the stats are almost negligibly small
> > in those processes.
> >
> > This thought seems valid for pgstat_send_wal(). I changed the thought
> > and am inclined to be ok not to call pgstat_send_wal() in some background
> > processes that are very unlikely to generate WAL.
> >
> 
> This makes sense to me. I think even if such background processes have

+1

> This makes sense to me. I think even if such background processes have
> to write WAL due to wal_buffers, it will be accounted next time the
> backend sends the stats.

Where do they send the stats? (I think it's ok to omit seding stats at
all for such low-wal/heap activity processes.)

> One minor point, don't we need to reset the counter
> WalStats.m_wal_buffers_full once we sent the stats, otherwise the same
> stats will be accounted multiple times.

Isn't this doing that?

+    /*
+     * Clear out the statistics buffer, so it can be re-used.
+     */
+    MemSet(&WalStats, 0, sizeof(WalStats));

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center