On Thu, Nov 30, 2023 at 6:37 AM Peter Smith <smithpb2...@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.
