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. 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
