On 2021/04/21 15:08, torikoshia wrote:
> On 2021-04-16 10:27, Masahiro Ikeda wrote:
>> On 2021/04/13 9:33, Fujii Masao wrote:
>>>
>>>
>>> On 2021/03/30 20:37, Masahiro Ikeda wrote:
>>>> OK, I added the condition to the fast-return check. I noticed that I
>>>> misunderstood that the purpose is to avoid expanding a clock check
>>>> using WAL stats counters. But, the purpose is to make the conditions
>>>> stricter, right?
>>>
>>> Yes. Currently if the following condition is false even when the WAL
>>> counters are updated, nothing is sent to the stats collector. But with
>>> your patch, in this case the WAL stats are sent.
>>>
>>> if ((pgStatTabList == NULL || pgStatTabList->tsa_used == 0) &&
>>> pgStatXactCommit == 0 && pgStatXactRollback == 0 &&
>>> !have_function_stats && !disconnect)
>>>
>>> Thanks for the patch! It now fails to be applied to the master
>>> cleanly. So could you rebase the patch?
>>
>> Thanks for your comments! I rebased it.
>
> Thanks for working on this!
Hi, thanks for your comments!
> I have some minor comments on
> performance-improvements-of-reporting-wal-stats-without-introducing-a-new-variable.patch.
>
>
>
>
> 177 @@ -3094,20 +3066,33 @@ pgstat_report_wal(void) 178 * Return true if
> the message is sent, and false otherwise.
>
> Since you changed the return value to void, it seems the description is not
> necessary anymore.
Right, I fixed it.
> 208 + * generate wal records. 'wal_writes' and 'wal_sync' are zero
> means the
>
> It may be better to change 'wal_writes' to 'wal_write' since single
> quotation seems to mean variable name.
Agreed.
> 234 + * set the counters related to generated WAL data if the
> counters are
>
>
> set -> Set?
Yes, I fixed.
> BTW, is it better to change PgStat_Counter from int64 to uint64 because> there aren't any counters with negative
number?
Although this is not related to torikoshi-san's comment, the above my
understanding is not right. Some counters like delta_live_tuples,
delta_dead_tuples and changed_tuples can be negative.
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION