Dear Peter,
Thank you for reviewing! PSA new version patch set.
> ======
> 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.
IIUC these points were disagreed by Amit, so I would keep my code until he posts
opinions.
> 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.
Your point is right.
I have checked some version-specific functions like check_for_aclitem_data_type_usage()
and check_for_user_defined_encoding_conversions(), they do not have assert(). So
removed from it. As for free_db_and_rel_infos(), the Assert() ensures that new
cluster does not have logical slots, so I kept it.
Also, I found that get_loadable_libraries() always read pg_replication_slots,
even if the old cluster is older than PG17. This let additional checks for logical
decoding output plugins. Moreover, prior than PG12 could not be upgrade because
they do not have an attribute wal_status.
I think the checking should be done only when old_cluster is >= PG17, so fixed.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED