Re: pg_upgrade and logical replication

On Thu, Nov 30, 2023 at 6:37 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are some review comments for patch v20-0001
>
> ======
>
> 1. getSubscriptions
>
> + if (dopt->binary_upgrade && fout->remoteVersion >= 170000)
> + appendPQExpBufferStr(query, " s.subenabled\n");
> + else
> + appendPQExpBufferStr(query, " false AS subenabled\n");
>
> Probably I misunderstood this logic... AFAIK the CREATE SUBSCRIPTION
> is normally default *enabled*, so why does this code set default
> differently as 'false'. OTOH, if this is some special case default
> needed because the subscription upgrade is not supported before PG17
> then maybe it needs a comment to explain.
>

Yes, it is for prior versions. By default subscriptions are restored
disabled even if they are enabled before dump. See docs [1] for
reasons (When dumping logical replication subscriptions, ..). I don't
think we need a comment here as that is a norm we use at other similar
places where we do version checking. We can argue that there could be
more comments as to why the 'connect' is false and if those are really
required, we should do that as a separate patch.

[1] - https://www.postgresql.org/docs/devel/app-pgdump.html

-- 
With Regards,
Amit Kapila.