Subject: Re: CREATE SUBSCRIPTION -- add missing tab-completes

From: Amit Kapila <[email-masked]>
Date: Fri, 7 Apr 2023 10:58:11 +0530

On Wed, Apr 5, 2023 at 5:58 AM Peter Smith <[email-masked]> wrote:
>
> There are some recent comment that added new options for CREATE SUBSCRIPTION
>
...
> PSA patches to add those tab completions.
>

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. I think "password_required" belongs to first section of docs
which says: "The following parameters control what happens during
subscription creation".

The attached patch makes those changes. What do you think?

-- 
With Regards,
Amit Kapila.

---

From: Masahiko Sawada <[email-masked]>
Date: Fri, 7 Apr 2023 16:41:27 +0900

On Fri, Apr 7, 2023 at 2:28 PM Amit Kapila <[email-masked]> wrote:
> boolean.

+1

> I think "password_required" belongs to first section of docs

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.

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.

Regards,

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

---

From: Amit Kapila <[email-masked]>
Date: Fri, 7 Apr 2023 14:40:36 +0530

> 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.

> streaming. So it looks good to me that password_required belongs to

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.

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."

-- 
With Regards,
Amit Kapila.

---

From: Masahiko Sawada <[email-masked]>
Date: Fri, 7 Apr 2023 22:28:43 +0900

> second category? If so, slot_name is also used during the start

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.

> 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,

---

From: "Gregory Stark (as CFM)" <[email-masked]>
Date: Sun, 9 Apr 2023 02:02:43 -0400

On Fri, 7 Apr 2023 at 01:28, Amit Kapila <[email-masked]> wrote:
> LGTM, so pushed.

I moved this to the next CF but actually I just noticed the thread
starts with the original patch being pushed. Maybe we should just
close the CF entry? Is this further change relevant?

-- 
Gregory Stark
As Commitfest Manager

---

From: Robert Haas <[email-masked]>
Date: Mon, 10 Apr 2023 12:24:38 -0400

> apply workers and tablesync workers, so it should be in the second

I agree. I think actually the current division is quite odd. The only
parameters that strictly affect the CREATE SUBSCRIPTION command are
"connect" and "create_slot". "enabled" and "slot_name" clearly control
later behavior, because you can alter both of them later, with ALTER
SUBSCRIPTION! The "enabled" parameter is changed using different
syntax, ALTER SUBSCRIPTION .. ENABLE | DISABLE instead of ALTER
SUBSCRIPTION ... SET (enabled = true | false), which is possibly not
the best choice, but regardless of that, these parameters clearly
affect behavior later, not just at CREATE SUBSCRIPTION time.

Probably we ought to just collapse the sections together somehow, and
use the text to clarify the exact behavior as required. I definitely
disagree with the idea of moving the new parameters to the other
section -- that's clearly wrong.

-- 
Robert Haas
EDB: http://www.enterprisedb.com