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. 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.
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. 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?
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(). Also checked v4 with the travis patch-tester. All is ok. With the best wishes! -- Anton A. Melnikov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
