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

On Wed, Sep 27, 2023 at 6:58 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Tue, Sep 26, 2023 at 11:57 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > On Tue, 26 Sept 2023 at 13:03, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > >
> > > Here are some comments for patch v2-0001.
> > >
> > > ======
> > > src/backend/replication/logical/worker.c
> > >
> > > 1. maybe_reread_subscription
> > >
> > > ereport(LOG,
> > > (errmsg("logical replication worker for subscription \"%s\"
> > > will restart because of a parameter change",
> > > MySubscription->name)));
> > >
> > > Is this really a "parameter change" though? It might be a stretch to
> > > call the user role change a subscription parameter change. Perhaps
> > > this case needs its own LOG message?
> >
> > When I was doing this change the same thought had come to my mind too
> > but later I noticed that in case of owner change there was no separate
> > log message. Since superuser check is somewhat similar to owner
> > change, I felt no need to make any change for this.
> >
>
> Yeah, I had seen the same already before my comment. Anyway, YMMV.
>

But OTOH, the owner of the subscription can be changed by the Alter
Subscription command whereas superuser status can't be changed. I
think we should consider changing the message for this case.

BTW, do we want to backpatch this patch? I think we should backatch to
PG16 as it impacts password_required functionality. Before this patch
even if the subscription owner's superuser status is lost, it won't
use a password for connection till the server gets restarted or the
apply worker gets restarted due to some other reason. What do you
think?

Adding Jeff and Robert to see what is their opinion on whether we
should backpatch this or not.

--
With Regards,
Amit Kapila.