Re: Skipping schema changes in publication

On Tue, May 17, 2022 at 7:35 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> 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].

I have made TABLE optional.

> ~~~
>
> 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?

Earlier we had discussed whether to use SKIP, but felt SKIP was not
appropriate and planned to use except as in [1]. Let's use except
unless we find a better alternative.

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

I have kept the behavior similar to FOR TABLE

> b) There are no test cases using ONLY.

Added tests for the same

> ~~~
>
> 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.

Modified

> ~~
>
> 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:

Modified

> ~~~
>
> 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.

Modified

> ~~~
>
> 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.

Modified

> ~~~
>
> 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.

Modified

> ~~~
>
> 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.

Modified

> ~~~
>
> 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?

Earlier we were not allowed to add ALL TABLES, while altering
publication. This is mentioned in this patch as we suport:
ALTER PUBLICATION pubname ADD ALL TABLES syntax.

> ~~~
>
> 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 [ * ]

Modified

> ~~~
>
> 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.

Modified

> ~~~
>
> 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"

Modified

> ~~~
>
> 14. doc/src/sgml/ref/create_publication.sgml
>
> (Same comment about the ONLY syntax as #11)

Modified

> ~~~
>
> 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.

Modified

> ~~~
>
> 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.

I mentioned it that way so that it is clearer and to avoid confusions
to be pointed out by other members later. I felt let's keep it this
way.

> ~~~
>
> 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.

Modified

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

Modified

> ~~~
>
> 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.

Modified

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

Modified

> ~~~
>
> 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?

I have removed it

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

Modified

> ~~~
>
> 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.

Earlier tables cannot be specified with all tables, now except tables
can be specified with all tables, except tables should be added to
pg_publication_rel, to handle it the code changes are required.

> ~~~
>
> 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.

Modified

> ~~~
>
> 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;

This code can be removed because of grammar optimization, it will not
allow except table without "ALL TABLES". Removed these changes.

> ~~~
>
> 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.

This code can be removed because of grammar optimization, it will not
allow except table without "ALL TABLES". Removed these changes.

> ~~~
>
> 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."

This code can be removed because of grammar optimization, it will not
allow except table without "ALL TABLES". Removed this code

> ~~~
>
> 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"

Modified

> ~~~
>
> 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..."

Modified, included ONLY for all the tables

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

The table ONLY selection is handled appropriately while creating
publication and stored in pg_publication_rel. When we dump all the
parent and child table will be included specifying ONLY will handle
both scenarios with and without ONLY. This is the same behavior as in
FOR TABLE publication

> ~~~
>
> 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?

Currently exceptinfo holds all the exception tables and the
corresponding publications. When we dump the publication it will
select the appropriate exception tables that correspond to the
publication and dump the exception tables associated for this
publication. Since this is a special syntax "CREATE PUBLICATION FOR
ALL TABLES EXCEPT TABLE tb1 .." all the except tables should be
specified in a single statement unlike the other publication objects.

> ~~~
>
> 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.

Modified

> ~~~
>
> 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?

This order does not matter, since the new syntax is like "CREATE
PUBLICATION.. FOR ALL TABLES EXCEPT TABLE ....", all the except tables
need to be accumulated and handled during dump publication. This code
changes take care of accumulating the exception table which will be
used later by dump publication

> ~~~
>
> 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.

Modified

> ~~~
>
> 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.

Modified

> ~~~
>
> 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.

Modified

> ~~~
>
> 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?

I moved this to else. I felt this is applicable only for all tables
publication. Just keeping in else is fine.

> ~~~
>
> 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?

Modified

> ~~~
>
> 35. src/include/catalog/pg_publication.h
>
> I thought the param "bexpect" should be "except_flag".
>
> (same comment as #18a)

Modified

> ~~~
>
> 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 */

Changed it to exclude 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.

Modified

> ~~~
>
> 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?)

Modified

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

Modified
> ~~~
>
> 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"

Modified

> ~~~
>
>
> 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".

Modified

> ~~~
>
> 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"

Modified

Thanks for the comments, the v6 patch attached at [2] has the changes
for the same.
[1] - https://www.postgresql.org/message-id/a2004f08-eb2f-b124-115c-f8f18667e585%40enterprisedb.com
[2] - https://www.postgresql.org/message-id/CALDaNm0iZZDB300Dez_97S8G6_RW5QpQ8ef6X3wq8tyK-8wnXQ%40mail.gmail.com

Regards,
Vignesh