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
to write WAL due to wal_buffers, it will be accounted next time the
backend sends the stats.
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.
--
With Regards,
Amit Kapila.
