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

On 2020-10-30 11:50, Fujii Masao wrote:
> On 2020/10/29 17:03, Masahiro Ikeda wrote:
>> 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(at)oss(dot)nttdata(dot)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.
>
> + /* 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_fpi = pgWalUsage.wal_fpi - prevWalUsage.wal_fpi;
>
> It's better to use WalUsageAccumDiff() here?

Yes, thanks. I fixed it.

> prevWalUsage needs to be initialized with pgWalUsage?
>
> + if (AmWalWriterProcess()){
> + WalStats.m_wal_write_walwriter++;
> + }
> + else
> + {
> + WalStats.m_wal_write_backend++;
> + }
>
> I think that it's better not to separate m_wal_write_xxx into two for
> walwriter and other processes. Instead, we can use one m_wal_write_xxx
> counter and make pgstat_send_wal() send also the process type to
> the stats collector. Then the stats collector can accumulate the
> counters
> per process type if necessary. If we adopt this approach, we can easily
> extend pg_stat_wal so that any fields can be reported per process type.

I'll remove the above source code because these counters are not useful.

On 2020-10-30 12:00, Fujii Masao wrote:
> On 2020/10/20 11:31, Masahiro Ikeda wrote:
>> Hi,
>>
>> I think we need to add some statistics to pg_stat_wal view.
>>
>> Although there are some parameter related WAL,
>> there are few statistics for tuning them.
>>
>> I think it's better to provide the following statistics.
>> Please let me know your comments.
>>
>> ```
>> postgres=# SELECT * from pg_stat_wal;
>> -[ RECORD 1 ]-------+------------------------------
>> wal_records         | 2000224
>> wal_fpi             | 47
>> wal_bytes           | 248216337
>> wal_buffers_full    | 20954
>> wal_init_file       | 8
>> wal_write_backend   | 20960
>> wal_write_walwriter | 46
>> wal_write_time      | 51
>> wal_sync_backend    | 7
>> wal_sync_walwriter  | 8
>> wal_sync_time       | 0
>> stats_reset         | 2020-10-20 11:04:51.307771+09
>> ```
>>
>> 1. Basic statistics of WAL activity
>>
>> - wal_records: Total number of WAL records generated
>> - wal_fpi: Total number of WAL full page images generated
>> - wal_bytes: Total amount of WAL bytes generated
>>
>> To understand DB's performance, first, we will check the performance
>> trends for the entire database instance.
>> For example, if the number of wal_fpi becomes higher, users may tune
>> "wal_compression", "checkpoint_timeout" and so on.
>>
>> Although users can check the above statistics via EXPLAIN,
>> auto_explain,
>> autovacuum and pg_stat_statements now,
>> if users want to see the performance trends  for the entire database,
>> they must recalculate the statistics.
>>
>> I think it is useful to add the sum of the basic statistics.
>>
>>
>> 2.  WAL segment file creation
>>
>> - wal_init_file: Total number of WAL segment files created.
>>
>> To create a new WAL file may have an impact on the performance of
>> a write-heavy workload generating lots of WAL. If this number is
>> reported high,
>> to reduce the number of this initialization, we can tune WAL-related
>> parameters
>> so that more "recycled" WAL files can be held.
>>
>>
>>
>> 3. Number of when WAL is flushed
>>
>> - wal_write_backend : Total number of WAL data written to the disk by
>> backends
>> - wal_write_walwriter : Total number of WAL data written to the disk
>> by walwriter
>> - wal_sync_backend : Total number of WAL data synced to the disk by
>> backends
>> - wal_sync_walwriter : Total number of WAL data synced to the disk by
>> walwrite
>>
>> I think it's useful for tuning "synchronous_commit" and "commit_delay"
>> for query executions.
>> If the number of WAL is flushed is high, users can know
>> "synchronous_commit" is useful for the workload.
>
> I just wonder how useful these counters are. Even without these
> counters,
> we already know synchronous_commit=off is likely to cause the better
> performance (but has the risk of data loss). So ISTM that these
> counters are
> not so useful when tuning synchronous_commit.

Thanks, my understanding was wrong.
I agreed that your comments.

I merged the statistics of *_backend and *_walwriter.
I think the sum of them is useful to calculate the average per
write/sync time.
For example, per write time is equals wal_write_time / wal_write.

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION