On Friday, December 3, 2021 1:31 PM vignesh C <vignes...@gmail.com> wrote: > > On Fri, Dec 3, 2021 at 9:53 AM Greg Nancarrow <gregn4...@gmail.com> wrote: > > > > On Fri, Dec 3, 2021 at 2:06 PM vignesh C <vignes...@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
