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

On Wed, 27 Sept 2023 at 06:58, Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Tue, Sep 26, 2023 at 11:57 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Tue, 26 Sept 2023 at 13:03, Peter Smith <smithpb2250@gmail.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.
>
> > > ======
> > > src/include/catalog/pg_subscription.h
> > >
> > > 2.
> > >   char    *origin; /* Only publish data originating from the
> > >   * specified origin */
> > > + bool isownersuperuser; /* Is subscription owner superuser? */
> > >  } Subscription;
> > >
> > >
> > > Is a new Subscription field member really necessary? The Subscription
> > > already has 'owner' -- why doesn't function
> > > maybe_reread_subscription() just check:
> > >
> > > (!superuser_arg(sub->owner) && superuser_arg(MySubscription->owner))
> >
> > We need the new variable so that we store this value when the worker
> > is started and check the current value with the value that was when
> > the worker was started. Since we need the value of the superuser
> > status when the worker is started, I feel this variable is required.
> >
>
> OK. In that case, then shouldn't the patch replace the other
> superuser_arg() code already in function run_apply_worker() to make
> use of this variable? Otherwise, there are 2 ways of getting the same
> information.

Modified

> ======
> src/test/subscription/t/027_nosuperuser.pl
>
> I am suspicious that something may be wrong with either the code or
> the test because while experimenting, I accidentally found that even
> if I *completely* remove the important change below, the TAP test will
> still pass anyway.
>
> - !equal(newsub->publications, MySubscription->publications))
> + !equal(newsub->publications, MySubscription->publications) ||
> + (!newsub->isownersuperuser && MySubscription->isownersuperuser))

The test did not wait for subscription to sync, as the owner has
changed to non-superuser the tablesync were getting started later and
failing with this error in HEAD. Now I have added wait for
subscription to sync, so the error will always come from apply worker
restart and also when the test is run on HEAD we can notice that the
error message does not appear in the log.

The v3 patch attached at [1] has the changes for the same.
[1] - https://www.postgresql.org/message-id/CALDaNm2JwPmX0rhkTyjGRQmZjwbKax%3D3Ytgw2KY9msDPPNGmBg%40mail.gmail.com

Regards,
Vignesh