RE: Skipping schema changes in publication

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,

Several comments on v5-0002.

(1) One unnecessary space before "except_pub_obj_list" syntax definition

+ except_pub_obj_list: ExceptPublicationObjSpec
+ { $$ = list_make1($1); }
+ | except_pub_obj_list ',' ExceptPublicationObjSpec
+ { $$ = lappend($1, $3); }
+ | /*EMPTY*/ { $$ = NULL; }
+ ;
+

From above part, kindly change
FROM:
" except_pub_obj_list: ExceptPublicationObjSpec"
TO:
"except_pub_obj_list: ExceptPublicationObjSpec"

(2) doc/src/sgml/ref/create_publication.sgml

(2-1)

@@ -22,7 +22,7 @@ PostgreSQL documentation
<refsynopsisdiv>
<synopsis>
CREATE PUBLICATION <replaceable class="parameter">name</replaceable>
- [ FOR ALL TABLES
+ [ FOR ALL TABLES [EXCEPT TABLE [ ONLY ] <replaceable class="parameter">table_name</replaceable> [ * ] [, ... ]]
| FOR <replaceable class="parameter">publication_object</replaceable> [, ... ] ]
[ WITH ( <replaceable class="parameter">publication_parameter</replaceable> [= <replaceable class="parameter">value</replaceable>] [, ... ] ) ]

Here I think we need to add two more whitespaces around square brackets.
Please change
FROM:
"[ FOR ALL TABLES [EXCEPT TABLE [ ONLY ] <replaceable class="parameter">table_name</replaceable> [ * ] [, ... ]]"
TO:
"[ FOR ALL TABLES [ EXCEPT TABLE [ ONLY ] <replaceable class="parameter">table_name</replaceable> [ * ] [, ... ] ]"

When I check other documentations, I see whitespaces before/after square brackets.

(2-2)
This whitespace alignment applies to alter_publication.sgml as well.

(3)

@@ -156,6 +156,24 @@ CREATE PUBLICATION <replaceable class="parameter">name</replaceable>
</listitem>
</varlistentry>

+
+ <varlistentry>
+ <term><literal>EXCEPT TABLE</literal></term>
+ <listitem>
+ <para>
+ Marks the publication as one that excludes replicating changes for the
+ specified tables.
+ </para>
+
+ <para>
+ <literal>EXCEPT TABLE</literal> can be specified only for
+ <literal>FOR ALL TABLES</literal> publication. It is not supported for
+ <literal>FOR ALL TABLES IN SCHEMA </literal> publication and
+ <literal>FOR TABLE</literal> publication.
+ </para>
+ </listitem>
+ </varlistentry>
+

This EXCEPT TABLE clause is only for FOR ALL TABLES.
So, how about extracting the main message from above part and
moving it to an exising paragraph below, instead of having one independent paragraph ?

<varlistentry>
<term><literal>FOR ALL TABLES</literal></term>
<listitem>
<para>
Marks the publication as one that replicates changes for all tables in
the database, including tables created in the future.
</para>
</listitem>
</varlistentry>

Something like
"Marks the publication as one that replicates changes for all tables in
the database, including tables created in the future. EXCEPT TABLE indicates
excluded tables for the defined publication.
"

(4) One minor confirmation about the syntax

Currently, we allow one way of writing to indicate excluded tables like below.

(example) CREATE PUBLICATION mypub FOR ALL TABLES EXCEPT TABLE tab3, tab4, EXCEPT TABLE tab5;

This is because we define ExceptPublicationObjSpec with EXCEPT TABLE.
Is it OK to have a room to write duplicate "EXCEPT TABLE" clauses ?
I think there is no harm in having this,
but I'd like to confirm whether this syntax might be better to be adjusted or not.

(5) CheckAlterPublication

+
+ if (excepttable && !stmt->for_all_tables)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("publication \"%s\" is not defined as FOR ALL TABLES",
+ NameStr(pubform->pubname)),
+ errdetail("except table cannot be added to, dropped from, or set on NON ALL TABLES publications.")));

Could you please add a test for this ?

Best Regards,
Takamichi Osumi