Re: pg_upgrade and logical replication

On Thu, Apr 13, 2023 at 10:51:10AM +0800, Julien Rouhaud wrote:
>
> On Wed, Apr 12, 2023 at 09:48:15AM +0000, Hayato Kuroda (Fujitsu) wrote:
> >
> > 5. AlterSubscription
> >
> > ```
> > +                               supported_opts = SUBOPT_RELID | SUBOPT_STATE | SUBOPT_LSN;
> > +                               parse_subscription_options(pstate, stmt->options,
> > +                                                                                  supported_opts, &opts);
> > +
> > +                               /* relid and state should always be provided. */
> > +                               Assert(IsSet(opts.specified_opts, SUBOPT_RELID));
> > +                               Assert(IsSet(opts.specified_opts, SUBOPT_STATE));
> > +
> > ```
> >
> > SUBOPT_LSN accepts "none" string, which means InvalidLSN. Isn't it better to
> > reject it?
>
> If you mean have an Assert for that I agree.  It's not supposed to be used by
> users so I don't think having non debug check is sensible, as any user provided
> value has no reason to be correct anyway.

After looking at the code I remember that I kept the lsn optional in ALTER
SUBSCRIPTION name ADD TABLE command processing.  For now pg_upgrade checks that
all subscriptions have a valid remote_lsn so there should indeed always be a
value different from InvalidLSN/none specified, but it's still unclear to me
whether this check will eventually be weakened or not, so for now I think it's
better to keep AlterSubscription accept this case, here and in all other code
paths.

If there's a hard objection I will just make the lsn mandatory.

> > 9. parseCommandLine
> >
> > ```
> > +       user_opts.preserve_subscriptions = false;
> > ```
> >
> > I think this initialization is not needed because it is default.
>
> It's not strictly needed because of C rules but I think it doesn't really hurt
> to make it explicit and not have to remember what the standard says.

So I looked at nearby code and other option do rely on zero-initialized global
variables, so I agree that this initialization should be removed.