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
