RE: Skipping schema changes in publication

On Monday, May 23, 2022 2:13 PM vignesh C <vignesh21@gmail.com> wrote:
> Attached v7 patch which fixes the buildfarm warning for an unused warning in
> release mode as in  [1].
Hi, thank you for the patches.


I'll share several review comments.

For v7-0001.

(1) I'll suggest some minor rewording.

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

My suggestion is
"The RESET clause will reset the publication to the
default state. It resets the publication operations,
sets ALL TABLES flag to false and drops all relations
and schemas associated with the publication."

(2) typo and rewording

+/*
+ * Reset the publication.
+ *
+ * Reset the publication options, setting ALL TABLES flag to false and drop
+ * all relations and schemas that are associated with the publication.
+ */

The "setting" in this sentence should be "set".

How about changing like below ?
FROM:
"Reset the publication options, setting ALL TABLES flag to false and drop
all relations and schemas that are associated with the publication."
TO:
"Reset the publication operations, set ALL TABLES flag to false and drop
all relations and schemas associated with the publication."

(3) AlterPublicationReset

Do we need to call CacheInvalidateRelcacheAll() or
InvalidatePublicationRels() at the end of
AlterPublicationReset() like AlterPublicationOptions() ?


For v7-0002.

(4)

+       if (stmt->for_all_tables)
+       {
+               bool            isdefault = CheckPublicationDefValues(tup);
+
+               if (!isdefault)
+                       ereport(ERROR,
+                                       errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+                                       errmsg("adding ALL TABLES requires the publication to have default publication
options,no tables/....
 
+                                       errhint("Use ALTER PUBLICATION ... RESET to reset the publication"));


The errmsg string has three messages for user and is a bit long
(we have two sentences there connected by 'and').
Can't we make it concise and split it into a couple of lines for code readability ?

I'll suggest a change below.
FROM:
"adding ALL TABLES requires the publication to have default publication options, no tables/schemas associated and ALL
TABLESflag should not be set"
 
TO:
"adding ALL TABLES requires the publication defined not for ALL TABLES"
"to have default publish actions without any associated tables/schemas"

(5) typo

   <varlistentry>
+    <term><literal>EXCEPT TABLE</literal></term>
+    <listitem>
+     <para>
+      This clause specifies a list of tables to exclude from the publication.
+      It can only be used with <literal>FOR ALL TABLES</literal>.
+     </para>
+    </listitem>
+   </varlistentry>
+

Kindly change
FROM:
This clause specifies a list of tables to exclude from the publication.
TO:
This clause specifies a list of tables to be excluded from the publication.
or
This clause specifies a list of tables excluded from the publication.

(6) Minor suggestion for an expression change

       Marks the publication as one that replicates changes for all tables in
-      the database, including tables created in the future.
+      the database, including tables created in the future. If
+      <literal>EXCEPT TABLE</literal> is specified, then exclude replicating
+      the changes for the specified tables.


I'll suggest a minor rewording.
FROM:
...exclude replicating the changes for the specified tables
TO:
...exclude replication changes for the specified tables

(7)
(7-1)

+/*
+ * Check if the publication has default values
+ *
+ * Check the following:
+ * a) Publication is not set with "FOR ALL TABLES"
+ * b) Publication is having default options
+ * c) Publication is not associated with schemas
+ * d) Publication is not associated with relations
+ */
+static bool
+CheckPublicationDefValues(HeapTuple tup)


I think this header comment can be improved.
FROM:
Check the following:
TO:
Returns true if the publication satisfies all the following conditions:

(7-2)

b) should be changed as well
FROM:
Publication is having default options
TO:
Publication has the default publish operations



Best Regards,
    Takamichi Osumi