pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feed[16] ALTER SUBSCRIPTION ... SET (run_as_owner = ...) is a no-op
6+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2023-09-13 f062cddafe a81e5516fa Fix the ALTER SUBSCRIPTION to reflect the change in run_as_owner option.
* [16] ALTER SUBSCRIPTION ... SET (run_as_owner = ...) is a no-op
@ 2023-09-09 20:42 Jeff Davis <pgsql@j-davis.com>
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Davis @ 2023-09-09 20:42 UTC (permalink / raw)
To: pgsql-bugs; +Cc: Robert Haas <robertmhaas@gmail.com>
Repro:
ALTER SUBSCRIPTION s1 SET (run_as_owner = true);
SELECT subrunasowner FROM pg_subscription WHERE subname='s1';
subrunasowner
---------------
f
(1 row)
It also looks like a change to that field may not cause the
subscription worker to restart. It would be good to add a test for that
case.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 6+ messages in thread
* RE: [16] ALTER SUBSCRIPTION ... SET (run_as_owner = ...) is a no-op
@ 2023-09-11 03:26 Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 1 reply; 6+ messages in thread
From: Zhijie Hou (Fujitsu) @ 2023-09-11 03:26 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; pgsql-bugs; +Cc: Robert Haas <robertmhaas@gmail.com>
On Sunday, September 10, 2023 4:43 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
>
> Repro:
> ALTER SUBSCRIPTION s1 SET (run_as_owner = true);
> SELECT subrunasowner FROM pg_subscription WHERE subname='s1';
> subrunasowner
> ---------------
> f
> (1 row)
>
Thanks for reporting. I can also reproduce the issue. I think it's because we
didn't reflect the option change on catalog. Here is a small patch 0001 to fix it.
> It also looks like a change to that field may not cause the subscription worker to
> restart. It would be good to add a test for that case.
Currently, the changes on run_as_owner won't cause the worker to restart
because we don't need to rebuild the connection in this case. The option change
will be caught by apply worker in next loop and the later changes will be
applied using the new option. the 0002 patch adds a test to verfiy it, just to
show how it behaves.
Best Regards,
Hou zj
Attachments: download all (2 files, .tar.gz)
0002-add-a-test-to-verify-the-change-of-runasowner-take-e.patch (application/octet-stream, 2.1K) download
From 2149e3f716a16f0026057f9c1c01f0273253f22c Mon Sep 17 00:00:00 2001
From: Hou Zhijie <houzj.fnst@cn.fujitsu.com>
Date: Mon, 11 Sep 2023 11:10:04 +0800
Subject: [PATCH 2/2] add a test to verify the change of runasowner take effect
in apply worker
---
.../subscription/t/033_run_as_table_owner.pl | 29 ++++++++++++++++++-
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/src/test/subscription/t/033_run_as_table_owner.pl b/src/test/subscription/t/033_run_as_table_owner.pl
index 9de3c04a0c..a40b6f5162 100644
--- a/src/test/subscription/t/033_run_as_table_owner.pl
+++ b/src/test/subscription/t/033_run_as_table_owner.pl
@@ -193,6 +193,33 @@ GRANT regress_alice TO regress_admin WITH INHERIT TRUE, SET FALSE;
expect_replication("alice.unpartitioned", 3, 7, 13,
"with INHERIT but not SET ROLE can replicate");
+# Remove all privileges again.
+$node_subscriber->safe_psql(
+ 'postgres', qq(
+SET SESSION AUTHORIZATION regress_alice;
+REVOKE ALL PRIVILEGES ON alice.unpartitioned FROM regress_admin;
+RESET SESSION AUTHORIZATION;
+GRANT regress_alice TO regress_admin WITH INHERIT FALSE, SET TRUE;
+));
+
+# We can't yet replicate an INSERT because we revoked all privileges.
+publish_insert("alice.unpartitioned", 14);
+expect_failure(
+ "alice.unpartitioned",
+ 3,
+ 7,
+ 13,
+ qr/ERROR: ( [A-Z0-9]+:)? permission denied for table unpartitioned/msi,
+ "with no privileges cannot replicate");
+
+$node_subscriber->safe_psql(
+ 'postgres', qq(
+ALTER SUBSCRIPTION admin_sub SET (run_as_owner = false);
+));
+
+expect_replication("alice.unpartitioned", 4, 7, 14,
+ "can replicate after setting run_as_owner to false");
+
# Remove the subscrition and truncate the table for the initial data sync
# tests.
$node_subscriber->safe_psql(
@@ -222,7 +249,7 @@ ALTER SUBSCRIPTION admin_sub ENABLE;
# Because the initial data sync is working as the table owner, all
# data should be copied.
$node_subscriber->wait_for_subscription_sync($node_publisher, 'admin_sub');
-expect_replication("alice.unpartitioned", 3, 7, 13,
+expect_replication("alice.unpartitioned", 4, 7, 14,
"table owner can do the initial data copy");
done_testing();
--
2.30.0.windows.2
0001-Ensure-that-the-update-on-run_as_owner-is-reflected-.patch (application/octet-stream, 4.0K) download
From 09f66bc083e463df2fdf37ae147498bcb6799255 Mon Sep 17 00:00:00 2001
From: Hou Zhijie <houzj.fnst@cn.fujitsu.com>
Date: Mon, 11 Sep 2023 10:23:51 +0800
Subject: [PATCH 1/2] Ensure that the update on run_as_owner is reflected in
the catalog.
Previously, the update on run_as_owner was not properly applied. This patch
makes the changes to the "run_as_owner" subscription option take effect as
expected.
---
src/backend/commands/subscriptioncmds.c | 7 +++++++
src/test/regress/expected/subscription.out | 4 +++-
src/test/regress/sql/subscription.sql | 2 ++
3 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 34d881fd94..6fe111e98d 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1204,6 +1204,13 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
= true;
}
+ if (IsSet(opts.specified_opts, SUBOPT_RUN_AS_OWNER))
+ {
+ values[Anum_pg_subscription_subrunasowner - 1] =
+ BoolGetDatum(opts.runasowner);
+ replaces[Anum_pg_subscription_subrunasowner - 1] = true;
+ }
+
if (IsSet(opts.specified_opts, SUBOPT_ORIGIN))
{
values[Anum_pg_subscription_suborigin - 1] =
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 3c1a0869ec..b15eddbff3 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -155,14 +155,16 @@ ALTER SUBSCRIPTION regress_testsub SET PUBLICATION testpub2, testpub3 WITH (refr
ALTER SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist2';
ALTER SUBSCRIPTION regress_testsub SET (slot_name = 'newname');
ALTER SUBSCRIPTION regress_testsub SET (password_required = false);
+ALTER SUBSCRIPTION regress_testsub SET (run_as_owner = true);
\dRs+
List of subscriptions
Name | Owner | Enabled | Publication | Binary | Streaming | Two-phase commit | Disable on error | Origin | Password required | Run as owner? | Synchronous commit | Conninfo | Skip LSN
-----------------+---------------------------+---------+---------------------+--------+-----------+------------------+------------------+--------+-------------------+---------------+--------------------+------------------------------+----------
- regress_testsub | regress_subscription_user | f | {testpub2,testpub3} | f | off | d | f | any | f | f | off | dbname=regress_doesnotexist2 | 0/0
+ regress_testsub | regress_subscription_user | f | {testpub2,testpub3} | f | off | d | f | any | f | t | off | dbname=regress_doesnotexist2 | 0/0
(1 row)
ALTER SUBSCRIPTION regress_testsub SET (password_required = true);
+ALTER SUBSCRIPTION regress_testsub SET (run_as_owner = false);
-- fail
ALTER SUBSCRIPTION regress_testsub SET (slot_name = '');
ERROR: replication slot name "" is too short
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 55d7dbc9ab..444e563ff3 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -94,9 +94,11 @@ ALTER SUBSCRIPTION regress_testsub SET PUBLICATION testpub2, testpub3 WITH (refr
ALTER SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist2';
ALTER SUBSCRIPTION regress_testsub SET (slot_name = 'newname');
ALTER SUBSCRIPTION regress_testsub SET (password_required = false);
+ALTER SUBSCRIPTION regress_testsub SET (run_as_owner = true);
\dRs+
ALTER SUBSCRIPTION regress_testsub SET (password_required = true);
+ALTER SUBSCRIPTION regress_testsub SET (run_as_owner = false);
-- fail
ALTER SUBSCRIPTION regress_testsub SET (slot_name = '');
--
2.30.0.windows.2
* Re: [16] ALTER SUBSCRIPTION ... SET (run_as_owner = ...) is a no-op
@ 2023-09-11 11:49 Amit Kapila <amit.kapila16@gmail.com>
parent: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
0 siblings, 2 replies; 6+ messages in thread
From: Amit Kapila @ 2023-09-11 11:49 UTC (permalink / raw)
To: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; +Cc: Jeff Davis <pgsql@j-davis.com>; pgsql-bugs; Robert Haas <robertmhaas@gmail.com>
On Mon, Sep 11, 2023 at 10:46 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Sunday, September 10, 2023 4:43 AM Jeff Davis <pgsql@j-davis.com> wrote:
> >
> >
> > Repro:
> > ALTER SUBSCRIPTION s1 SET (run_as_owner = true);
> > SELECT subrunasowner FROM pg_subscription WHERE subname='s1';
> > subrunasowner
> > ---------------
> > f
> > (1 row)
> >
>
> Thanks for reporting. I can also reproduce the issue. I think it's because we
> didn't reflect the option change on catalog. Here is a small patch 0001 to fix it.
>
Your fix looks good to me.
> > It also looks like a change to that field may not cause the subscription worker to
> > restart. It would be good to add a test for that case.
>
> Currently, the changes on run_as_owner won't cause the worker to restart
> because we don't need to rebuild the connection in this case. The option change
> will be caught by apply worker in next loop and the later changes will be
> applied using the new option. the 0002 patch adds a test to verfiy it, just to
> show how it behaves.
>
Is there a reason to not include 0002 in the commit?
Shall we push this before 16 or wait for it? I don't if we have enough
time or if it is worth pushing at the last minute. I can take care of
pushing this tomorrow morning if we decide that it is okay to go ahead
with this unless Jeff or Robert pushes it.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [16] ALTER SUBSCRIPTION ... SET (run_as_owner = ...) is a no-op
@ 2023-09-11 16:27 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Amit Kapila <amit.kapila16@gmail.com>
1 sibling, 0 replies; 6+ messages in thread
From: Alvaro Herrera @ 2023-09-11 16:27 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; Jeff Davis <pgsql@j-davis.com>; pgsql-bugs; Robert Haas <robertmhaas@gmail.com>
On 2023-Sep-11, Amit Kapila wrote:
> Shall we push this before 16 or wait for it? I don't if we have enough
> time or if it is worth pushing at the last minute. I can take care of
> pushing this tomorrow morning if we decide that it is okay to go ahead
> with this unless Jeff or Robert pushes it.
You should not push anything to the 16 branch until you see the tag come
through tomorrow. There is not enough time for a buildfarm round.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Al principio era UNIX, y UNIX habló y dijo: "Hello world\n".
No dijo "Hello New Jersey\n", ni "Hello USA\n".
^ permalink raw reply [nested|flat] 6+ messages in thread
* RE: [16] ALTER SUBSCRIPTION ... SET (run_as_owner = ...) is a no-op
@ 2023-09-12 01:40 Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
1 sibling, 1 reply; 6+ messages in thread
From: Zhijie Hou (Fujitsu) @ 2023-09-12 01:40 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Jeff Davis <pgsql@j-davis.com>; pgsql-bugs; Robert Haas <robertmhaas@gmail.com>
On Monday, September 11, 2023 7:49 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Sep 11, 2023 at 10:46 AM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
> wrote:
> >
> > On Sunday, September 10, 2023 4:43 AM Jeff Davis <pgsql@j-davis.com>
> wrote:
> > >
> > >
> > > Repro:
> > > ALTER SUBSCRIPTION s1 SET (run_as_owner = true);
> > > SELECT subrunasowner FROM pg_subscription WHERE subname='s1';
> > > subrunasowner
> > > ---------------
> > > f
> > > (1 row)
> > >
> >
> > Thanks for reporting. I can also reproduce the issue. I think it's
> > because we didn't reflect the option change on catalog. Here is a small patch
> 0001 to fix it.
> >
>
> Your fix looks good to me.
Thanks for reviewing.
>
> > > It also looks like a change to that field may not cause the
> > > subscription worker to restart. It would be good to add a test for that case.
> >
> > Currently, the changes on run_as_owner won't cause the worker to
> > restart because we don't need to rebuild the connection in this case.
> > The option change will be caught by apply worker in next loop and the
> > later changes will be applied using the new option. the 0002 patch
> > adds a test to verfiy it, just to show how it behaves.
> >
>
> Is there a reason to not include 0002 in the commit?
No, I think it's fine to include 0002.
>
> Shall we push this before 16 or wait for it? I don't if we have enough time or if it
> is worth pushing at the last minute. I can take care of pushing this tomorrow
> morning if we decide that it is okay to go ahead with this unless Jeff or Robert
> pushes it.
I saw the "Stamp 16.0" has came in, so it seems we can only include this fix in 16.1.
Best Regards,
Hou zj
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [16] ALTER SUBSCRIPTION ... SET (run_as_owner = ...) is a no-op
@ 2023-09-13 10:42 Amit Kapila <amit.kapila16@gmail.com>
parent: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
0 siblings, 0 replies; 6+ messages in thread
From: Amit Kapila @ 2023-09-13 10:42 UTC (permalink / raw)
To: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; +Cc: Jeff Davis <pgsql@j-davis.com>; pgsql-bugs; Robert Haas <robertmhaas@gmail.com>
On Tue, Sep 12, 2023 at 7:10 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Monday, September 11, 2023 7:49 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Sep 11, 2023 at 10:46 AM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
> > wrote:
> > >
> > > On Sunday, September 10, 2023 4:43 AM Jeff Davis <pgsql@j-davis.com>
> > wrote:
> > > >
> > > >
> > > > Repro:
> > > > ALTER SUBSCRIPTION s1 SET (run_as_owner = true);
> > > > SELECT subrunasowner FROM pg_subscription WHERE subname='s1';
> > > > subrunasowner
> > > > ---------------
> > > > f
> > > > (1 row)
> > > >
> > >
> > > Thanks for reporting. I can also reproduce the issue. I think it's
> > > because we didn't reflect the option change on catalog. Here is a small patch
> > 0001 to fix it.
> > >
> >
> > Your fix looks good to me.
>
> Thanks for reviewing.
>
> >
> > > > It also looks like a change to that field may not cause the
> > > > subscription worker to restart. It would be good to add a test for that case.
> > >
> > > Currently, the changes on run_as_owner won't cause the worker to
> > > restart because we don't need to rebuild the connection in this case.
> > > The option change will be caught by apply worker in next loop and the
> > > later changes will be applied using the new option. the 0002 patch
> > > adds a test to verfiy it, just to show how it behaves.
> > >
> >
> > Is there a reason to not include 0002 in the commit?
>
> No, I think it's fine to include 0002.
>
Pushed by slightly adjusting the comments in the test case.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2023-09-13 10:42 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-09-09 20:42 [16] ALTER SUBSCRIPTION ... SET (run_as_owner = ...) is a no-op Jeff Davis <pgsql@j-davis.com>
2023-09-11 03:26 ` Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
2023-09-11 11:49 ` Amit Kapila <amit.kapila16@gmail.com>
2023-09-11 16:27 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-09-12 01:40 ` Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
2023-09-13 10:42 ` Amit Kapila <amit.kapila16@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox