Re: Skipping schema changes in publication

Hi Vignesh

Here are some review comments for the patch v24-0002.

These comments are just for the SGML docs. The patch needs a rebase so
I was unable to review the code.

======
Commit message

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

~

/to maintain/to flag/

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

2.
<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.
+ To create a publication using FOR ALL TABLES or FOR ALL TABLES IN SCHEMA,
+ the user must be a superuser. To add ALL TABLES or ALL TABLES IN SCHEMA to a
+ publication, the user must be a superuser. To add tables to a publication,
+ the user must have ownership rights on the table.
</para>

Those "FOR ALL TABLES" etc are missing SGML markup.

======
doc/src/sgml/ref/alter_publication.sgml

3.
+ALTER PUBLICATION <replaceable class="parameter">name</replaceable>
ADD ALL TABLES [ EXCEPT [ TABLE ] <replaceable
class="parameter">exception_object</replaceable> [, ... ] ]

and

+
+<phrase>where <replaceable
class="parameter">exception_object</replaceable> is:</phrase>
+
+ [ ONLY ] <replaceable class="parameter">table_name</replaceable> [ * ]
+

It is not clear from the syntax which of these is possible.

... ADD ALL TABLES EXCEPT TABLE t1,t2,t3
... ADD ALL TABLES EXCEPT TABLE t1, TABLE t2, TABLES t3

IMO it is best put the "[TABLE]" within the exception_object:
[ TABLE ] [ ONLY ] <replaceable class="parameter">table_name</replaceable> [ * ]

Then both are possible, which is consistent with how "FOR TABLE" syntax works.

Furthermore, you might want later to say EXCLUDE SEQUENCE, so doing it
this way makes that possible.

~~~

4.
- Adding a table to a publication additionally requires owning that table.
- The <literal>ADD TABLES IN SCHEMA</literal>,
+ Adding a table to or excluding a table from a publication additionally
+ requires owning that table. The <literal>ADD ALL TABLES</literal>,

This wording seems a bit awkward. How are re-phrasing like:

SUGGESTION
Adding or excluding a table from a publication requires ownership of that table.

~~~

5.
- name to explicitly indicate that descendant tables are included.
+ name to explicitly indicate that descendant tables are affected. For
+ partitioned tables, <literal>ONLY</literal> donot have any effect.

typo: /donot/does not/

======
doc/src/sgml/ref/create_publication.sgml

6.
- [ FOR ALL TABLES
+ [ FOR ALL TABLES [ EXCEPT [ TABLE ] <replaceable
class="parameter">exception_object</replaceable> [, ... ] ]
| FOR <replaceable
class="parameter">publication_object</replaceable> [, ... ] ]
[ WITH ( <replaceable
class="parameter">publication_parameter</replaceable> [= <replaceable
class="parameter">value</replaceable>] [, ... ] ) ]

@@ -30,6 +30,10 @@ CREATE PUBLICATION <replaceable
class="parameter">name</replaceable>

TABLE [ ONLY ] <replaceable
class="parameter">table_name</replaceable> [ * ] [ ( <replaceable
class="parameter">column_name</replaceable> [, ... ] ) ] [ WHERE (
<replaceable class="parameter">expression</replaceable> ) ] [, ... ]
TABLES IN SCHEMA { <replaceable
class="parameter">schema_name</replaceable> | CURRENT_SCHEMA } [, ...
]
+
+<phrase>where <replaceable
class="parameter">exception_object</replaceable> is:</phrase>
+
+ [ ONLY ] <replaceable class="parameter">table_name</replaceable> [ * ]

Same review comment as #3 before.

I think it is clearer (and more flexible) to change the
exception_object to include [TABLE].
[ TABLE ] [ ONLY ] <replaceable class="parameter">table_name</replaceable> [ * ]

It also helps pave the way for any future EXCLUDE SEQUENCE feature.

~~~

7.
+ <para>
+ This clause specifies a list of tables to be excluded from the
+ publication. It can only be used with <literal>FOR ALL TABLES</literal>.
+ If <literal>ONLY</literal> is specified before the table name, only
+ that table is excluded from the publication. If
<literal>ONLY</literal> is
+ not specified, the table and all its descendant tables (if any) are
+ excluded. Optionally, <literal>*</literal> can be specified after the
+ table name to explicitly indicate that descendant tables are excluded.
+ This does not apply to a partitioned table, however. The partitioned
+ table or its partitions are excluded from the publication based on the
+ parameter <literal>publish_via_partition_root</literal>.
+ </para>
+ <para>
+ When <literal>publish_via_partition_root</literal> is set to
+ <literal>true</literal>, specifying a root partitioned table in
+ <literal>EXCEPT TABLE</literal> excludes it and all its partitions from
+ replication. Specifying a leaf partition has no effect, as its
changes are
+ still replicated via the root partitioned table. When
+ <literal>publish_via_partition_root</literal> is set to
+ <literal>false</literal>, specifying a partitioned table or non-leaf
+ partition has no effect, as changes are replicated via the leaf
+ partitions. Specifying a leaf partition excludes only that partition from
+ replication.
+ </para>

I felt that the second paragraph should be started with the sentence
"The partitioned table or its partitions are excluded...", so then
everything related to "publish_via_partition_root" is kept together.

~~~

8.
+ <para>
+ Create a publication that publishes all changes in all the tables except for
+ the changes of <structname>users</structname> and
+ <structname>departments</structname>:
+<programlisting>
+CREATE PUBLICATION mypublication FOR ALL TABLES EXCEPT users, departments;
+</programlisting>
+ </para>

The words "the changes of" are not needed, and you did not use that
wording in the ALTER PUBLICATION example.

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

9.
If <literal>x</literal> is appended to the command name, the results
are displayed in expanded mode.
- 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>

/excluded tables and schemas/excluded tables, and schemas/

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