Re: Alter all tables in schema owner fix

On Fri, Dec 3, 2021 at 9:58 AM Bossart, Nathan <bossartn(at)amazon(dot)com> wrote:
>
> On 12/2/21, 7:07 PM, "vignesh C" <vignesh21(at)gmail(dot)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