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