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

Peter Smith <smithpb2250@gmail.com>

From: Peter Smith <smithpb2250@gmail.com>
To: vignesh C <vignesh21@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, Jeff Davis <pgsql@j-davis.com>, Robert Haas <robertmhaas@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2023-10-03T00:39:09Z
Lists: pgsql-hackers
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



Commits

  1. Restart the apply worker if the privileges have been revoked.