On Thu, Aug 24, 2023 at 7:55 AM Peter Smith <smithpb2250@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
PG17by definition.
>
> OTOH, I also don't recognise the reason why there has to be a PG17 restriction on the 'old_cluster' version. Such a
restrictionseems to cripple the usefulness of this patch (eg. cannot even upgrade slots from PG16 to PG17), and there
isno explanation given for it. If there is some valid incompatibility reason why only PG17 old_cluster slots can be
upgradedthen 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.