Horton

Thread

Commits

  1. Fix changing the ownership of ALL TABLES IN SCHEMA publication.

  1. Alter all tables in schema owner fix

    vignesh C <vignesh21@gmail.com> — 2021-12-03T03:06:23Z

    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

  2. Re: Alter all tables in schema owner fix

    Greg Nancarrow <gregn4422@gmail.com> — 2021-12-03T04:23:21Z

    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

  3. Re: Alter all tables in schema owner fix

    Nathan Bossart <bossartn@amazon.com> — 2021-12-03T04:28:02Z

    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

  4. Re: Alter all tables in schema owner fix

    vignesh C <vignesh21@gmail.com> — 2021-12-03T04:44:33Z

    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

  5. Re: Alter all tables in schema owner fix

    vignesh C <vignesh21@gmail.com> — 2021-12-03T05:30:49Z

    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

  6. RE: Alter all tables in schema owner fix

    tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> — 2021-12-03T07:54:06Z

    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

  7. Re: Alter all tables in schema owner fix

    Nathan Bossart <bossartn@amazon.com> — 2021-12-03T17:20:35Z

    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

Nathan

  8. Re: Alter all tables in schema owner fix

    Michael Paquier <michael@paquier.xyz> — 2021-12-06T06:16:11Z

    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

  9. Re: Alter all tables in schema owner fix

    Amit Kapila <amit.kapila16@gmail.com> — 2021-12-06T08:44:51Z

    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.

  10. Re: Alter all tables in schema owner fix

    vignesh C <vignesh21@gmail.com> — 2021-12-07T02:51:40Z

    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

  11. Re: Alter all tables in schema owner fix

    Amit Kapila <amit.kapila16@gmail.com> — 2021-12-07T10:00:59Z

    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.

  12. Re: Alter all tables in schema owner fix

    Greg Nancarrow <gregn4422@gmail.com> — 2021-12-07T10:46:36Z

    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

  13. Re: Alter all tables in schema owner fix

    Nathan Bossart <bossartn@amazon.com> — 2021-12-07T17:49:57Z

    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

  14. Re: Alter all tables in schema owner fix

    Amit Kapila <amit.kapila16@gmail.com> — 2021-12-08T10:15:20Z

    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.