Обсуждение: Invalidate the subscription worker in cases where a user loses their superuser status

Hi,

The subscription worker was not getting invalidated when the
subscription owner changed from superuser to non-superuser. Here is a
test case for the same:
Publisher:
   CREATE USER repl REPLICATION PASSWORD 'secret';
   CREATE TABLE t(i INT);
   INSERT INTO t VALUES(1);
   GRANT SELECT ON t TO repl;
   CREATE PUBLICATION p1 FOR TABLE t;

Subscriber (has a PGPASSFILE for user "repl"):
   CREATE USER u1 SUPERUSER;
   CREATE TABLE t(i INT);
   ALTER TABLE t OWNER TO u1;
   -- no password specified
   CREATE SUBSCRIPTION s1
     CONNECTION 'dbname=postgres host=127.0.0.1 port=5432 user=repl'
     PUBLICATION p1;

   ALTER USER u1 NOSUPERUSER: -- Change u1 user to non-superuser

Publisher:
INSERT INTO t VALUES(1);

Subscriber:
SELECT COUNT(*) FROM t; -- should have been 1 but is 2, the apply
worker has not exited after changing from superuser to non-superuser.

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.
Thanks to Jeff Davis for identifying this issue and reporting it at [1].

[1] - https://www.postgresql.org/message-id/5dff4caf26f45ce224a33a5e18e110b93a351b2f.camel%40j-davis.com

Regards,
Vignesh

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.

--
With Regards,
Amit Kapila.

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.

Regards,
Vignesh

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

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?

======
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))

======
src/test/subscription/t/027_nosuperuser.pl

3.
# The apply worker should get restarted after the superuser prvileges are
# revoked for subscription owner alice.

typo

/prvileges/privileges/

~

4.
+# After the user becomes non-superuser the apply worker should be restarted and
+# it should fail with 'password is required' error as password option is not
+# part of the connection string.

/as password option/because the password option/

======
Kind Regards,
Peter Smith.
Fujitsu Australia.

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.

> ======
> 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.

> ======
> src/test/subscription/t/027_nosuperuser.pl
>
> 3.
> # The apply worker should get restarted after the superuser prvileges are
> # revoked for subscription owner alice.
>
> typo
>
> /prvileges/privileges/
>.
> ~

I will change this in the next version

> 4.
> +# After the user becomes non-superuser the apply worker should be restarted and
> +# it should fail with 'password is required' error as password option is not
> +# part of the connection string.
>
> /as password option/because the password option/

I will change this in the next version

Regards,
Vignesh

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.

======
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

On Tue, 26 Sept 2023 at 13:03, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are some comments for patch v2-0001.
> ======
> src/test/subscription/t/027_nosuperuser.pl
>
> 3.
> # The apply worker should get restarted after the superuser prvileges are
> # revoked for subscription owner alice.
>
> typo
>
> /prvileges/privileges/

Modified

> ~
>
> 4.
> +# After the user becomes non-superuser the apply worker should be restarted and
> +# it should fail with 'password is required' error as password option is not
> +# part of the connection string.
>
> /as password option/because the password option/

Modified

The attached v3 version patch has the changes for the same.

Regards,
Vignesh

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

On Wed, Sep 27, 2023 at 6:58 AM 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.
>

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.

On Wed, 27 Sept 2023 at 12:28, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Sep 27, 2023 at 6:58 AM 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.
> >
>
> 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

On Wed, Sep 27, 2023 at 2:58 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 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.

The superuser status of the subscription owner is definitely *not* a
parameter of the subscription, so it doesn't seem like the same
message is appropriate.

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

I guess it depends on whether we think this is a bug. I think you
could argue it either way.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Thu, 2023-09-28 at 11:34 -0400, Robert Haas wrote:
> I guess it depends on whether we think this is a bug. I think you
> could argue it either way.

I'd suggest backporting to 16 unless there's some kind of difficulty.
Otherwise we have a minor difference in behavior for no reason.

Regards,
    Jeff Davis

Some minor review comments for v4-0001:

======
src/backend/replication/logical/worker.c

1.
+ /*
+ * Exit if the owner of the subscription has changed from superuser to a
+ * non-superuser.
+ */
+ if (!newsub->isownersuperuser && MySubscription->isownersuperuser)
+ {
+ if (am_parallel_apply_worker())
+ ereport(LOG,
+ errmsg("logical replication parallel apply worker for subscription
\"%s\" will stop because subscription owner has become non-superuser",
+    MySubscription->name));
+ else
+ ereport(LOG,
+ errmsg("logical replication worker for subscription \"%s\" will
restart because subscription owner has become non-superuser",
+    MySubscription->name));
+
+ apply_worker_exit();
+ }
+

/because subscription owner has become non-superuser/because the
subscription owner has become a non-superuser/  (in 2 places)

======
src/include/catalog/pg_subscription.h

2.
  char    *origin; /* Only publish data originating from the
  * specified origin */
+ bool isownersuperuser; /* Is subscription owner superuser? */
 } Subscription;

~

2a.
Would it be better to put this new field adjacent to the existing
'owner' field, since they kind of belong together?

~

2b.
None of the other bool fields here has an 'is' prefix, so you could
consider a shorter field name, like 'ownersuperuser' or
'superuserowner', etc.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Fri, 29 Sept 2023 at 04:55, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Some minor review comments for v4-0001:
>
> ======
> src/backend/replication/logical/worker.c
>
> 1.
> + /*
> + * Exit if the owner of the subscription has changed from superuser to a
> + * non-superuser.
> + */
> + if (!newsub->isownersuperuser && MySubscription->isownersuperuser)
> + {
> + if (am_parallel_apply_worker())
> + ereport(LOG,
> + errmsg("logical replication parallel apply worker for subscription
> \"%s\" will stop because subscription owner has become non-superuser",
> +    MySubscription->name));
> + else
> + ereport(LOG,
> + errmsg("logical replication worker for subscription \"%s\" will
> restart because subscription owner has become non-superuser",
> +    MySubscription->name));
> +
> + apply_worker_exit();
> + }
> +
>
> /because subscription owner has become non-superuser/because the
> subscription owner has become a non-superuser/  (in 2 places)

Modified

> ======
> src/include/catalog/pg_subscription.h
>
> 2.
>   char    *origin; /* Only publish data originating from the
>   * specified origin */
> + bool isownersuperuser; /* Is subscription owner superuser? */
>  } Subscription;
>
> ~
>
> 2a.
> Would it be better to put this new field adjacent to the existing
> 'owner' field, since they kind of belong together?

Modified

> ~
>
> 2b.
> None of the other bool fields here has an 'is' prefix, so you could
> consider a shorter field name, like 'ownersuperuser' or
> 'superuserowner', etc.

Modified

The attached v5 version patch has the changes for the same.

Regards,
Vignesh

Some review comments for v5.

======
src/backend/catalog/pg_subscription.c

1. GetSubscription - comment

+ /* Get superuser for subscription owner */
+ sub->ownersuperuser = superuser_arg(sub->owner);
+

The comment doesn't seem very good.

SUGGESTION
/* Is the subscription owner a superuser? */

======

2. General - consistency

Below are the code fragments using the new Subscription field.

AlterSubscription_refresh:
must_use_password = !sub->ownersuperuser && sub->passwordrequired;

AlterSubscription:
walrcv_check_conninfo(stmt->conninfo, sub->passwordrequired &&
!sub->ownersuperuser);

LogicalRepSyncTableStart:
must_use_password = MySubscription->passwordrequired &&
!MySubscription->ownersuperuser;

run_apply_worker:
must_use_password = MySubscription->passwordrequired &&
!MySubscription->ownersuperuser;

~

It is not a difference caused by this patch, but since you are
modifying these lines anyway, I felt it would be better if all the
expressions were consistent. So, in AlterSubscription_refresh IMO it
would be better like:

BEFORE
must_use_password = !sub->ownersuperuser && sub->passwordrequired;

SUGGESTION
must_use_password = sub->passwordrequired && !sub->ownersuperuser;

======

Other than those trivial things, v5 LGTM.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Tue, 3 Oct 2023 at 06:09, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Some review comments for v5.
>
> ======
> src/backend/catalog/pg_subscription.c
>
> 1. GetSubscription - comment
>
> + /* Get superuser for subscription owner */
> + sub->ownersuperuser = superuser_arg(sub->owner);
> +
>
> The comment doesn't seem very good.
>
> SUGGESTION
> /* Is the subscription owner a superuser? */

Modified

> ======
>
> 2. General - consistency
>
> Below are the code fragments using the new Subscription field.
>
> AlterSubscription_refresh:
> must_use_password = !sub->ownersuperuser && sub->passwordrequired;
>
> AlterSubscription:
> walrcv_check_conninfo(stmt->conninfo, sub->passwordrequired &&
> !sub->ownersuperuser);
>
> LogicalRepSyncTableStart:
> must_use_password = MySubscription->passwordrequired &&
> !MySubscription->ownersuperuser;
>
> run_apply_worker:
> must_use_password = MySubscription->passwordrequired &&
> !MySubscription->ownersuperuser;
>
> ~
>
> It is not a difference caused by this patch, but since you are
> modifying these lines anyway, I felt it would be better if all the
> expressions were consistent. So, in AlterSubscription_refresh IMO it
> would be better like:
>
> BEFORE
> must_use_password = !sub->ownersuperuser && sub->passwordrequired;
>
> SUGGESTION
> must_use_password = sub->passwordrequired && !sub->ownersuperuser;

Modified

Thanks for the comments, the attached v6 version patch has the changes
for the same.

Regards,
Vignesh

On Tue, Oct 3, 2023 at 5:42 PM vignesh C <vignesh21@gmail.com> wrote:
>
> Thanks for the comments, the attached v6 version patch has the changes
> for the same.
>

v6 LGTM.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Wed, 4 Oct 2023 at 16:56, Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Tue, Oct 3, 2023 at 5:42 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Thanks for the comments, the attached v6 version patch has the changes
> > for the same.
> >
>
> v6 LGTM.
>
I have verified the patch and it is working fine for me.

On Wed, 4 Oct 2023 at 17:01, Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> On Wed, 4 Oct 2023 at 16:56, Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > On Tue, Oct 3, 2023 at 5:42 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > Thanks for the comments, the attached v6 version patch has the changes
> > > for the same.
> > >
> >
> > v6 LGTM.
> >
> I have verified the patch and it is working fine for me.

I have created the commitfest entry for this at:
https://commitfest.postgresql.org/45/4595/

Regards.
Vignesh

On Tue, Oct 3, 2023 at 12:12 PM vignesh C <vignesh21@gmail.com> wrote:
>
> Thanks for the comments, the attached v6 version patch has the changes
> for the same.
>

Few comments:
=============
1.
/* Is the use of a password mandatory? */
  must_use_password = MySubscription->passwordrequired &&
- !superuser_arg(MySubscription->owner);
+ !MySubscription->ownersuperuser;

- /* Note that the superuser_arg call can access the DB */
  CommitTransactionCommand();

We can call CommitTransactionCommand() before the above check now. It
was done afterward to invoke superuser_arg(), so, if that requirement
is changed, we no longer need to keep the transaction open for a
longer time. Please check other places for similar changes.

2.
+ ereport(LOG,
+ errmsg("logical replication worker for subscription \"%s\" will
restart because the subscription owner has become a non-superuser",

How about something on the below lines?
logical replication worker for subscription \"%s\" will restart
because superuser privileges have been revoked for the subscription
owner
OR
logical replication worker for subscription \"%s\" will restart
because the subscription owner's superuser privileges have been
revoked

3.
- /* Keep us informed about subscription changes. */
+ /*
+ * Keep us informed about subscription changes or pg_authid rows.
+ * (superuser can become non-superuser.)
+ */

Let's slightly change the comment to: "Keep us informed about
subscription or role changes. Note that role's superuser privilege can
be revoked."

--
With Regards,
Amit Kapila.

On Sat, 7 Oct 2023 at 08:12, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Oct 3, 2023 at 12:12 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Thanks for the comments, the attached v6 version patch has the changes
> > for the same.
> >
>
> Few comments:
> =============
> 1.
> /* Is the use of a password mandatory? */
>   must_use_password = MySubscription->passwordrequired &&
> - !superuser_arg(MySubscription->owner);
> + !MySubscription->ownersuperuser;
>
> - /* Note that the superuser_arg call can access the DB */
>   CommitTransactionCommand();
>
> We can call CommitTransactionCommand() before the above check now. It
> was done afterward to invoke superuser_arg(), so, if that requirement
> is changed, we no longer need to keep the transaction open for a
> longer time. Please check other places for similar changes.

Modified

> 2.
> + ereport(LOG,
> + errmsg("logical replication worker for subscription \"%s\" will
> restart because the subscription owner has become a non-superuser",
>
> How about something on the below lines?
> logical replication worker for subscription \"%s\" will restart
> because superuser privileges have been revoked for the subscription
> owner
> OR
> logical replication worker for subscription \"%s\" will restart
> because the subscription owner's superuser privileges have been
> revoked

Modified

> 3.
> - /* Keep us informed about subscription changes. */
> + /*
> + * Keep us informed about subscription changes or pg_authid rows.
> + * (superuser can become non-superuser.)
> + */
>
> Let's slightly change the comment to: "Keep us informed about
> subscription or role changes. Note that role's superuser privilege can
> be revoked."

Modified

The attached v7 version patch has the changes for the same.

Regards,
Vignesh

On Sun, Oct 8, 2023 at 8:22 AM vignesh C <vignesh21@gmail.com> wrote:
>

--- a/src/include/catalog/pg_subscription.h
+++ b/src/include/catalog/pg_subscription.h
@@ -127,6 +127,7 @@ typedef struct Subscription
  * skipped */
  char    *name; /* Name of the subscription */
  Oid owner; /* Oid of the subscription owner */
+ bool ownersuperuser; /* Is the subscription owner a superuser? */
  bool enabled; /* Indicates if the subscription is enabled */
  bool binary; /* Indicates if the subscription wants data in
  * binary format */

We normally don't change the exposed structure in back branches as
that poses a risk of breaking extensions. In this case, if we want, we
can try to squeeze some padding space or we even can fix it without
introducing a new member. OTOH, it is already debatable whether to fix
it in back branches, so we can even commit this patch just in HEAD.

Thoughts?

--
With Regards,
Amit Kapila.

On Thu, 12 Oct 2023 at 11:10, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sun, Oct 8, 2023 at 8:22 AM vignesh C <vignesh21@gmail.com> wrote:
> >
>
> --- a/src/include/catalog/pg_subscription.h
> +++ b/src/include/catalog/pg_subscription.h
> @@ -127,6 +127,7 @@ typedef struct Subscription
>   * skipped */
>   char    *name; /* Name of the subscription */
>   Oid owner; /* Oid of the subscription owner */
> + bool ownersuperuser; /* Is the subscription owner a superuser? */
>   bool enabled; /* Indicates if the subscription is enabled */
>   bool binary; /* Indicates if the subscription wants data in
>   * binary format */
>
> We normally don't change the exposed structure in back branches as
> that poses a risk of breaking extensions. In this case, if we want, we
> can try to squeeze some padding space or we even can fix it without
> introducing a new member. OTOH, it is already debatable whether to fix
> it in back branches, so we can even commit this patch just in HEAD.

I too feel we can commit this patch only in HEAD.

Regards,
Vignesh

On Fri, Oct 13, 2023 at 10:04 AM vignesh C <vignesh21@gmail.com> wrote:
>
> On Thu, 12 Oct 2023 at 11:10, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Sun, Oct 8, 2023 at 8:22 AM vignesh C <vignesh21@gmail.com> wrote:
> > >
> >
> > --- a/src/include/catalog/pg_subscription.h
> > +++ b/src/include/catalog/pg_subscription.h
> > @@ -127,6 +127,7 @@ typedef struct Subscription
> >   * skipped */
> >   char    *name; /* Name of the subscription */
> >   Oid owner; /* Oid of the subscription owner */
> > + bool ownersuperuser; /* Is the subscription owner a superuser? */
> >   bool enabled; /* Indicates if the subscription is enabled */
> >   bool binary; /* Indicates if the subscription wants data in
> >   * binary format */
> >
> > We normally don't change the exposed structure in back branches as
> > that poses a risk of breaking extensions. In this case, if we want, we
> > can try to squeeze some padding space or we even can fix it without
> > introducing a new member. OTOH, it is already debatable whether to fix
> > it in back branches, so we can even commit this patch just in HEAD.
>
> I too feel we can commit this patch only in HEAD.
>

Fair enough. I'll wait till early next week (say till Monday EOD) to
see if anyone thinks otherwise and push this patch to HEAD after some
more testing and review.

--
With Regards,
Amit Kapila.

On Fri, Oct 13, 2023 at 11:08 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Oct 13, 2023 at 10:04 AM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Thu, 12 Oct 2023 at 11:10, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Sun, Oct 8, 2023 at 8:22 AM vignesh C <vignesh21@gmail.com> wrote:
> > > >
> > >
> > > --- a/src/include/catalog/pg_subscription.h
> > > +++ b/src/include/catalog/pg_subscription.h
> > > @@ -127,6 +127,7 @@ typedef struct Subscription
> > >   * skipped */
> > >   char    *name; /* Name of the subscription */
> > >   Oid owner; /* Oid of the subscription owner */
> > > + bool ownersuperuser; /* Is the subscription owner a superuser? */
> > >   bool enabled; /* Indicates if the subscription is enabled */
> > >   bool binary; /* Indicates if the subscription wants data in
> > >   * binary format */
> > >
> > > We normally don't change the exposed structure in back branches as
> > > that poses a risk of breaking extensions. In this case, if we want, we
> > > can try to squeeze some padding space or we even can fix it without
> > > introducing a new member. OTOH, it is already debatable whether to fix
> > > it in back branches, so we can even commit this patch just in HEAD.
> >
> > I too feel we can commit this patch only in HEAD.
> >
>
> Fair enough. I'll wait till early next week (say till Monday EOD) to
> see if anyone thinks otherwise and push this patch to HEAD after some
> more testing and review.
>

Pushed.

--
With Regards,
Amit Kapila.

On Tue, 17 Oct 2023 at 14:17, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Oct 13, 2023 at 11:08 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Oct 13, 2023 at 10:04 AM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > On Thu, 12 Oct 2023 at 11:10, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > On Sun, Oct 8, 2023 at 8:22 AM vignesh C <vignesh21@gmail.com> wrote:
> > > > >
> > > >
> > > > --- a/src/include/catalog/pg_subscription.h
> > > > +++ b/src/include/catalog/pg_subscription.h
> > > > @@ -127,6 +127,7 @@ typedef struct Subscription
> > > >   * skipped */
> > > >   char    *name; /* Name of the subscription */
> > > >   Oid owner; /* Oid of the subscription owner */
> > > > + bool ownersuperuser; /* Is the subscription owner a superuser? */
> > > >   bool enabled; /* Indicates if the subscription is enabled */
> > > >   bool binary; /* Indicates if the subscription wants data in
> > > >   * binary format */
> > > >
> > > > We normally don't change the exposed structure in back branches as
> > > > that poses a risk of breaking extensions. In this case, if we want, we
> > > > can try to squeeze some padding space or we even can fix it without
> > > > introducing a new member. OTOH, it is already debatable whether to fix
> > > > it in back branches, so we can even commit this patch just in HEAD.
> > >
> > > I too feel we can commit this patch only in HEAD.
> > >
> >
> > Fair enough. I'll wait till early next week (say till Monday EOD) to
> > see if anyone thinks otherwise and push this patch to HEAD after some
> > more testing and review.
> >
>
> Pushed.

Thanks for committing this.

Regards,
Vignesh

On Tue, 2023-10-17 at 14:17 +0530, Amit Kapila wrote:
> > Fair enough. I'll wait till early next week (say till Monday EOD)
> > to
> > see if anyone thinks otherwise and push this patch to HEAD after
> > some
> > more testing and review.
> >
>
> Pushed.

There was a brief discussion on backporting this here:

https://www.postgresql.org/message-id/CA%2BTgmob2mYpaUMT7aoFOukYTrpxt-WdgcitJnsjWhufnbDWEeg%40mail.gmail.com

It looks like you opted not to backport it. Was there a reason for
that? The only risky thing I see there is a change in the Subscription
structure -- I suppose that could be used by extensions?

(I am fine with it not being backported, but I was inclined to think it
should be backported.)

Regards,
    Jeff Davis

On Sat, Jan 13, 2024 at 2:02 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Tue, 2023-10-17 at 14:17 +0530, Amit Kapila wrote:
> > > Fair enough. I'll wait till early next week (say till Monday EOD)
> > > to
> > > see if anyone thinks otherwise and push this patch to HEAD after
> > > some
> > > more testing and review.
> > >
> >
> > Pushed.
>
> There was a brief discussion on backporting this here:
>
> https://www.postgresql.org/message-id/CA%2BTgmob2mYpaUMT7aoFOukYTrpxt-WdgcitJnsjWhufnbDWEeg%40mail.gmail.com
>
> It looks like you opted not to backport it. Was there a reason for
> that? The only risky thing I see there is a change in the Subscription
> structure -- I suppose that could be used by extensions?
>

Right, the same is pointed out by me in an email [1].

> (I am fine with it not being backported, but I was inclined to think it
> should be backported.)
>

I don't mind backporting it if you think so but we need to ensure that
we don't break any extensions.

[1] - https://www.postgresql.org/message-id/CAA4eK1JztFkYeVANuH0Ja_c3zqDjTyz0j15xQqwCDRPokYhWgw%40mail.gmail.com

--
With Regards,
Amit Kapila.