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

Hi,

Thanks for your comments and advice. I updated the patch.

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 Horiguchi-san's advice, I changed to reuse pgWalUsage
which is already defined and eliminates the extra overhead.

> 5. I notice some code in issue_xlog_fsync() function to collect sync 
> info,
> a standby may call the issue_xlog_fsync() too, which do not want to
> to collect this info. I think this need some change avoid run by
> standby side.

IIUC, issue_xlog_fsync is called by wal receiver on standby side.
But it doesn't send collected statistics to the stats collecter.
So, I think it's not necessary to change the code to avoid collecting 
the stats on the standby side.

Regards,
-- 
Masahiro Ikeda
NTT DATA CORPORATION