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

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?

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.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION