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

On Fri, Aug 25, 2023 at 2:14 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are my review comments for patch v25-0002.
>
> In general, I feel where possible the version checking is best done in
> the "check.c" file (the filename is a hint). Most of the review
> comments below are repeating this point.
>
> ======
> Commit message.
>
> 1.
> I felt this should mention the limitation that the slot upgrade
> feature is only supported from PG17 slots upwards.
>
> ======
> 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.
>
> ======
> 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 "
>

I don't think it is a good idea to move these comments atop
pg_upgrade.c as it is specific to slots. To me, the current place
proposed by the patch appears reasonable.

> ~~~
>
> 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.
>

I think we should do it where it makes more sense. As far as I can see
currently there is no such rule.

> 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();
> }
>

I find the current place better than this suggestion.

> ~~~
>
> 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;
>

+1.

> ======
> 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()
>
> ~~~
>
> 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.
>

-1. This is not generic enough to be moved to pg_upgrade.c.

>
> 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);
>     }
> }
>

I don't like suggestion#2 much. I don't feel the need for such a WARNING.


-- 
With Regards,
Amit Kapila.