RE: [PoC] pg_upgrade: allow to upgrade publisher node

Dear Peter,

Thank you for reviewing! PSA new version patch set.
 
> ======
> Commit message.
> 
> 1.
> I felt this should mention the limitation that the slot upgrade
> feature is only supported from PG17 slots upwards.

Added. The same sentence as doc was used.

> doc/src/sgml/ref/pgupgrade.sgml
> 
> 2.
> +    <para>
> +     <application>pg_upgrade</application> attempts to migrate logical
> +     replication slots. This helps avoid the need for manually defining the
> +     same replication slots on the new publisher. Currently,
> +     <application>pg_upgrade</application> supports migrate logical
> replication
> +     slots when the old cluster is 17.X and later.
> +    </para>
> 
> Currently, <application>pg_upgrade</application> supports migrate
> logical replication slots when the old cluster is 17.X and later.
> 
> SUGGESTION
> Migration of logical replication slots is only supported when the old
> cluster is version 17.0 or later.

Fixed.

> src/bin/pg_upgrade/check.c
> 
> 3. GENERAL
> 
> IMO all version checking for this feature should only be done within
> this "check.c" file as much as possible.
> 
> The detailed reason for this PG17 limitation can be in the file header
> comment of "pg_upgrade.c", and then all the version checks can simply
> say something like:
> "Logical slot migration is only support for slots in PostgreSQL 17.0
> and later. See atop file pg_upgrade.c for an explanation of this
> limitation "

Hmm, I'm not sure it should be and Amit disagreed [1].
I did not address this one.

> 4. check_and_dump_old_cluster
> 
> + /* Extract a list of logical replication slots */
> + get_logical_slot_infos();
> +
> 
> IMO the version checking should only be done in the "checking"
> functions, so it should be removed from the within
> get_logical_slot_infos() and put here in the caller.
> 
> SUGGESTION
> 
> /* Logical slots can be migrated since PG17. */
> if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)
> {
> /* Extract a list of logical replication slots */
> get_logical_slot_infos();
> }

Per discussion [1], I did not address the comment.

> 5. check_new_cluster_logical_replication_slots
> 
> +check_new_cluster_logical_replication_slots(void)
> +{
> + PGresult   *res;
> + PGconn    *conn;
> + int nslots = count_logical_slots();
> + int max_replication_slots;
> + char    *wal_level;
> +
> + /* Quick exit if there are no logical slots on the old cluster */
> + if (nslots == 0)
> + return;
> 
> IMO the version checking should only be done in the "checking"
> functions, so it should be removed from the count_logical_slots() and
> then this code should be written more like this:
> 
> SUGGESTION (notice the quick return comment change too)
> 
> int nslots = 0;
> 
> /* Logical slots can be migrated since PG17. */
> if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)
>     nslots = count_logical_slots();
> 
> /* Quick return if there are no logical slots to be migrated. */
> if (nslots == 0)
>     return;

Fixed.

> src/bin/pg_upgrade/info.c
> 
> 6. GENERAL
> 
> For the sake of readability it might be better to make the function
> names more explicit:
> 
> get_logical_slot_infos() -> get_old_cluster_logical_slot_infos()
> count_logical_slots() -> count_old_cluster_logical_slots()

Fixed. Moreover, get_logical_slot_infos_per_db() also followed the style.

> 7. get_logical_slot_infos
> 
> +/*
> + * get_logical_slot_infos()
> + *
> + * Higher level routine to generate LogicalSlotInfoArr for all databases.
> + *
> + * Note: This function will not do anything if the old cluster is pre-PG 17.
> + * The logical slots are not saved at shutdown, and the confirmed_flush_lsn is
> + * always behind the SHUTDOWN_CHECKPOINT record. Subsequent checks
> done in
> + * check_for_confirmed_flush_lsn() would raise a FATAL error if such slots are
> + * included.
> + */
> +void
> +get_logical_slot_infos(void)
> 
> Move all this detailed explanation about the limitation to the
> file-level comment in "pg_upgrade.c". See also review comment #3.

Per discussion [1], I did not address the comment.

> 8. get_logical_slot_infos
> 
> +void
> +get_logical_slot_infos(void)
> +{
> + int dbnum;
> +
> + /* Logical slots can be migrated since PG17. */
> + if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1600)
> + return;
> 
> IMO the version checking is best done in the "checking" functions. See
> previous review comments about the caller of this. If you want to put
> something here, then just have an Assert:
> 
> Assert(GET_MAJOR_VERSION(old_cluster.major_version) >= 1700);

As I said above, check_and_dump_old_cluster() still does not check major version
before calling get_old_cluster_logical_slot_infos(). So I kept current style.

> 9. count_logical_slots
> 
> +/*
> + * count_logical_slots()
> + *
> + * Sum up and return the number of logical replication slots for all databases.
> + */
> +int
> +count_logical_slots(void)
> +{
> + int dbnum;
> + int slot_count = 0;
> +
> + /* Quick exit if the version is prior to PG17. */
> + if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1600)
> + return 0;
> +
> + for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
> + slot_count += old_cluster.dbarr.dbs[dbnum].slot_arr.nslots;
> +
> + return slot_count;
> +}
> 
> IMO it is better to remove the version-checking side-effect here. Do
> the version checks from the "check" functions where this is called
> from. Also removing the check from here gives the ability to output
> more useful messages -- e.g. review comment #11

Apart from this, count_old_cluster_logical_slots() are called after checking
major version. Assert() was added instead.

> src/bin/pg_upgrade/pg_upgrade.c
> 
> 10. File-level comment
> 
> Add a detailed explanation about the limitation in the file-level
> comment. See review comment #3 for details.

Per discussion [1], I did not address the comment.

> 11.
> + /*
> + * Create logical replication slots.
> + *
> + * Note: This must be done after doing the pg_resetwal command because
> + * pg_resetwal would remove required WALs.
> + */
> + if (count_logical_slots())
> + {
> + start_postmaster(&new_cluster, true);
> + create_logical_replication_slots();
> + stop_postmaster(false);
> + }
> +
> 
> IMO it is better to do the explicit version checking here, instead of
> relying on a side-effect within the count_logical_slots() function.
> 
> SUGGESTION #1
> 
> /* Logical replication slot upgrade only supported for old_cluster >= PG17 
*/
> if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)
> {
> if (count_logical_slots())
> {
> start_postmaster(&new_cluster, true);
> create_logical_replication_slots();
> stop_postmaster(false);
> }
> }
> 
> AND...
> 
> By doing this, you will be able to provide more useful output here like this:
> 
> SUGGESTION #2 (my preferred)
> 
> if (count_logical_slots())
> {
>     if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1600)
>     {
>         pg_log(PG_WARNING,
>             "\nWARNING: This utility can only upgrade logical
> replication slots present in PostgreSQL version %s and later.",
>             "17.0");
>     }
>     else
>     {
>         start_postmaster(&new_cluster, true);
>         create_logical_replication_slots();
>         stop_postmaster(false);
>     }
> }
>

Per discussion [1], SUGGESTION #1 was chosen.

[1]: https://www.postgresql.org/message-id/CAA4eK1Jfk6eQSpasg+GoJVjtkQ3tFSihurbCFwnL3oV75BoUgQ@mail.gmail.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED