Re: Skipping schema changes in publication

Below are my review comments for v5-0002.

There may be an overlap with comments recently posted by Osumi-san [1].

(I also have review comments for v5-0002; will post them tomorrow)

======

1. General

Is it really necessary to have to say "EXCEPT TABLE" instead of just
"EXCEPT". It seems unnecessarily verbose and redundant when you write
"FOR ALL TABLES EXCEPT TABLE...".

If you want to keep this TABLE keyword (maybe you have plans for other
kinds of except?) then IMO perhaps at least it can be the optional
default except type. e.g. EXCEPT [TABLE].

~~~

2. General

(I was unsure whether to even mention this one).

I understand the "EXCEPT" is chosen as the user-facing syntax, but it
still seems strange when reading the patch to see attribute members
and column names called 'except'. I think the problem is that "except"
is not a verb, so saying except=t/f just does not make much sense.
Sometimes I feel that for all the internal usage
(code/comments/catalog) using "skip" and "skip-list" etc would be a
much better choice of names. OTOH I can see that having consistency
with the outside syntax might also be good. Anyway, please consider -
maybe other people feel the same?

~~~

3. General

The ONLY keyword seems supported by the syntax for tables of the
except-list (more on this in later comments) but:
a) I am not sure if the patch code is accounting for that, and
b) There are no test cases using ONLY.

~~~

4. Commit message

A new option "EXCEPT TABLE" in Create/Alter Publication allows
one or more tables to be excluded, publisher will exclude sending the data
of the excluded tables to the subscriber.

SUGGESTION
A new "EXCEPT TABLE" clause for CREATE/ALTER PUBLICATION allows one or
more tables to be excluded. The publisher will not send the data of
excluded tables to the subscriber.

~~

5. Commit message

The new syntax allows specifying exclude relations while creating a publication
or exclude relations in alter publication. For example:

SUGGESTION
The new syntax allows specifying excluded relations when creating or
altering a publication. For example:

~~~

6. Commit message

A new column prexcept is added to table "pg_publication_rel", to maintain
the relations that the user wants to exclude publishing through the publication.

SUGGESTION
A new column "prexcept" is added to table "pg_publication_rel", to
maintain the relations that the user wants to exclude from the
publications.

~~~

7. Commit message

Modified the output plugin (pgoutput) to exclude publishing the changes of the
excluded tables.

I did not feel it was necessary to say this. It is already said above
that the data is not sent, so that seems enough.

~~~

8. Commit message

Updates pg_dump to identify and dump the excluded tables of the publications.
Updates the \d family of commands to display excluded tables of the
publications and \dRp+ variant will now display associated except tables if any.

SUGGESTION
pg_dump is updated to identify and dump the excluded tables of the publications.

The psql \d family of commands to display excluded tables. e.g. psql
\dRp+ variant will now display associated "except tables" if any.

~~~

9. doc/src/sgml/catalogs.sgml

@@ -6426,6 +6426,15 @@ SCRAM-SHA-256$<replaceable><iteration
count></replaceable>:<replaceable>&l
       if there is no publication qualifying condition.</para></entry>
      </row>

+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+      <structfield>prexcept</structfield> <type>bool</type>
+      </para>
+      <para>
+       True if the table must be excluded
+      </para></entry>
+     </row>

Other descriptions on this page refer to "relation" instead of
"table". Probably this should do the same to be consistent.

~~~

10. doc/src/sgml/logical-replication.sgml

@@ -1167,8 +1167,9 @@ CONTEXT:  processing remote data for replication
origin "pg_16395" during "INSER
   <para>
    To add tables to a publication, the user must have ownership rights on the
    table. To add all tables in schema to a publication, the user must be a
-   superuser. To create a publication that publishes all tables or
all tables in
-   schema automatically, the user must be a superuser.
+   superuser. To add all tables to a publication, the user must be a superuser.
+   To create a publication that publishes all tables or all tables in schema
+   automatically, the user must be a superuser.
   </para>

It seems like a valid change but how is this related to this EXCEPT
patch. Maybe this fix should be patched separately?

~~~

11. doc/src/sgml/ref/alter_publication.sgml

@@ -22,6 +22,7 @@ PostgreSQL documentation
  <refsynopsisdiv>
 <synopsis>
 ALTER PUBLICATION <replaceable class="parameter">name</replaceable>
ADD <replaceable class="parameter">publication_object</replaceable> [,
...]
+ALTER PUBLICATION <replaceable class="parameter">name</replaceable>
ADD ALL TABLES [EXCEPT TABLE [ ONLY ] <replaceable
class="parameter">table_name</replaceable> [ * ] [, ... ]]

The [ONLY] looks misplaced when the syntax is described like this. For
example, in practice it is possible to write "EXCEPT TABLE ONLY t1,
ONLY t2, t3, ONLY t4" but it doesn't seem that way by looking at these
PG DOCS.

IMO would be better described like this:

[ FOR ALL TABLES [ EXCEPT TABLE exception_object [,...] ]]

where exception_object is:

    [ ONLY ] table_name [ * ]

~~~

12. doc/src/sgml/ref/alter_publication.sgml

@@ -82,8 +83,8 @@ ALTER PUBLICATION <replaceable
class="parameter">name</replaceable> RESET

   <para>
    You must own the publication to use <command>ALTER PUBLICATION</command>.
-   Adding a table to a publication additionally requires owning that table.
-   The <literal>ADD ALL TABLES IN SCHEMA</literal> and
+   Adding a table or excluding a table to a publication additionally requires
+   owning that table. The <literal>ADD ALL TABLES IN SCHEMA</literal> and

SUGGESTION
Adding a table to or excluding a table from a publication additionally
requires owning that table.

~~~

13. doc/src/sgml/ref/alter_publication.sgml

@@ -213,6 +214,14 @@ ALTER PUBLICATION sales_publication ADD ALL
TABLES IN SCHEMA marketing, sales;
 </programlisting>
   </para>

+  <para>
+   Alter publication <structname>production_publication</structname> that
+   publishes all tables except <structname>users</structname> and
+   <structname>departments</structname> tables:
+<programlisting>

"that publishes" -> "to publish"

~~~

14. doc/src/sgml/ref/create_publication.sgml

(Same comment about the ONLY syntax as #11)

~~~

15. doc/src/sgml/ref/create_publication.sgml

+   <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>

IMO you can remove all that "It is not supported for..." sentence. You
don't need to spell that out again when it is already clear from the
syntax.

~~~

16. doc/src/sgml/ref/psql-ref.sgml

@@ -1868,8 +1868,9 @@ testdb=>
         If <replaceable class="parameter">pattern</replaceable> is
         specified, only those publications whose names match the pattern are
         listed.
-        If <literal>+</literal> is appended to the command name, the tables and
-        schemas associated with each publication are shown as well.
+        If <literal>+</literal> is appended to the command name, the tables,
+        excluded tables and schemas associated with each publication
are shown as
+        well.
         </para>

Perhaps this is OK just as-is, but OTOH I felt that the change was
almost unnecessary because saying it displays "the tables" kind of
implies it would also have to account for the "excluded tables" too.

~~~

17. src/backend/catalog/pg_publication.c - GetTopMostAncestorInPublication

@@ -302,8 +303,9 @@ GetTopMostAncestorInPublication(Oid puboid, List
*ancestors, int *ancestor_level
  foreach(lc, ancestors)
  {
  Oid ancestor = lfirst_oid(lc);
- List    *apubids = GetRelationPublications(ancestor);
+ List    *apubids = GetRelationPublications(ancestor, false);
  List    *aschemaPubids = NIL;
+ List    *aexceptpubids = NIL;

17a.
I think the var "aschemaPubids" and "aexceptpubids" are only used in
the 'else' block so it seems better they can be declared and freed in
that block too instead of always.

17b.
Also, the camel-case of those variables is inconsistent so may fix
that at the same time.

~~~

18. src/backend/catalog/pg_publication.c - GetRelationPublications

@@ -666,7 +673,7 @@ publication_add_schema(Oid pubid, Oid schemaid,
bool if_not_exists)

 /* Gets list of publication oids for a relation */
 List *
-GetRelationPublications(Oid relid)
+GetRelationPublications(Oid relid, bool bexcept)

18a.
I felt that "except_flag" is a better name than "bexcept" for this param.

18b.
The function comment should be updated to say only relations matching
this except_flag are returned in the list.

~~~

19. src/backend/catalog/pg_publication.c - GetAllTablesPublicationRelations

@@ -787,6 +795,15 @@ GetAllTablesPublicationRelations(bool pubviaroot)
  HeapTuple tuple;
  List    *result = NIL;

+ /*
+ * pg_publication_rel and pg_publication_namespace will only have excluded
+ * tables in case of all tables publication, no need to pass except flag
+ * to get the relations.
+ */
+ List    *exceptpubtablelist;
+
+ exceptpubtablelist = GetPublicationRelations(pubid, PUBLICATION_PART_ALL);
+

19a.
I wasn't very sure of the meaning/intent of the comment, but IIUC it
seems to be explaining why it is not necessary to use an "except_flag"
parameter in this code. Is it necessary/helpful to explain parameters
that do NOT exist?

19b.
The var name "exceptpubtablelist" seems a bit overkill. (e.g.
"excepttablelist" or "exceptlist" etc... are shorter but seem equally
informative).

~~~

20. src/backend/commands/publicationcmds.c  - CreatePublication

@@ -843,54 +849,52 @@ CreatePublication(ParseState *pstate,
CreatePublicationStmt *stmt)
  /* Make the changes visible. */
  CommandCounterIncrement();

- /* Associate objects with the publication. */
- if (stmt->for_all_tables)
- {
- /* Invalidate relcache so that publication info is rebuilt. */
- CacheInvalidateRelcacheAll();
- }
- else
- {
- ObjectsInPublicationToOids(stmt->pubobjects, pstate, &relations,
-    &schemaidlist);
+ ObjectsInPublicationToOids(stmt->pubobjects, pstate, &relations,
+ &schemaidlist);

- /* FOR ALL TABLES IN SCHEMA requires superuser */
- if (list_length(schemaidlist) > 0 && !superuser())
- ereport(ERROR,
- errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser to create FOR ALL TABLES IN SCHEMA publication"));
+ /* FOR ALL TABLES IN SCHEMA requires superuser */
+ if (list_length(schemaidlist) > 0 && !superuser())
+ ereport(ERROR,
+ errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("must be superuser to create FOR ALL TABLES IN SCHEMA publication"));

- if (list_length(relations) > 0)
- {
- List    *rels;
+ if (list_length(relations) > 0)
+ {
+ List    *rels;

- rels = OpenTableList(relations);
- CheckObjSchemaNotAlreadyInPublication(rels, schemaidlist,
-   PUBLICATIONOBJ_TABLE);
+ rels = OpenTableList(relations);
+ CheckObjSchemaNotAlreadyInPublication(rels, schemaidlist,
+ PUBLICATIONOBJ_TABLE);

- TransformPubWhereClauses(rels, pstate->p_sourcetext,
- publish_via_partition_root);
+ TransformPubWhereClauses(rels, pstate->p_sourcetext,
+ publish_via_partition_root);

- CheckPubRelationColumnList(rels, pstate->p_sourcetext,
-    publish_via_partition_root);
+ CheckPubRelationColumnList(rels, pstate->p_sourcetext,
+ publish_via_partition_root);

- PublicationAddTables(puboid, rels, true, NULL);
- CloseTableList(rels);
- }
+ PublicationAddTables(puboid, rels, true, NULL);
+ CloseTableList(rels);
+ }

- if (list_length(schemaidlist) > 0)
- {
- /*
- * Schema lock is held until the publication is created to prevent
- * concurrent schema deletion.
- */
- LockSchemaList(schemaidlist);
- PublicationAddSchemas(puboid, schemaidlist, true, NULL);
- }
+ if (list_length(schemaidlist) > 0)
+ {
+ /*
+ * Schema lock is held until the publication is created to prevent
+ * concurrent schema deletion.
+ */
+ LockSchemaList(schemaidlist);
+ PublicationAddSchemas(puboid, schemaidlist, true, NULL);
  }

  table_close(rel, RowExclusiveLock);

+ /* Associate objects with the publication. */
+ if (stmt->for_all_tables)
+ {
+ /* Invalidate relcache so that publication info is rebuilt. */
+ CacheInvalidateRelcacheAll();
+ }
+

This function is refactored a lot to not use "if/else" as it did
before. But AFAIK (maybe I misunderstood) this refactor doesn't seem
to actually have anything to do with the EXCEPT patch. If it really is
unrelated maybe it should not be part of this patch.

~~~

21. src/backend/commands/publicationcmds.c - CheckPublicationDefValues

+ if (pubform->puballtables)
+ return false;
+
+ if (!pubform->pubinsert || !pubform->pubupdate || !pubform->pubdelete ||
+ !pubform->pubtruncate || pubform->pubviaroot)
+ return false;

Now you have all the #define for the PUB_DEFAULT_XXX values, perhaps
this function should be using them instead of the hardcoded
assumptions what the default values are.

e.g.

if (pubform->puballtables != PUB_DEFAULT_ALL_TABLES) return false;
if (pubform->pubinsert != PUB_DEFAULT_ACTION_INSERT) return false;
...
etc.

~~~

22. src/backend/commands/publicationcmds.c -  CheckAlterPublication


@@ -1442,6 +1516,19 @@ CheckAlterPublication(AlterPublicationStmt
*stmt, HeapTuple tup,
    List *tables, List *schemaidlist)
 {
  Form_pg_publication pubform = (Form_pg_publication) GETSTRUCT(tup);
+ ListCell   *lc;
+ bool nonexcepttable = false;
+ bool excepttable = false;
+
+ foreach(lc, tables)
+ {
+ PublicationTable *pub_table = lfirst_node(PublicationTable, lc);
+
+ if (!pub_table->except)
+ nonexcepttable = true;
+ else
+ excepttable = true;
+ }

22a.
The names are very confusing. e.g. "nonexcepttable" is like a double-negative.

SUGGEST:
bool has_tables = false;
bool has_except_tables = false;

22b.
Reverse the "if" condition to be positive instead of negative (remove !)
e.g.
if (pub_table->except)
has_except_table = true;
else
has_table = true;

~~~

23. src/backend/commands/publicationcmds.c -  CheckAlterPublication

@@ -1461,12 +1548,19 @@ CheckAlterPublication(AlterPublicationStmt
*stmt, HeapTuple tup,
  errdetail("Tables from schema cannot be added to, dropped from, or
set on FOR ALL TABLES publications.")));

  /* Check that user is allowed to manipulate the publication tables. */
- if (tables && pubform->puballtables)
+ if (nonexcepttable && tables && pubform->puballtables)
  ereport(ERROR,

Seems no reason for "tables" to be in the condition since
"nonexcepttable" can't be true if "tables" is NIL.

~~~

24. src/backend/commands/publicationcmds.c -  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.")));

The errdetail message seems over-complex.

SUGGESTION
"EXCEPT TABLE clause is only allowed for FOR ALL TABLES publications."

~~~

25. src/backend/commands/publicationcmds.c - AlterPublication

@@ -1500,6 +1594,20 @@ AlterPublication(ParseState *pstate,
AlterPublicationStmt *stmt)
  aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_PUBLICATION,
     stmt->pubname);

+ if (stmt->for_all_tables)
+ {
+ bool isdefault = CheckPublicationDefValues(tup);
+
+ if (!isdefault)
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("Setting ALL TABLES requires publication \"%s\" to have
default values",
+    stmt->pubname),
+ errhint("Either the publication has tables/schemas associated or
does not have default publication options or ALL TABLES option is
set."));

The errhint message seems over-complex.

SUGGESTION
"Use ALTER PUBLICATION ... RESET"

~~~

26. src/bin/pg_dump/pg_dump.c - dumpPublication

@@ -3980,8 +3982,34 @@ dumpPublication(Archive *fout, const
PublicationInfo *pubinfo)
    qpubname);

  if (pubinfo->puballtables)
+ {
+ SimplePtrListCell *cell;
+ bool first = true;
  appendPQExpBufferStr(query, " FOR ALL TABLES");

+ /* Include exception tables if the publication has except tables */
+ for (cell = exceptinfo.head; cell; cell = cell->next)
+ {
+ PublicationRelInfo *pubrinfo = (PublicationRelInfo *) cell->ptr;
+ PublicationInfo *relpubinfo = pubrinfo->publication;
+ TableInfo  *tbinfo;
+
+ if (pubinfo == relpubinfo)
+ {
+ tbinfo = pubrinfo->pubtable;
+
+ if (first)
+ {
+ appendPQExpBufferStr(query, " EXCEPT TABLE ONLY");
+ first = false;
+ }
+ else
+ appendPQExpBufferStr(query, ", ");
+ appendPQExpBuffer(query, " %s", fmtQualifiedDumpable(tbinfo));
+ }
+ }
+ }
+

IIUC this usage of ONLY looks incorrect.

26a.
Firstly, if you want to hardwire ONLY then shouldn't it apply to every
of the except-list table, not just the first one? e.g. "EXCEPT TABLE
ONLY t1, ONLY t2, ONLY t3..."

26b.
Secondly, is it even correct to unconditionally hardwire the ONLY? How
do you know that is how the user wanted it?

~~~

27. src/bin/pg_dump/pg_dump.c

@@ -127,6 +127,8 @@ static SimpleOidList foreign_servers_include_oids
= {NULL, NULL};
 static SimpleStringList extension_include_patterns = {NULL, NULL};
 static SimpleOidList extension_include_oids = {NULL, NULL};

+static SimplePtrList exceptinfo = {NULL, NULL};
+

Probably I just did not understand how this logic works, but how does
this static work properly if there are multiple publications and 2
different EXCEPT lists? E.g. where is it clearing the "exceptinfo" so
that multiple EXCEPT TABLE lists don't become muddled?

~~~

28. src/bin/pg_dump/pg_dump.c - dumpPublicationTable

@@ -4330,8 +4378,11 @@ dumpPublicationTable(Archive *fout, const
PublicationRelInfo *pubrinfo)

  query = createPQExpBuffer();

- appendPQExpBuffer(query, "ALTER PUBLICATION %s ADD TABLE ONLY",
+ appendPQExpBuffer(query, "ALTER PUBLICATION %s ADD ",
    fmtId(pubinfo->dobj.name));
+
+ appendPQExpBufferStr(query, "TABLE ONLY");
+

That code refactor does not seem necessary for this patch.

~~~

29. src/bin/pg_dump/pg_dump_sort.c

@@ -90,6 +90,7 @@ enum dbObjectTypePriorities
  PRIO_FK_CONSTRAINT,
  PRIO_POLICY,
  PRIO_PUBLICATION,
+ PRIO_PUBLICATION_EXCEPT_REL,
  PRIO_PUBLICATION_REL,
  PRIO_PUBLICATION_TABLE_IN_SCHEMA,
  PRIO_SUBSCRIPTION,

I'm not sure how this enum is used (so perhaps this makes no
difference) but judging by the enum comment why did you put the sort
priority order PRIO_PUBLICATION_EXCEPT_REL before
PRIO_PUBLICATION_REL. Wouldn’t it make more sense the other way
around?

~~~

30. src/bin/psql/describe.c

@@ -2950,17 +2950,34 @@ describeOneTableDetails(const char *schemaname,
    "          WHERE attrelid = pr.prrelid AND attnum = prattrs[s])\n"
    "        ELSE NULL END) "
    "FROM pg_catalog.pg_publication p\n"
-   "     JOIN pg_catalog.pg_publication_rel pr ON p.oid = pr.prpubid\n"
-   "     JOIN pg_catalog.pg_class c ON c.oid = pr.prrelid\n"
-   "WHERE pr.prrelid = '%s'\n"
-   "UNION\n"
+   " JOIN pg_catalog.pg_publication_rel pr ON p.oid = pr.prpubid\n"
+   " JOIN pg_catalog.pg_class c ON c.oid = pr.prrelid\n"
+   "WHERE pr.prrelid = '%s'",
+   oid, oid, oid);

I feel that trailing "\n" ("WHERE pr.prrelid = '%s'\n") should not
have been removed.

~~~

31. src/bin/psql/describe.c

+ /* FIXME: 150000 should be changed to 160000 later for PG16. */
+ if (pset.sversion >= 150000)
+ appendPQExpBufferStr(&buf, " AND pr.prexcept = 'f'\n");
+
+ appendPQExpBuffer(&buf, "UNION\n"

The "UNION\n" param might be better wrapped onto the next line like it
used to be.

~~~

32. src/bin/psql/describe.c

+ /* FIXME: 150000 should be changed to 160000 later for PG16. */
+ if (pset.sversion >= 150000)
+ appendPQExpBuffer(&buf,
+   " AND NOT EXISTS (SELECT 1\n"
+   " FROM pg_catalog.pg_publication_rel pr\n"
+   " JOIN pg_catalog.pg_class pc\n"
+   "   ON pr.prrelid = pc.oid\n"
+   " WHERE pr.prrelid = '%s' AND pr.prpubid = p.oid)\n",
+   oid);

The whitespace indents in the SQL seem excessive here.

~~~

33. src/bin/psql/describe.c - describePublications

@@ -6322,6 +6344,22 @@ describePublications(const char *pattern)
  }
  }

+ /* FIXME: 150000 should be changed to 160000 later for PG16. */
+ if (pset.sversion >= 150000)
+ {
+ /* Get the excluded tables for the specified publication */
+ printfPQExpBuffer(&buf,
+   "SELECT concat(c.relnamespace::regnamespace, '.', c.relname)\n"
+   "FROM pg_catalog.pg_class c\n"
+   "     JOIN pg_catalog.pg_publication_rel pr ON c.oid = pr.prrelid\n"
+   "WHERE pr.prpubid = '%s'\n"
+   "  AND pr.prexcept = 't'\n"
+   "ORDER BY 1", pubid);
+ if (!addFooterToPublicationDesc(&buf, "Except tables:",
+ true, &cont))
+ goto error_return;
+ }
+

I think this code is misplaced. Shouldn't it be if/else and be above
the other 150000 check, otherwise when you change this to PG16 it may
not work as expected?

~~~

34. src/bin/psql/describe.c - describePublications

+ if (!addFooterToPublicationDesc(&buf, "Except tables:",
+ true, &cont))
+ goto error_return;
+ }

Should this be using the _T() macros same as the other prompts for translation?

~~~

35. src/include/catalog/pg_publication.h

I thought the param "bexpect" should be "except_flag".

(same comment as #18a)

~~~

36. src/include/catalog/pg_publication_rel.h

@@ -31,6 +31,7 @@ CATALOG(pg_publication_rel,6106,PublicationRelRelationId)
  Oid oid; /* oid */
  Oid prpubid BKI_LOOKUP(pg_publication); /* Oid of the publication */
  Oid prrelid BKI_LOOKUP(pg_class); /* Oid of the relation */
+ bool prexcept BKI_DEFAULT(f); /* except the relation */

SUGGEST (comment)
/* skip the relation */

~~~

37. src/include/commands/publicationcmds.h

@@ -32,8 +32,8 @@ extern ObjectAddress AlterPublicationOwner(const
char *name, Oid newOwnerId);
 extern void AlterPublicationOwner_oid(Oid pubid, Oid newOwnerId);
 extern void InvalidatePublicationRels(List *relids);
 extern bool pub_rf_contains_invalid_column(Oid pubid, Relation relation,
-    List *ancestors, bool pubviaroot);
+    List *ancestors, bool pubviaroot, bool alltables);
 extern bool pub_collist_contains_invalid_column(Oid pubid, Relation relation,
- List *ancestors, bool pubviaroot);
+ List *ancestors, bool pubviaroot, bool alltables);

Elsewhere in this patch, a similarly added param is called
"puballtables" (not "alltables"). Please check all places and use a
consistent param name for all of them.

~~~

38. src/test/regress/sql/publication.sql

There don't seem to be any tests for more than one EXCEPT TABLE (e.g.
no list tests?)

~~~

38. src/test/regress/sql/publication.sql

Maybe adjust all the below comments (a-d) to say "EXCEPT TABLES"
intead of "except tables"

38a.
+-- can't add except table to 'FOR ALL TABLES' publication

38b.
+-- can't add except table to 'FOR TABLE' publication

38c.
+-- can't add except table to 'FOR ALL TABLES IN SCHEMA' publication

38d.
+-- can't add except table when publish_via_partition_root option does not
+-- have default value

38e.
+-- can't add except table when the publication options does not have default
+-- values

SUGGESTION
can't add EXCEPT TABLE when the publication options are not the default values

~~~

39. .../t/032_rep_changes_except_table.pl

39a.
+# Check the table data does not sync for excluded table
+my $result = $node_subscriber->safe_psql('postgres',
+ "SELECT count(*), min(a), max(a) FROM sch1.tab1");
+is($result, qq(0||), 'check tablesync is excluded for excluded tables');

Maybe the "is" message should say "check there is no initial data
copied for the excluded table"

~~~


40 .../t/032_rep_changes_except_table.pl

+# Insert some data into few tables and verify that inserted data is not
+# replicated
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO sch1.tab1 VALUES(generate_series(11,20))");

The comment is not quite correct. You are inserting into only one
table here - not "few tables".

~~~

41. .../t/032_rep_changes_except_table.pl

+# Alter publication to exclude data changes in public.tab1 and verify that
+# subscriber does not get the new table data.

"new table data" -> "changed data for this table"

------
[1]
https://www.postgresql.org/message-id/TYCPR01MB83737C28187A6E0BADAE98F0EDCF9%40TYCPR01MB8373.jpnprd01.prod.outlook.com

Kind Regards,
Peter Smith.
Fujitsu Australia