Hi, here are my comments for patch v26-0002. ====== 1. About the PG17 limitation
In my previous review of v25-0002, I suggested that the PG17 limitation should be documented atop one of the source files. See [1]#3, [1]#7, [1]#10 I just wanted to explain the reason for that suggestion. Currently, all the new version checks have a comment like "/* Logical slots can be migrated since PG17. */". I felt that it would be better if those comments said something more like "/* Logical slots can be migrated since PG17. See XYZ for details. */". I don't really care *where* the main explanation lives, but I thought since it is referenced from multiple places it might be easier to find if it was atop some file instead of just in a function comment. YMMV. ====== 2. Do version checking in check_and_dump_old_cluster instead of inside get_old_cluster_logical_slot_infos check_and_dump_old_cluster - Should check version before calling get_old_cluster_logical_slot_infos get_old_cluster_logical_slot_infos - Keep a sanity check Assert if you wish (or do nothing -- e.g. see #3 below) Refer to [1]#4, [1]#8 Isn't it self-evident from the file/function names what kind of logic they are intended to have in them? Sure, there may be some exceptions but unless it is difficult to implement I think most people would reasonably assume: - checking code should be in file "check.c" -- e.g. a function called 'check_and_dump_old_cluster' ought to be *checking* stuff - info fetching code should be in file "info.c" ~~ Another motivation for this suggestion becomes more obvious later with patch 0003. By checking at the "higher" level (in check.c) it means multiple related functions can all be called under one version check. Less checking means less code and/or simpler code. For example, multiple redundant calls to get_old_cluster_count_slots() can be avoided in patch 0003 by writing *less* code, than v26* currently has. ====== 3. count_old_cluster_logical_slots I think there is nothing special in this logic that will crash if PG version <= 1600. Keep the Assert for sanity checking if you wish, but this is already guarded by the call in pg_upgrade.c so perhaps it is overkill. ------ [1] My review of v25-0002 - https://www.postgresql.org/message-id/CAHut%2BPtQcou3Bfm9A5SbhFuo2uKK-6u4_j_59so3skAi8Ns03A%40mail.gmail.com Kind Regards, Peter Smith. Fujitsu Australia
