Re: wal stats questions - Mailing list pgsql-hackers

On 2021/04/27 21:56, Fujii Masao wrote:
> 
> 
> On 2021/04/26 10:11, Masahiro Ikeda wrote:
>>
>> First patch has only the changes for pg_stat_wal view.
>> ("v6-0001-performance-improvements-of-reporting-wal-stats-without-introducing-a-new-variable.patch")
>>
> 
> +        pgWalUsage.wal_records == prevWalUsage.wal_records &&
> +        walStats.wal_write == 0 && walStats.wal_sync == 0 &&
> > WalStats.m_wal_write should be checked here instead of walStats.wal_write?

Thanks! Yes, I'll fix it.


> Is there really the case where the number of sync is larger than zero when
> the number of writes is zero? If not, it's enough to check only the number
> of writes?

I thought that there is the case if "wal_sync_method" is fdatasync, fsync or
fsync_writethrough. The example case is following.

(1) backend-1 writes the wal data because wal buffer has no space. But, it
doesn't sync the wal data.
(2) backend-2 reads data pages. In the execution, it need to write and sync
the wal because dirty pages is selected as victim pages. backend-2 need to
only sync the wal data because the wal data were already written by backend-1,
but they weren't synced.

I'm ok to change it since it's rare case.


> +     * wal records weren't generated. So, the counters of 'wal_fpi',
> +     * 'wal_bytes', 'm_wal_buffers_full' are not updated neither.
> 
> It's better to add the assertion check that confirms
> m_wal_buffers_full == 0 whenever wal_records is larger than zero?

Sorry, I couldn't understand yet. I thought that m_wal_buffers_full can be
larger than 0 if wal_records > 0.

Do you suggest that the following assertion is needed?

-       if (memcmp(&WalStats, &all_zeroes, sizeof(PgStat_MsgWal)) == 0)
-               return false;
+       if (pgWalUsage.wal_records == prevWalUsage.wal_records &&
+               WalStats.m_wal_write == 0 && WalStats.m_wal_sync == 0)
+       {
+               Assert(pgWalUsage.wal_fpi == 0 && pgWalUsage.wal_bytes &&
+                               WalStats.m_wal_buffers_full == 0 &&
WalStats.m_wal_write_time == 0 &&
+                               WalStats.m_wal_sync_time == 0);
+               return;
+       }


>> Second one has the changes for the type of the BufferUsage's and WalUsage's
>> members. I change the type from long to int64. Is it better to make new thread?
>> ("v6-0002-change-the-data-type-of-XXXUsage-from-long-to-int64.patch")
> 
> Will review the patch later. I'm ok to discuss that in this thread.

Thanks!


Regards,
-- 
Masahiro Ikeda
NTT DATA CORPORATION