Re: Skipping schema changes in publication

On Thu, May 12, 2022 at 2:24 PM vignesh C <vignesh21@gmail.com> wrote:
>
...
> The attached patch has the implementation for "ALTER PUBLICATION
> pubname RESET". This command will reset the publication to default
> state which includes resetting the publication options, setting ALL
> TABLES option to false and dropping the relations and schemas that are
> associated with the publication.
>

Please see below my review comments for the v1-0001 (RESET) patch

======

1. Commit message

This patch adds a new RESET option to ALTER PUBLICATION which

Wording: "RESET option" -> "RESET clause"

~~~

2. doc/src/sgml/ref/alter_publication.sgml

+  <para>
+   The <literal>RESET</literal> clause will reset the publication to default
+   state which includes resetting the publication options, setting
+   <literal>ALL TABLES</literal> option to <literal>false</literal>
and drop the
+   relations and schemas that are associated with the publication.
   </para>

2a. Wording: "to default state" -> "to the default state"

2b. Wording: "and drop the relations..." -> "and dropping all relations..."

~~~

3. doc/src/sgml/ref/alter_publication.sgml

+   invoking user to be a superuser.  <literal>RESET</literal> of publication
+   requires invoking user to be a superuser. To alter the owner, you must also

Wording: "requires invoking user" -> "requires the invoking user"

~~~

4. doc/src/sgml/ref/alter_publication.sgml - Example

@@ -207,6 +220,12 @@ ALTER PUBLICATION sales_publication ADD ALL
TABLES IN SCHEMA marketing, sales;
    <structname>production_publication</structname>:
 <programlisting>
 ALTER PUBLICATION production_publication ADD TABLE users,
departments, ALL TABLES IN SCHEMA production;
+</programlisting></para>
+
+  <para>
+   Resetting the publication <structname>production_publication</structname>:
+<programlisting>
+ALTER PUBLICATION production_publication RESET;

Wording: "Resetting the publication" -> "Reset the publication"

~~~

5. src/backend/commands/publicationcmds.c

+ /* Check and reset the options */

IMO the code can just reset all these options unconditionally. I did
not see the point to check for existing option values first. I feel
the simpler code outweighs any negligible performance difference in
this case.

~~~

6. src/backend/commands/publicationcmds.c

+ /* Check and reset the options */

Somehow it seemed a pity having to hardcode all these default values
true/false in multiple places; e.g. the same is already hardcoded in
the parse_publication_options function.

To avoid multiple hard coded bools you could just call the
parse_publication_options with an empty options list. That would set
the defaults which you can then use:
values[Anum_pg_publication_pubinsert - 1] = BoolGetDatum(pubactiondefs->insert);

Alternatively, maybe there should be #defines to use instead of having
the scattered hardcoded bool defaults:
#define PUBACTION_DEFAULT_INSERT true
#define PUBACTION_DEFAULT_UPDATE true
etc

~~~

7. src/include/nodes/parsenodes.h

@@ -4033,7 +4033,8 @@ typedef enum AlterPublicationAction
 {
  AP_AddObjects, /* add objects to publication */
  AP_DropObjects, /* remove objects from publication */
- AP_SetObjects /* set list of objects */
+ AP_SetObjects, /* set list of objects */
+ AP_ReSetPublication /* reset the publication */
 } AlterPublicationAction;

Unusual case: "AP_ReSetPublication" -> "AP_ResetPublication"

~~~

8. src/test/regress/sql/publication.sql

8a.
+-- Test for RESET PUBLICATION
SUGGESTED
+-- Tests for ALTER PUBLICATION ... RESET

8b.
+-- Verify that 'ALL TABLES' option is reset
SUGGESTED:
+-- Verify that 'ALL TABLES' flag is reset

8c.
+-- Verify that publish option and publish via root option is reset
SUGGESTED:
+-- Verify that publish options and publish_via_partition_root option are reset

8d.
+-- Verify that only superuser can execute RESET publication
SUGGESTED
+-- Verify that only superuser can reset a publication

------
Kind Regards,
Peter Smith.
Fujitsu Australia