Hi Hou-San. Here are my review comments for v4-0001. ======
1. Add links in the docs IMO it would be good for all these confl_* descriptions (in doc/src/sgml/monitoring.sgml) to include links back to where each of those conflict types was defined [1]. Indeed, when links are included to the original conflict type information then I think you should remove mentioning "track_commit_timestamp": + counted only when the + <link linkend="guc-track-commit-timestamp"><varname>track_commit_timestamp</varname></link> + option is enabled on the subscriber. It should be obvious that you cannot count a conflict if the conflict does not happen, but I don't think we should scatter/duplicate those rules in different places saying when certain conflicts can/can't happen -- we should just link everywhere back to the original description for those rules. ~~~ 2. Arrange all the counts into an intuitive/natural order There is an intuitive/natural ordering for these counts. For example, the 'confl_*' count fields are in the order insert -> update -> delete, which LGTM. Meanwhile, the 'apply_error_count' and the 'sync_error_count' are not in a good order. IMO it makes more sense if everything is ordered as: 'sync_error_count', then 'apply_error_count', then all the 'confl_*' counts. This comment applies to lots of places, e.g.: - docs (doc/src/sgml/monitoring.sgml) - function pg_stat_get_subscription_stats (pg_proc.dat) - view pg_stat_subscription_stats (src/backend/catalog/system_views.sql) - TAP test SELECTs (test/subscription/t/026_stats.pl) As all those places are already impacted by this patch, I think it would be good if (in passing) we (if possible) also swapped the sync/apply counts so everything is ordered intuitively top-to-bottom or left-to-right. ====== [1] https://www.postgresql.org/docs/devel/logical-replication-conflicts.html#LOGICAL-REPLICATION-CONFLICTS Kind Regards, Peter Smith. Fujitsu Australia
