From: | vignesh C <vignesh21(at)gmail(dot)com> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | Peter Smith <smithpb2250(at)gmail(dot)com>, Jeff Davis <pgsql(at)j-davis(dot)com>, Robert Haas <robertmhaas(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-28 11:24:58 |
Message-ID: | CALDaNm3NHQxuExnssTG9CMcYFMw3BcvSkYvGvPnF6jpx2kfX2A@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, 27 Sept 2023 at 12:28, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> 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.
Modified
> 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?
I felt since password_required functionality is there in PG16, we
should fix this in PG16 too. I have checked that password_required
functionality is not there in PG15, so no need to make any change in
PG15
The updated patch has the changes for the same.
Regards,
Vignesh
Attachment | Content-Type | Size |
---|---|---|
v4-0001-Restart-the-apply-worker-if-the-subscription-owne_PG16.patch | text/x-patch | 7.5 KB |
v4-0001-Restart-the-apply-worker-if-the-subscription-owne.patch | text/x-patch | 7.4 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Ranier Vilela | 2023-09-28 12:00:39 | Re: Avoid a possible out-of-bounds access (src/backend/optimizer/util/relnode.c) |
Previous Message | David Rowley | 2023-09-28 11:23:00 | Is it worth adding Assert(false) for unknown paths in print_path()? |