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

On Tue, 3 Oct 2023 at 06:09, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Some review comments for v5.
>
> ======
> src/backend/catalog/pg_subscription.c
>
> 1. GetSubscription - comment
>
> + /* Get superuser for subscription owner */
> + sub->ownersuperuser = superuser_arg(sub->owner);
> +
>
> The comment doesn't seem very good.
>
> SUGGESTION
> /* Is the subscription owner a superuser? */

Modified

> ======
>
> 2. General - consistency
>
> Below are the code fragments using the new Subscription field.
>
> AlterSubscription_refresh:
> must_use_password = !sub->ownersuperuser && sub->passwordrequired;
>
> AlterSubscription:
> walrcv_check_conninfo(stmt->conninfo, sub->passwordrequired &&
> !sub->ownersuperuser);
>
> LogicalRepSyncTableStart:
> must_use_password = MySubscription->passwordrequired &&
> !MySubscription->ownersuperuser;
>
> run_apply_worker:
> must_use_password = MySubscription->passwordrequired &&
> !MySubscription->ownersuperuser;
>
> ~
>
> It is not a difference caused by this patch, but since you are
> modifying these lines anyway, I felt it would be better if all the
> expressions were consistent. So, in AlterSubscription_refresh IMO it
> would be better like:
>
> BEFORE
> must_use_password = !sub->ownersuperuser && sub->passwordrequired;
>
> SUGGESTION
> must_use_password = sub->passwordrequired && !sub->ownersuperuser;

Modified

Thanks for the comments, the attached v6 version patch has the changes
for the same.

Regards,
Vignesh