Re: Questions about the new subscription parameter: password_required

On Fri, 2023-10-13 at 11:18 +0200, Benoit Lobréau wrote:
> I tried adding a section in "Logical Replication > Subscription" with
> the text you suggested and links in the CREATE / ALTER SUBSRIPTION
> commands.
>
> Is it better ?

Minor comments:

* Use possessive "its" instead of the contraction, i.e. "before
transferring its ownership".
* I like that docs cover the case where a password is specified, but
the remote server doesn't require one. But the warning is the wrong
place to explain that, it should be in the main behavioral description
in 31.2.2.
* The warning feels like it has too many negatives and confused me at
first. I struggled myself a bit to come up with something less
confusing, but perhaps less is more: "Ensure that password_required is
properly set before transferring ownership of a subscription to a non-
superuser, otherwise the subscription may start to fail."
* Missing space in the warning after "password_required = true"
* Mention that a non-superuser-owned subscription with
password_required = false is partially locked down, e.g. the owner
can't change the connection string any more.
* 31.2.2 could probably be in the CREATE SUBSCRIPTION docs instead,
and linked from the ALTER docs. That's fairly normal for other commands
and I'm not sure there needs to be a separate section in logical
replication. I don't have a strong opinion here.

I like the changes; this is a big improvement. I'll leave it to Robert
to commit it, so that he can ensure it matches how he expected the
feature to be used and sufficiently covers the behavioral aspects.

Regards,
Jeff Davis