Re: CREATE SUBSCRIPTION -- add missing tab-completes

On Fri, Apr 7, 2023 at 6:10 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Fri, Apr 7, 2023 at 1:12 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Fri, Apr 7, 2023 at 2:28 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Wed, Apr 5, 2023 at 5:58 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > > >
> > >
> > > LGTM, so pushed. BTW, while looking at this, I noticed that newly
> > > added options "password_required" and "run_as_owner" has incorrectly
> > > mentioned their datatype as a string in the docs. It should be
> > > boolean.
> >
> > +1
> >
> > > I think "password_required" belongs to first section of docs
> > > which says: "The following parameters control what happens during
> > > subscription creation".
> >
> > But the documentation of ALTER SUBSCRIPTION says:
> >
> > The parameters that can be altered are slot_name, synchronous_commit,
> > binary, streaming, disable_on_error, password_required, run_as_owner,
> > and origin. Only a superuser can set password_required = false.
> >
>
> By the above, do you intend to say that all the parameters that can be
> altered are in the second list? If so, slot_name belongs to the first
> category.
>
> > ISTM that both password_required and run_as_owner are parameters to
> > control the subscription's behavior, like disable_on_error and
> > streaming. So it looks good to me that password_required belongs to
> > the second section.
> >
>
> Do you mean that because 'password_required' is used each time we make
> a connection to a publisher during replication, it should be in the
> second category? If so, slot_name is also used during the start
> replication each time.

I think that parameters used by the backend process when performing
CREATE SUBSCRIPTION belong to the first category. And other parameters
used by apply workers and tablesync workers belong to the second
category. Since slot_name is used by both I'm not sure it should be in
the second category, but password_requried seems to be used by only
apply workers and tablesync workers, so it should be in the second
category.

>
> BTW, do we need to check one or both of these parameters in
> maybe_reread_subscription() where we "Exit if any parameter that
> affects the remote connection was changed."

As for run_as_owner, since we can dynamically switch the behavior I
think we don't need to reconnect. I'm not really sure about
password_required. From the implementation point of view, we don't
need to reconnect. Even if password_required is changed from false to
true, the apply worker already has the established connection. If it's
changed from true to false, we might not want to reconnect. I think we
need to consider it from the security point of view while checking the
motivation that password_required was introduced. So probably it's
better to discuss it on the original thread.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com