On Wed, Aug 30, 2023 at 10:58 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are some review comments for v28-0003.
>
> ======
> src/bin/pg_upgrade/check.c
>
> 1. check_and_dump_old_cluster
> + /*
> + * Logical replication slots can be migrated since PG17. See comments atop
> + * get_old_cluster_logical_slot_infos().
> + */
> + if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)
> + check_old_cluster_for_valid_slots(live_check);
> +
>
> IIUC we are preferring to use the <= 1600 style of version check
> instead of >= 1700 where possible.
>
Yeah, but in this case, following the nearby code style, I think it is
okay to keep it as it is.
> ~
>
> 3b.
> /Quick exit/Quick return/
>
Hmm, either way should be okay.
> ~
>
> 4.
> + prep_status("Checking for logical replication slots");
>
> I felt that should add the word "valid" like:
> "Checking for valid logical replication slots"
>
Agreed and fixed.
> ~~~
>
> 5.
> + /* Check there are no logical replication slots with a 'lost' state. */
> + res = executeQueryOrDie(conn,
> + "SELECT slot_name FROM pg_catalog.pg_replication_slots "
> + "WHERE wal_status = 'lost' AND "
> + "temporary IS FALSE;");
>
> Since the SQL is checking if there *are* lost slots I felt it would be
> more natural to reverse that comment.
>
> SUGGESTION
> /* Check and reject if there are any logical replication slots with a
> 'lost' state. */
>
I changed the comments but differently.
> ~~~
>
> 6.
> + /*
> + * Do additional checks if a live check is not required. This requires
> + * that confirmed_flush_lsn of all the slots is the same as the latest
> + * checkpoint location, but it would be satisfied only when the server has
> + * been shut down.
> + */
> + if (!live_check)
>
> I think the comment can be rearranged slightly:
>
> SUGGESTION
> Do additional checks to ensure that 'confirmed_flush_lsn' of all the
> slots is the same as the latest checkpoint location.
> Note: This can be satisfied only when the old_cluster has been shut
> down, so we skip this for "live" checks.
>
Changed as per suggestion.
> ======
> src/bin/pg_upgrade/controldata.c
>
> 7.
> + /*
> + * Read the latest checkpoint location if the cluster is PG17
> + * or later. This is used for upgrading logical replication
> + * slots.
> + */
> + if (GET_MAJOR_VERSION(cluster->major_version) >= 1700)
> + {
>
> Fetching this "Latest checkpoint location:" value is only needed for
> the check_old_cluster_for_valid_slots validation check, isn't it? But
> AFAICT this code is common for both old_cluster and new_cluster.
>
> I am not sure what is best to do:
> - Do only the minimal logic needed?
> - Read the value redundantly even for new_cluster just to keep code simpler?
>
> Either way, maybe the comment should say something about this.
>
Added the comment.
--
With Regards,
Amit Kapila.