Re: Add statistics to pg_stat_wal view for wal related parameter tuning - Mailing list pgsql-hackers

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@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.
> 
> +    /* 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