On 2024/09/22 13:55, Anton A. Melnikov wrote:
> On 20.09.2024 19:19, Fujii Masao wrote:
>> I've attached the updated version (0001.patch). I made some cosmetic changes,
>> including reverting the switch in the entries for pg_stat_get_checkpointer_write_time
>> and pg_stat_get_checkpointer_sync_time in pg_proc.dat, as I didn’t think
>> that change was necessary. Could you please review the latest version?
>
> Thanks for corrections!
> All looks good for me.
Thanks for the review! I've pushed the 0001 patch.
> As for switching in the pg_proc.dat entries the idea was to put them in order
> so that the pg_stat_get_checkpointer* functions were grouped together.
> I don't know if this is the common and accepted practice. Simply i like it better this way.
> Sure, if you think it's unnecessary, let it stay as is with minimal diff.
I understand your point, but I didn't made that change to keep the diff minimal,
which should make future back-patching easier.
>> After we commit 0001.patch, how about applying 0002.patch, which updates
>> the documentation for the pg_stat_checkpointer view to clarify what types
>> of checkpoints and restartpoints each counter tracks?
>
> I liked that the short definitions of the counters are now separated from
> the description of its work features which are combined into one paragraph.
> It seems to me that is much more logical and easier to understand.
Thanks for the review!
> In addition, checkpoints may be skipped due to "checkpoints are occurring
> too frequently" error. Not sure, but maybe add this information to
> the new description?
From what I can see in the code, that error message doesn’t seem to indicate
the checkpoint is being skipped. In fact, checkpoints are still happening
actually when that message appears. Am I misunderstanding something?
>> In 0002.patch, I also modified the description of num_requested from
>> "Number of backend requested checkpoints" to remove "backend," as it can
>> be confusing since num_requested includes requests from sources other than
>> the backend. Thought?
>
> Agreed. E.g. from xlog. Then maybe changed it also in the function
> descriptions in the pg_proc.dat? For pg_stat_get_checkpointer_num_requested()
> and pg_stat_get_checkpointer_restartpoints_requested().
Yes, good catch!
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
