Re: Invalidate the subscription worker in cases where a user loses their superuser status

On Tue, Oct 3, 2023 at 12:12 PM vignesh C <vignesh21@gmail.com> wrote:
>
> Thanks for the comments, the attached v6 version patch has the changes
> for the same.
>

Few comments:
=============
1.
/* Is the use of a password mandatory? */
  must_use_password = MySubscription->passwordrequired &&
- !superuser_arg(MySubscription->owner);
+ !MySubscription->ownersuperuser;

- /* Note that the superuser_arg call can access the DB */
  CommitTransactionCommand();

We can call CommitTransactionCommand() before the above check now. It
was done afterward to invoke superuser_arg(), so, if that requirement
is changed, we no longer need to keep the transaction open for a
longer time. Please check other places for similar changes.

2.
+ ereport(LOG,
+ errmsg("logical replication worker for subscription \"%s\" will
restart because the subscription owner has become a non-superuser",

How about something on the below lines?
logical replication worker for subscription \"%s\" will restart
because superuser privileges have been revoked for the subscription
owner
OR
logical replication worker for subscription \"%s\" will restart
because the subscription owner's superuser privileges have been
revoked

3.
- /* Keep us informed about subscription changes. */
+ /*
+ * Keep us informed about subscription changes or pg_authid rows.
+ * (superuser can become non-superuser.)
+ */

Let's slightly change the comment to: "Keep us informed about
subscription or role changes. Note that role's superuser privilege can
be revoked."

-- 
With Regards,
Amit Kapila.