Re: Skipping schema changes in publication

On Mon, May 16, 2022 at 8:32 AM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
>
> On Saturday, May 14, 2022 10:33 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > Thanks for the comments, the attached v5 patch has the changes for the same.
> > Also I have made the changes for SKIP Table based on the new syntax, the
> > changes for the same are available in
> > v5-0002-Skip-publishing-the-tables-specified-in-EXCEPT-TA.patch.
> Hi,
>
>
> Thank you for updating the patch.
> I'll share few minor review comments on v5-0001.
>
>
> (1) doc/src/sgml/ref/alter_publication.sgml
>
> @@ -73,12 +85,13 @@ ALTER PUBLICATION <replaceable class="parameter">name</replaceable> RENAME TO <r
> Adding a table to a publication additionally requires owning that table.
> The <literal>ADD ALL TABLES IN SCHEMA</literal> and
> <literal>SET ALL TABLES IN SCHEMA</literal> to a publication requires the
> - invoking user to be a superuser. To alter the owner, you must also be a
> - direct or indirect member of the new owning role. The new owner must have
> - <literal>CREATE</literal> privilege on the database. Also, the new owner
> - of a <literal>FOR ALL TABLES</literal> or <literal>FOR ALL TABLES IN
> - SCHEMA</literal> publication must be a superuser. However, a superuser can
> - change the ownership of a publication regardless of these restrictions.
> + invoking user to be a superuser. <literal>RESET</literal> of publication
> + requires the invoking user to be a superuser. To alter the owner, you must
> ...
>
>
> I suggest to combine the first part of your change with one existing sentence
> before your change, to make our description concise.
>
> FROM:
> "The <literal>ADD ALL TABLES IN SCHEMA</literal> and
> <literal>SET ALL TABLES IN SCHEMA</literal> to a publication requires the
> invoking user to be a superuser. <literal>RESET</literal> of publication
> requires the invoking user to be a superuser."
>
> TO:
> "The <literal>ADD ALL TABLES IN SCHEMA</literal>,
> <literal>SET ALL TABLES IN SCHEMA</literal> to a publication and
> <literal>RESET</literal> of publication requires the invoking user to be a superuser."

Modified

>
> (2) typo
>
> +++ b/src/backend/commands/publicationcmds.c
> @@ -53,6 +53,13 @@
> #include "utils/syscache.h"
> #include "utils/varlena.h"
>
> +#define PUB_ATION_INSERT_DEFAULT true
> +#define PUB_ACTION_UPDATE_DEFAULT true
>
>
> Kindly change
> FROM:
> "PUB_ATION_INSERT_DEFAULT"
> TO:
> "PUB_ACTION_INSERT_DEFAULT"

Modified

>
> (3) src/test/regress/expected/publication.out
>
> +-- Verify that only superuser can reset a publication
> +ALTER PUBLICATION testpub_reset OWNER TO regress_publication_user2;
> +SET ROLE regress_publication_user2;
> +ALTER PUBLICATION testpub_reset RESET; -- fail
>
>
> We have "-- fail" for one case in this patch.
> On the other hand, isn't better to add "-- ok" (or "-- success") for
> other successful statements,
> when we consider the entire tests description consistency ?

We generally do not mention success comments for all the success cases
as that might be an overkill. I felt it is better to keep it as it is.
Thoughts?

The attached v6 patch has the changes for the same.

Regards,
Vignesh