Thread: 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
Attachment
On Fri, Dec 3, 2021 at 2:06 PM vignesh C <vignesh21@gmail.com> 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
On 12/2/21, 7:07 PM, "vignesh C" <vignesh21@gmail.com> 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
On Fri, Dec 3, 2021 at 9:58 AM Bossart, Nathan <bossartn@amazon.com> wrote: > > On 12/2/21, 7:07 PM, "vignesh C" <vignesh21@gmail.com> 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
On Fri, Dec 3, 2021 at 9:53 AM Greg Nancarrow <gregn4422@gmail.com> wrote: > > On Fri, Dec 3, 2021 at 2:06 PM vignesh C <vignesh21@gmail.com> 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
Attachment
On Friday, December 3, 2021 1:31 PM vignesh C <vignesh21@gmail.com> wrote: > > On Fri, Dec 3, 2021 at 9:53 AM Greg Nancarrow <gregn4422@gmail.com> wrote: > > > > On Fri, Dec 3, 2021 at 2:06 PM vignesh C <vignesh21@gmail.com> 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
On Fri, Dec 03, 2021 at 05:20:35PM +0000, Bossart, Nathan wrote: > On 12/2/21, 11:57 PM, "tanghy.fnst@fujitsu.com" <tanghy.fnst@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
Attachment
On Mon, Dec 6, 2021 at 11:46 AM Michael Paquier <michael@paquier.xyz> wrote: > > On Fri, Dec 03, 2021 at 05:20:35PM +0000, Bossart, Nathan wrote: > > On 12/2/21, 11:57 PM, "tanghy.fnst@fujitsu.com" <tanghy.fnst@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.
On Fri, Dec 3, 2021 at 10:50 PM Bossart, Nathan <bossartn@amazon.com> wrote: > > On 12/2/21, 11:57 PM, "tanghy.fnst@fujitsu.com" <tanghy.fnst@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
Attachment
On Tue, Dec 7, 2021 at 8:21 AM vignesh C <vignesh21@gmail.com> 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.
Attachment
On Tue, Dec 7, 2021 at 9:01 PM Amit Kapila <amit.kapila16@gmail.com> 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
On 12/7/21, 2:47 AM, "Greg Nancarrow" <gregn4422@gmail.com> wrote: > On Tue, Dec 7, 2021 at 9:01 PM Amit Kapila <amit.kapila16@gmail.com> 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
On Tue, Dec 7, 2021 at 11:20 PM Bossart, Nathan <bossartn@amazon.com> wrote: > > On 12/7/21, 2:47 AM, "Greg Nancarrow" <gregn4422@gmail.com> wrote: > > On Tue, Dec 7, 2021 at 9:01 PM Amit Kapila <amit.kapila16@gmail.com> 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.