On Thu, Aug 24, 2023 at 7:55 AM Peter Smith <smithpb2...@gmail.com> wrote:
>
> ======
> src/bin/pg_upgrade/info.c
>
> 4. get_logical_slot_infos
>
> +/*
> + * get_logical_slot_infos()
> + *
> + * Higher level routine to generate LogicalSlotInfoArr for all databases.
> + */
> +void
> +get_logical_slot_infos(ClusterInfo *cluster)
> +{
> + int dbnum;
> +
> + /* Logical slots can be migrated since PG17. */
> + if (GET_MAJOR_VERSION(cluster->major_version) <= 1600)
> + return;
>
> It is no longer clear to me what is the purpose of these version checks.
>
> As mentioned in comment #2 above, I don't think we need to check the
> new_cluster >= 1700, because this patch is for PG17 by definition.
>
> OTOH, I also don't recognise the reason why there has to be a PG17
> restriction on the 'old_cluster' version. Such a restriction seems to cripple
> the usefulness of this patch (eg. cannot even upgrade slots from PG16 to
> PG17), and there is no explanation given for it. If there is some valid
> incompatibility reason why only PG17 old_cluster slots can be upgraded then
> it ought to be described in detail and probably also mentioned in the PG DOCS.
>
One of the main reasons is that slots prior to v17 won't persist
confirm_flush_lsn as discussed in the email thread [1] which means it
will always fail even if we allow to upgrade from versions prior to
v17. Now, there is an argument that let's backpatch what's being
discussed in [1] and then we will be able to upgrade slots from the
prior version. Normally, we don't backatch new enhancements, so even
if we want to do that in this case, a separate argument has to be made
for it. We have already discussed this point in this thread. We can
probably add a comment in the patch where we do version checks so that
it will be a bit easier to understand the reason.
[1] -
https://www.postgresql.org/message-id/CAA4eK1JzJagMmb_E8D4au%3DGYQkxox0AfNBm1FbP7sy7t4YWXPQ%40mail.gmail.com
--
With Regards,
Amit Kapila.
