On 2021/05/11 16:44, Fujii Masao wrote:
>
>
> On 2021/04/28 9:10, Masahiro Ikeda wrote:
>>
>>
>> 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.
>
> Thanks!
Thanks for your comments!
I fixed them.
>>> 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.
>
> You're right. So let's leave the check of "m_wal_sync == 0".
OK.
>>> + * 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;
>> + }
>
> I was thinking to add the "Assert(WalStats.m_wal_buffers_full)" as a safe-guard
> because only m_wal_buffers_full is incremented in different places where
> wal_records, m_wal_write and m_wal_sync are incremented.
Understood. I added the assertion for m_wal_buffers_full only.
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
