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