Re: Alter all tables in schema owner fix
On Tue, Dec 7, 2021 at 11:20 PM Bossart, Nathan wrote: > > On 12/7/21, 2:47 AM, "Greg Nancarrow" wrote: > > On Tue, Dec 7, 2021 at 9:01 PM Amit Kapila wrote: > >> > >> Thanks, the patch looks mostly good to me. I have slightly modified it > >> to incorporate one of Michael's suggestions, ran pgindent, and > >> modified the commit message. > >> > > > > LGTM, except in the patch commit message I'd change "Create > > Publication" to "CREATE PUBLICATION". > > LGTM, too. > Pushed! -- With Regards, Amit Kapila.
Re: Alter all tables in schema owner fix
On 12/7/21, 2:47 AM, "Greg Nancarrow" wrote: > On Tue, Dec 7, 2021 at 9:01 PM Amit Kapila wrote: >> >> Thanks, the patch looks mostly good to me. I have slightly modified it >> to incorporate one of Michael's suggestions, ran pgindent, and >> modified the commit message. >> > > LGTM, except in the patch commit message I'd change "Create > Publication" to "CREATE PUBLICATION". LGTM, too. Nathan
Re: Alter all tables in schema owner fix
On Tue, Dec 7, 2021 at 9:01 PM Amit Kapila wrote: > > Thanks, the patch looks mostly good to me. I have slightly modified it > to incorporate one of Michael's suggestions, ran pgindent, and > modified the commit message. > LGTM, except in the patch commit message I'd change "Create Publication" to "CREATE PUBLICATION". Regards, Greg Nancarrow Fujitsu Australia
Re: Alter all tables in schema owner fix
On Tue, Dec 7, 2021 at 8:21 AM vignesh C wrote: > > Thanks for your comments, I have made the changes. Additionally I have > renamed IsSchemaPublication to is_schema_publication for keeping the > naming similar around the code. The attached v3 patch has the changes > for the same. > Thanks, the patch looks mostly good to me. I have slightly modified it to incorporate one of Michael's suggestions, ran pgindent, and modified the commit message. I am planning to push the attached tomorrow unless there are further comments. Michael, do let me know if you have any questions or objections about this? -- With Regards, Amit Kapila. v4-0001-Fix-changing-the-ownership-of-ALL-TABLES-IN-SCHEM.patch Description: Binary data
Re: Alter all tables in schema owner fix
On Fri, Dec 3, 2021 at 10:50 PM Bossart, Nathan wrote:
>
> On 12/2/21, 11:57 PM, "tanghy.f...@fujitsu.com"
> wrote:
> > Thanks for your patch.
> > I tested it and it fixed this problem as expected. It also passed "make
> > check-world".
>
> +1, the patch looks good to me, too. My only other suggestion would
> be to move IsSchemaPublication() to pg_publication.c
Thanks for your comments, I have made the changes. Additionally I have
renamed IsSchemaPublication to is_schema_publication for keeping the
naming similar around the code. The attached v3 patch has the changes
for the same.
Regards,
Vignesh
From cc4226ced0e535a324dde5b1ff0e48d6e0035e17 Mon Sep 17 00:00:00 2001
From: Vignesh C
Date: Thu, 2 Dec 2021 19:44:15 +0530
Subject: [PATCH v3] Fix for new owner of ALL TABLES IN SCHEMA publication
should be superuser.
Currently while changing the owner of ALL TABLES IN SCHEMA publication, it is
not checked if the new owner has superuser permission or not. Added a check to
throw an error if the new owner does not have super user permission.
---
src/backend/catalog/pg_publication.c | 31 +++
src/backend/commands/publicationcmds.c| 7 +
src/include/catalog/pg_publication.h | 1 +
src/test/regress/expected/publication.out | 15 +++
src/test/regress/sql/publication.sql | 15 +++
5 files changed, 69 insertions(+)
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 63579b2f82..265bd538c3 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -193,6 +193,37 @@ is_publishable_relation(Relation rel)
return is_publishable_class(RelationGetRelid(rel), rel->rd_rel);
}
+/*
+ * Returns true if any schema is associated with the publication, false if no
+ * schema is associated with the publication.
+ */
+bool
+is_schema_publication(Oid pubid)
+{
+ Relation pubschsrel;
+ ScanKeyData scankey;
+ SysScanDesc scan;
+ HeapTuple tup;
+ bool result = false;
+
+ pubschsrel = table_open(PublicationNamespaceRelationId, AccessShareLock);
+ ScanKeyInit(,
+Anum_pg_publication_namespace_pnpubid,
+BTEqualStrategyNumber, F_OIDEQ,
+ObjectIdGetDatum(pubid));
+
+ scan = systable_beginscan(pubschsrel,
+ PublicationNamespacePnnspidPnpubidIndexId,
+ true, NULL, 1, );
+ tup = systable_getnext(scan);
+ if (HeapTupleIsValid(tup))
+ result = true;
+
+ systable_endscan(scan);
+ table_close(pubschsrel, AccessShareLock);
+
+ return result;
+}
/*
* SQL-callable variant of the above
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 7d4a0e95f6..404bb5d0c8 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -1192,6 +1192,13 @@ AlterPublicationOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
errmsg("permission denied to change owner of publication \"%s\"",
NameStr(form->pubname)),
errhint("The owner of a FOR ALL TABLES publication must be a superuser.")));
+
+ if (!superuser_arg(newOwnerId) && is_schema_publication(form->oid))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied to change owner of publication \"%s\"",
+ NameStr(form->pubname)),
+ errhint("The owner of a FOR ALL TABLES IN SCHEMA publication must be a superuser.")));
}
form->pubowner = newOwnerId;
diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h
index 1ae439e6f3..902f2f2f0d 100644
--- a/src/include/catalog/pg_publication.h
+++ b/src/include/catalog/pg_publication.h
@@ -122,6 +122,7 @@ extern List *GetPubPartitionOptionRelations(List *result,
Oid relid);
extern bool is_publishable_relation(Relation rel);
+extern bool is_schema_publication(Oid pubid);
extern ObjectAddress publication_add_relation(Oid pubid, PublicationRelInfo *targetrel,
bool if_not_exists);
extern ObjectAddress publication_add_schema(Oid pubid, Oid schemaid,
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 1feb558968..b7df48e87c 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -373,6 +373,21 @@ ALTER PUBLICATION testpub2 ADD TABLE testpub_tbl1; -- ok
DROP PUBLICATION testpub2;
DROP PUBLICATION testpub3;
SET ROLE regress_publication_user;
+CREATE ROLE regress_publication_user3;
+GRANT regress_publication_user2 TO regress_publication_user3;
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION testpub4 FOR ALL TABLES IN SCHEMA pub_test;
+RESET client_min_messages;
+ALTER PUBLICATION testpub4 OWNER TO regress_publication_user3;
+SET ROLE regress_publication_user3;
+-- fail - new owner must be superuser
+ALTER PUBLICATION testpub4 owner to regress_publication_user2; -- fail
+ERROR: permission denied to change owner of publication "testpub4"
+HINT: The owner of
Re: Alter all tables in schema owner fix
On Mon, Dec 6, 2021 at 11:46 AM Michael Paquier wrote: > > On Fri, Dec 03, 2021 at 05:20:35PM +, Bossart, Nathan wrote: > > On 12/2/21, 11:57 PM, "tanghy.f...@fujitsu.com" > > wrote: > > > Thanks for your patch. > > > I tested it and it fixed this problem as expected. It also passed "make > > > check-world". > > > > +1, the patch looks good to me, too. My only other suggestion would > > be to move IsSchemaPublication() to pg_publication.c > > There is more to that, no? It seems to me that anything that opens > PublicationNamespaceRelationId should be in pg_publication.c, so that > would include RemovePublicationSchemaById(). > It is currently similar to RemovePublicationById, RemovePublicationRelById, etc. which are also in publicationcmds.c. > If you do that, > GetSchemaPublicationRelations() could be local to pg_publication.c. > > + tup = systable_getnext(scan); > + if (HeapTupleIsValid(tup)) > + result = true; > This can be written as just "result = HeapTupleIsValid(tup)". Anyway, > this code also means that once we drop the schema this publication > won't be considered anymore as a schema publication, meaning that it > also makes this code weaker to actual cache lookup failures? > How, can you be a bit more specific? > I find > the semantics around pg_publication_namespace is bit weird because of > that, and inconsistent with the existing > puballtables/pg_publication_rel. > What do you mean by inconsistent with puballtables/pg_publication_rel? -- With Regards, Amit Kapila.
Re: Alter all tables in schema owner fix
On Fri, Dec 03, 2021 at 05:20:35PM +, Bossart, Nathan wrote: > On 12/2/21, 11:57 PM, "tanghy.f...@fujitsu.com" > wrote: > > Thanks for your patch. > > I tested it and it fixed this problem as expected. It also passed "make > > check-world". > > +1, the patch looks good to me, too. My only other suggestion would > be to move IsSchemaPublication() to pg_publication.c There is more to that, no? It seems to me that anything that opens PublicationNamespaceRelationId should be in pg_publication.c, so that would include RemovePublicationSchemaById(). If you do that, GetSchemaPublicationRelations() could be local to pg_publication.c. + tup = systable_getnext(scan); + if (HeapTupleIsValid(tup)) + result = true; This can be written as just "result = HeapTupleIsValid(tup)". Anyway, this code also means that once we drop the schema this publication won't be considered anymore as a schema publication, meaning that it also makes this code weaker to actual cache lookup failures? I find the semantics around pg_publication_namespace is bit weird because of that, and inconsistent with the existing puballtables/pg_publication_rel. -- Michael signature.asc Description: PGP signature
Re: Alter all tables in schema owner fix
On 12/2/21, 11:57 PM, "tanghy.f...@fujitsu.com" wrote: > Thanks for your patch. > I tested it and it fixed this problem as expected. It also passed "make > check-world". +1, the patch looks good to me, too. My only other suggestion would be to move IsSchemaPublication() to pg_publication.c Nathan
RE: Alter all tables in schema owner fix
On Friday, December 3, 2021 1:31 PM vignesh C wrote: > > On Fri, Dec 3, 2021 at 9:53 AM Greg Nancarrow wrote: > > > > On Fri, Dec 3, 2021 at 2:06 PM vignesh C wrote: > > > > > > Currently while changing the owner of ALL TABLES IN SCHEMA > > > publication, it is not checked if the new owner has superuser > > > permission or not. Added a check to throw an error if the new owner > > > does not have superuser permission. > > > Attached patch has the changes for the same. Thoughts? > > > > > > > It looks OK to me, but just two things: > > > > 1) Isn't it better to name "CheckSchemaPublication" as > > "IsSchemaPublication", since it has a boolean return and also > > typically CheckXXX type functions normally do checking and error-out > > if they find a problem. > > Modified > > > 2) Since superuser_arg() caches previous input arg (last_roleid) and > > has a fast-exit, and has been called immediately before for the FOR > > ALL TABLES case, it would be better to write: > > > > + if (CheckSchemaPublication(form->oid) && !superuser_arg(newOwnerId)) > > > > as: > > > > + if (!superuser_arg(newOwnerId) && IsSchemaPublication(form->oid)) > > Modified > > Thanks for the comments, the attached v2 patch has the changes for the same. > Thanks for your patch. I tested it and it fixed this problem as expected. It also passed "make check-world". Regards, Tang
Re: Alter all tables in schema owner fix
On Fri, Dec 3, 2021 at 9:53 AM Greg Nancarrow wrote:
>
> On Fri, Dec 3, 2021 at 2:06 PM vignesh C wrote:
> >
> > Currently while changing the owner of ALL TABLES IN SCHEMA
> > publication, it is not checked if the new owner has superuser
> > permission or not. Added a check to throw an error if the new owner
> > does not have superuser permission.
> > Attached patch has the changes for the same. Thoughts?
> >
>
> It looks OK to me, but just two things:
>
> 1) Isn't it better to name "CheckSchemaPublication" as
> "IsSchemaPublication", since it has a boolean return and also
> typically CheckXXX type functions normally do checking and error-out
> if they find a problem.
Modified
> 2) Since superuser_arg() caches previous input arg (last_roleid) and
> has a fast-exit, and has been called immediately before for the FOR
> ALL TABLES case, it would be better to write:
>
> + if (CheckSchemaPublication(form->oid) && !superuser_arg(newOwnerId))
>
> as:
>
> + if (!superuser_arg(newOwnerId) && IsSchemaPublication(form->oid))
Modified
Thanks for the comments, the attached v2 patch has the changes for the same.
Regards,
Vignesh
From c75ce78efc80e86bcc62f8560a767cc73666a7d2 Mon Sep 17 00:00:00 2001
From: Vignesh C
Date: Thu, 2 Dec 2021 19:44:15 +0530
Subject: [PATCH v2] Fix for new owner of ALL TABLES IN SCHEMA publication
should be superuser.
Currently while changing the owner of ALL TABLES IN SCHEMA publication, it is
not checked if the new owner has superuser permission or not. Added a check to
throw an error if the new owner does not have super user permission.
---
src/backend/commands/publicationcmds.c| 39 +++
src/test/regress/expected/publication.out | 15 +
src/test/regress/sql/publication.sql | 15 +
3 files changed, 69 insertions(+)
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 7d4a0e95f6..e709ddb589 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -234,6 +234,38 @@ CheckObjSchemaNotAlreadyInPublication(List *rels, List *schemaidlist,
}
}
+/*
+ * Returns true if any schema is associated with the publication, false if no
+ * schema is associated with the publication.
+ */
+static bool
+IsSchemaPublication(Oid pubid)
+{
+ Relation pubschsrel;
+ ScanKeyData scankey;
+ SysScanDesc scan;
+ HeapTuple tup;
+ bool result = false;
+
+ pubschsrel = table_open(PublicationNamespaceRelationId, AccessShareLock);
+ ScanKeyInit(,
+Anum_pg_publication_namespace_pnpubid,
+BTEqualStrategyNumber, F_OIDEQ,
+ObjectIdGetDatum(pubid));
+
+ scan = systable_beginscan(pubschsrel,
+ PublicationNamespacePnnspidPnpubidIndexId,
+ true, NULL, 1, );
+ tup = systable_getnext(scan);
+ if (HeapTupleIsValid(tup))
+ result = true;
+
+ systable_endscan(scan);
+ table_close(pubschsrel, AccessShareLock);
+
+ return result;
+}
+
/*
* Create new publication.
*/
@@ -1192,6 +1224,13 @@ AlterPublicationOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
errmsg("permission denied to change owner of publication \"%s\"",
NameStr(form->pubname)),
errhint("The owner of a FOR ALL TABLES publication must be a superuser.")));
+
+ if (!superuser_arg(newOwnerId) && IsSchemaPublication(form->oid))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied to change owner of publication \"%s\"",
+ NameStr(form->pubname)),
+ errhint("The owner of a FOR ALL TABLES IN SCHEMA publication must be a superuser.")));
}
form->pubowner = newOwnerId;
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 1feb558968..b7df48e87c 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -373,6 +373,21 @@ ALTER PUBLICATION testpub2 ADD TABLE testpub_tbl1; -- ok
DROP PUBLICATION testpub2;
DROP PUBLICATION testpub3;
SET ROLE regress_publication_user;
+CREATE ROLE regress_publication_user3;
+GRANT regress_publication_user2 TO regress_publication_user3;
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION testpub4 FOR ALL TABLES IN SCHEMA pub_test;
+RESET client_min_messages;
+ALTER PUBLICATION testpub4 OWNER TO regress_publication_user3;
+SET ROLE regress_publication_user3;
+-- fail - new owner must be superuser
+ALTER PUBLICATION testpub4 owner to regress_publication_user2; -- fail
+ERROR: permission denied to change owner of publication "testpub4"
+HINT: The owner of a FOR ALL TABLES IN SCHEMA publication must be a superuser.
+ALTER PUBLICATION testpub4 owner to regress_publication_user; -- ok
+SET ROLE regress_publication_user;
+DROP PUBLICATION testpub4;
+DROP ROLE regress_publication_user3;
REVOKE CREATE ON DATABASE regression FROM regress_publication_user2;
DROP TABLE testpub_parted;
DROP TABLE testpub_tbl1;
diff --git a/src/test/regress/sql/publication.sql
Re: Alter all tables in schema owner fix
On Fri, Dec 3, 2021 at 9:58 AM Bossart, Nathan wrote: > > On 12/2/21, 7:07 PM, "vignesh C" wrote: > > Currently while changing the owner of ALL TABLES IN SCHEMA > > publication, it is not checked if the new owner has superuser > > permission or not. Added a check to throw an error if the new owner > > does not have superuser permission. > > Attached patch has the changes for the same. Thoughts? > > Yeah, the documentation clearly states that "the new owner of a FOR > ALL TABLES or FOR ALL TABLES IN SCHEMA publication must be a > superuser" [0]. > > +/* > + * Check if any schema is associated with the publication. > + */ > +static bool > +CheckSchemaPublication(Oid pubid) > > I don't think the name CheckSchemaPublication() accurately describes > what this function is doing. I would suggest something like > PublicationHasSchema() or PublicationContainsSchema(). Also, much of > this new function appears to be copied from GetPublicationSchemas(). > Should we just use that instead? I was thinking of changing it to IsSchemaPublication as suggested by Greg unless you feel differently. I did not use GetPublicationSchemas function because in our case we just need to check if there is any schema publication, we don't need the schema list to be prepared in this case. That is the reason I wrote a new function which just checks if any schema is present or not for the publication. I'm planning to use CheckSchemaPublication (renamed to IsSchemaPublication) so that the list need not be prepared. > +CREATE ROLE regress_publication_user3 LOGIN SUPERUSER; > +GRANT regress_publication_user2 TO regress_publication_user3; > +SET ROLE regress_publication_user3; > +SET client_min_messages = 'ERROR'; > +CREATE PUBLICATION testpub4 FOR ALL TABLES IN SCHEMA pub_test; > +RESET client_min_messages; > +SET ROLE regress_publication_user; > +ALTER ROLE regress_publication_user3 NOSUPERUSER; > +SET ROLE regress_publication_user3; > > I think this test setup can be simplified a bit: > > CREATE ROLE regress_publication_user3 LOGIN; > GRANT regress_publication_user2 TO regress_publication_user3; > SET client_min_messages = 'ERROR'; > CREATE PUBLICATION testpub4 FOR ALL TABLES IN SCHEMA pub_test; > RESET client_min_messages; > ALTER PUBLICATION testpub4 OWNER TO regress_publication_user3; > SET ROLE regress_publication_user3; I will make this change in the next version. Regards, VIgnesh
Re: Alter all tables in schema owner fix
On 12/2/21, 7:07 PM, "vignesh C" wrote: > Currently while changing the owner of ALL TABLES IN SCHEMA > publication, it is not checked if the new owner has superuser > permission or not. Added a check to throw an error if the new owner > does not have superuser permission. > Attached patch has the changes for the same. Thoughts? Yeah, the documentation clearly states that "the new owner of a FOR ALL TABLES or FOR ALL TABLES IN SCHEMA publication must be a superuser" [0]. +/* + * Check if any schema is associated with the publication. + */ +static bool +CheckSchemaPublication(Oid pubid) I don't think the name CheckSchemaPublication() accurately describes what this function is doing. I would suggest something like PublicationHasSchema() or PublicationContainsSchema(). Also, much of this new function appears to be copied from GetPublicationSchemas(). Should we just use that instead? +CREATE ROLE regress_publication_user3 LOGIN SUPERUSER; +GRANT regress_publication_user2 TO regress_publication_user3; +SET ROLE regress_publication_user3; +SET client_min_messages = 'ERROR'; +CREATE PUBLICATION testpub4 FOR ALL TABLES IN SCHEMA pub_test; +RESET client_min_messages; +SET ROLE regress_publication_user; +ALTER ROLE regress_publication_user3 NOSUPERUSER; +SET ROLE regress_publication_user3; I think this test setup can be simplified a bit: CREATE ROLE regress_publication_user3 LOGIN; GRANT regress_publication_user2 TO regress_publication_user3; SET client_min_messages = 'ERROR'; CREATE PUBLICATION testpub4 FOR ALL TABLES IN SCHEMA pub_test; RESET client_min_messages; ALTER PUBLICATION testpub4 OWNER TO regress_publication_user3; SET ROLE regress_publication_user3; Nathan [0] https://www.postgresql.org/docs/devel/sql-alterpublication.html
Re: Alter all tables in schema owner fix
On Fri, Dec 3, 2021 at 2:06 PM vignesh C wrote: > > Currently while changing the owner of ALL TABLES IN SCHEMA > publication, it is not checked if the new owner has superuser > permission or not. Added a check to throw an error if the new owner > does not have superuser permission. > Attached patch has the changes for the same. Thoughts? > It looks OK to me, but just two things: 1) Isn't it better to name "CheckSchemaPublication" as "IsSchemaPublication", since it has a boolean return and also typically CheckXXX type functions normally do checking and error-out if they find a problem. 2) Since superuser_arg() caches previous input arg (last_roleid) and has a fast-exit, and has been called immediately before for the FOR ALL TABLES case, it would be better to write: + if (CheckSchemaPublication(form->oid) && !superuser_arg(newOwnerId)) as: + if (!superuser_arg(newOwnerId) && IsSchemaPublication(form->oid)) Regards, Greg Nancarrow Fujitsu Australia
Alter all tables in schema owner fix
Hi,
Currently while changing the owner of ALL TABLES IN SCHEMA
publication, it is not checked if the new owner has superuser
permission or not. Added a check to throw an error if the new owner
does not have superuser permission.
Attached patch has the changes for the same. Thoughts?
Regards,
Vignesh
From d0d0d2d8944a66f28b239da5fb80c15415a016e6 Mon Sep 17 00:00:00 2001
From: Vignesh C
Date: Thu, 2 Dec 2021 19:44:15 +0530
Subject: [PATCH v1] Fix for new owner of ALL TABLES IN SCHEMA publication
should be superuser.
Currently while changing the owner of ALL TABLES IN SCHEMA publication, it is
not checked if the new owner has superuser permission or not. Added a check to
throw an error if the new owner does not have super user permission.
---
src/backend/commands/publicationcmds.c| 38 +++
src/test/regress/expected/publication.out | 17 ++
src/test/regress/sql/publication.sql | 17 ++
3 files changed, 72 insertions(+)
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 7d4a0e95f6..7081919ffc 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -234,6 +234,37 @@ CheckObjSchemaNotAlreadyInPublication(List *rels, List *schemaidlist,
}
}
+/*
+ * Check if any schema is associated with the publication.
+ */
+static bool
+CheckSchemaPublication(Oid pubid)
+{
+ Relation pubschsrel;
+ ScanKeyData scankey;
+ SysScanDesc scan;
+ HeapTuple tup;
+ bool result = false;
+
+ pubschsrel = table_open(PublicationNamespaceRelationId, AccessShareLock);
+ ScanKeyInit(,
+Anum_pg_publication_namespace_pnpubid,
+BTEqualStrategyNumber, F_OIDEQ,
+ObjectIdGetDatum(pubid));
+
+ scan = systable_beginscan(pubschsrel,
+ PublicationNamespacePnnspidPnpubidIndexId,
+ true, NULL, 1, );
+ tup = systable_getnext(scan);
+ if (HeapTupleIsValid(tup))
+ result = true;
+
+ systable_endscan(scan);
+ table_close(pubschsrel, AccessShareLock);
+
+ return result;
+}
+
/*
* Create new publication.
*/
@@ -1192,6 +1223,13 @@ AlterPublicationOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
errmsg("permission denied to change owner of publication \"%s\"",
NameStr(form->pubname)),
errhint("The owner of a FOR ALL TABLES publication must be a superuser.")));
+
+ if (CheckSchemaPublication(form->oid) && !superuser_arg(newOwnerId))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied to change owner of publication \"%s\"",
+ NameStr(form->pubname)),
+ errhint("The owner of a FOR ALL TABLES IN SCHEMA publication must be a superuser.")));
}
form->pubowner = newOwnerId;
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 1feb558968..4c74488113 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -373,6 +373,23 @@ ALTER PUBLICATION testpub2 ADD TABLE testpub_tbl1; -- ok
DROP PUBLICATION testpub2;
DROP PUBLICATION testpub3;
SET ROLE regress_publication_user;
+CREATE ROLE regress_publication_user3 LOGIN SUPERUSER;
+GRANT regress_publication_user2 TO regress_publication_user3;
+SET ROLE regress_publication_user3;
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION testpub4 FOR ALL TABLES IN SCHEMA pub_test;
+RESET client_min_messages;
+SET ROLE regress_publication_user;
+ALTER ROLE regress_publication_user3 NOSUPERUSER;
+SET ROLE regress_publication_user3;
+-- fail - new owner must be superuser
+ALTER PUBLICATION testpub4 owner to regress_publication_user2; -- fail
+ERROR: permission denied to change owner of publication "testpub4"
+HINT: The owner of a FOR ALL TABLES IN SCHEMA publication must be a superuser.
+ALTER PUBLICATION testpub4 owner to regress_publication_user; -- ok
+SET ROLE regress_publication_user;
+DROP PUBLICATION testpub4;
+DROP ROLE regress_publication_user3;
REVOKE CREATE ON DATABASE regression FROM regress_publication_user2;
DROP TABLE testpub_parted;
DROP TABLE testpub_tbl1;
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index 8fa0435c32..880f0caa1d 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -218,6 +218,23 @@ DROP PUBLICATION testpub2;
DROP PUBLICATION testpub3;
SET ROLE regress_publication_user;
+CREATE ROLE regress_publication_user3 LOGIN SUPERUSER;
+GRANT regress_publication_user2 TO regress_publication_user3;
+SET ROLE regress_publication_user3;
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION testpub4 FOR ALL TABLES IN SCHEMA pub_test;
+RESET client_min_messages;
+SET ROLE regress_publication_user;
+ALTER ROLE regress_publication_user3 NOSUPERUSER;
+SET ROLE regress_publication_user3;
+-- fail - new owner must be superuser
+ALTER PUBLICATION testpub4 owner to regress_publication_user2; -- fail
+ALTER PUBLICATION testpub4 owner to
