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

On Mon, 25 Sept 2023 at 00:32, vignesh C <vignesh21@gmail.com> wrote:
>
> On Sat, 23 Sept 2023 at 11:28, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Sat, Sep 23, 2023 at 1:27 AM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > >
> > > Fixed this issue by checking if the subscription owner has changed
> > > from superuser to non-superuser in case the pg_authid rows changes.
> > > The attached patch has the changes for the same.
> > >
> >
> > @@ -3952,7 +3953,9 @@ maybe_reread_subscription(void)
> >   newsub->passwordrequired != MySubscription->passwordrequired ||
> >   strcmp(newsub->origin, MySubscription->origin) != 0 ||
> >   newsub->owner != MySubscription->owner ||
> > - !equal(newsub->publications, MySubscription->publications))
> > + !equal(newsub->publications, MySubscription->publications) ||
> > + (!superuser_arg(MySubscription->owner) &&
> > + MySubscription->isownersuperuser))
> >   {
> >   if (am_parallel_apply_worker())
> >   ereport(LOG,
> > @@ -4605,6 +4608,13 @@ InitializeLogRepWorker(void)
> >   proc_exit(0);
> >   }
> >
> > + /*
> > + * Fetch subscription owner is a superuser. This value will be later
> > + * checked to see when there is any change with this role and the worker
> > + * will be restarted if required.
> > + */
> > + MySubscription->isownersuperuser = superuser_arg(MySubscription->owner);
> >
> > Why didn't you filled this parameter in GetSubscription() like other
> > parameters? If we do that then the comparison of first change in your
> > patch will look similar to all other comparisons.
>
> I felt this variable need not be added to the pg_subscription catalog
> table, instead we could save the state of subscription owner when the
> worker is started and compare this value during invalidations. As this
> information is added only to the memory Subscription structure and not
> added to the catalog FormData_pg_subscription, the checking is
> slightly different in this case. Also since this variable will be used
> only within the worker, I felt we need not add it to the catalog.

On further thinking I felt getting superuser can be moved to
GetSubscription which will make the code consistent with other
checking and will fix the comment Amit had given, the attached version
has the change for the same.

Regards,
Vignesh