On Mon, 25 Sept 2023 at 00:32, vignesh C <vignes...@gmail.com> wrote:
>
> On Sat, 23 Sept 2023 at 11:28, Amit Kapila <amit.kapil...@gmail.com> wrote:
> >
> > On Sat, Sep 23, 2023 at 1:27 AM vignesh C <vignes...@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
From f5731c4bde69e4c7f1a8c10f795d49956d335694 Mon Sep 17 00:00:00 2001
From: Vignesh C <vignes...@gmail.com>
Date: Fri, 22 Sep 2023 15:12:23 +0530
Subject: [PATCH v2] Restart the apply worker if the subscription owner has
changed from superuser to non-superuser.
Restart the apply worker if the subscription owner has changed from
superuser to non-superuser. This is required so that the subscription
connection string gets revalidated to identify cases where the
password option is not specified as part of the connection string for
non-superuser.
---
src/backend/catalog/pg_subscription.c | 3 +++
src/backend/replication/logical/worker.c | 15 ++++++++++++---
src/include/catalog/pg_subscription.h | 1 +
src/test/subscription/t/027_nosuperuser.pl | 21 +++++++++++++++++++++
4 files changed, 37 insertions(+), 3 deletions(-)
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index d07f88ce28..bc74b695c6 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -108,6 +108,9 @@ GetSubscription(Oid subid, bool missing_ok)
Anum_pg_subscription_suborigin);
sub->origin = TextDatumGetCString(datum);
+ /* Get superuser for subscription owner */
+ sub->isownersuperuser = superuser_arg(sub->owner);
+
ReleaseSysCache(tup);
return sub;
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 597947410f..8dd41836b1 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -3942,7 +3942,8 @@ maybe_reread_subscription(void)
* The launcher will start a new worker but note that the parallel apply
* worker won't restart if the streaming option's value is changed from
* 'parallel' to any other value or the server decides not to stream the
- * in-progress transaction.
+ * in-progress transaction. Exit if the owner of the subscription has
+ * changed from superuser to a non-superuser.
*/
if (strcmp(newsub->conninfo, MySubscription->conninfo) != 0 ||
strcmp(newsub->name, MySubscription->name) != 0 ||
@@ -3952,7 +3953,8 @@ 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) ||
+ (!newsub->isownersuperuser && MySubscription->isownersuperuser))
{
if (am_parallel_apply_worker())
ereport(LOG,
@@ -4621,11 +4623,18 @@ InitializeLogRepWorker(void)
SetConfigOption("synchronous_commit", MySubscription->synccommit,
PGC_BACKEND, PGC_S_OVERRIDE);
- /* Keep us informed about subscription changes. */
+ /*
+ * Keep us informed about subscription changes or pg_authid rows.
+ * (superuser can become non-superuser.)
+ */
CacheRegisterSyscacheCallback(SUBSCRIPTIONOID,
subscription_change_cb,
(Datum) 0);
+ CacheRegisterSyscacheCallback(AUTHOID,
+ subscription_change_cb,
+ (Datum) 0);
+
if (am_tablesync_worker())
ereport(LOG,
(errmsg("logical replication table synchronization worker for subscription \"%s\", table \"%s\" has started",
diff --git a/src/include/catalog/pg_subscription.h b/src/include/catalog/pg_subscription.h
index be36c4a820..87f6f644a9 100644
--- a/src/include/catalog/pg_subscription.h
+++ b/src/include/catalog/pg_subscription.h
@@ -144,6 +144,7 @@ typedef struct Subscription
List *publications; /* List of publication names to subscribe to */
char *origin; /* Only publish data originating from the
* specified origin */
+ bool isownersuperuser; /* Is subscription owner superuser? */
} Subscription;
/* Disallow streaming in-progress transactions. */
diff --git a/src/test/subscription/t/027_nosuperuser.pl b/src/test/subscription/t/027_nosuperuser.pl
index d7a7e3ef5b..27f85537f0 100644
--- a/src/test/subscription/t/027_nosuperuser.pl
+++ b/src/test/subscription/t/027_nosuperuser.pl
@@ -104,6 +104,7 @@ for my $node ($node_publisher, $node_subscriber)
CREATE ROLE regress_admin SUPERUSER LOGIN;
CREATE ROLE regress_alice NOSUPERUSER LOGIN;
GRANT CREATE ON DATABASE postgres TO regress_alice;
+ GRANT PG_CREATE_SUBSCRIPTION TO regress_alice;
SET SESSION AUTHORIZATION regress_alice;
CREATE SCHEMA alice;
GRANT USAGE ON SCHEMA alice TO regress_admin;
@@ -303,4 +304,24 @@ GRANT SELECT ON alice.unpartitioned TO regress_alice;
expect_replication("alice.unpartitioned", 3, 17, 21,
"restoring SELECT permission permits replication to continue");
+# The apply worker should get restarted after the superuser prvileges are
+# revoked for subscription owner alice.
+grant_superuser("regress_alice");
+$node_subscriber->safe_psql(
+ 'postgres', qq(
+SET SESSION AUTHORIZATION regress_alice;
+CREATE SUBSCRIPTION regression_sub1 CONNECTION '$publisher_connstr' PUBLICATION alice;
+));
+
+# Check the subscriber log from now on.
+$offset = -s $node_subscriber->logfile;
+
+revoke_superuser("regress_alice");
+
+# 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.
+$node_subscriber->wait_for_log(qr/ERROR: ( [A-Z0-9]+:)? password is required/,
+ $offset);
+
done_testing();
--
2.34.1
