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

On Sat, 7 Oct 2023 at 08:12, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Oct 3, 2023 at 12:12 PM vignesh C <vignesh21(at)gmail(dot)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.

Modified

> 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

Modified

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

Modified

The attached v7 version patch has the changes for the same.

Regards,
Vignesh