From: | Peter Smith <smithpb2250(at)gmail(dot)com> |
---|---|
To: | vignesh C <vignesh21(at)gmail(dot)com> |
Cc: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Invalidate the subscription worker in cases where a user loses their superuser status |
Date: | 2023-09-27 01:28:19 |
Message-ID: | CAHut+PtuGCEOZ4DhaGJnr55tCc_ORy5BCVOnXzgPNQ62BFiMyw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
> > ======
> > 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.
======
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))
======
Kind Regards,
Peter Smith.
Fujitsu Australia
From | Date | Subject | |
---|---|---|---|
Next Message | Noah Misch | 2023-09-27 01:28:30 | Re: Fail hard if xlogreader.c fails on out-of-memory |
Previous Message | Japin Li | 2023-09-27 01:15:00 | Re: Could not run generate_unaccent_rules.py script when update unicode |