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.
