Обсуждение: 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

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

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

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

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.

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.