On Fri, Sep 25, 2020 at 11:06 PM Fujii Masao <masao.fu...@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 > >> <ikeda...@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.