At Sat, 26 Sep 2020 15:48:49 +0530, Amit Kapila <amit.kapil...@gmail.com> wrote in > 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
+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
