Re: Include sequences in publications created by pg_createsubscriber

Hi Shlok.

Although I'd previously said "v6 LGTM", I decided to feed the v7 patch
through my AI to see what else it might come up with.

It got a bit pedantic, but OTOH the suggestions really *do* seem like
improvements to me, so I feel obliged to pass them on.

======
doc/src/sgml/ref/pg_createsubscriber.sgml

1.
+ replica from a physical standby server. By default,
+ <application>pg_createsubscriber</application> configures
+ <link linkend="logical-replication">logical replication</link> by
+ automatically creating internal publication and subscription objects. In
+ this mode, a publication and subscription are created for each database, and
+ the publications include all tables and sequences in their respective
+ databases.

"In this mode" is vague — consider "When using internally generated
publications," to be consistent with the terminology used later in the
same paragraph.

~~~

2.
+ which ensures each table is brought up to a synchronized state. When using
+ internally generated publications, they automatically include all sequences
+ in the database; however, their values are not automatically synchronized.

"they" is ambiguous — could refer to publications or sequences.
Suggest: "however, sequence values are not automatically
synchronized."

~~~

3.
+ For user-specified publications, sequences are only replicated if they were
+ explicitly added to those publications prior to running the tool, and like
+ the default mode, their values will require manual synchronization after the
+ upgrade.

"their values will require manual synchronization" — "will require" is
slightly awkward. Suggest "their values must be manually synchronized
after the upgrade."

Also "like the default mode" is a slightly informal comparison —
consider "as with the default mode."

~~~

4.
+ <para>
+ To synchronize sequence values, run
+ <link linkend="sql-altersubscription-params-refresh-sequences">
+ <command>ALTER SUBSCRIPTION ... REFRESH SEQUENCES</command></link>.
+ </para>
```
This is quite abrupt as a standalone paragraph. It doesn't say *when*
to run it or *which* subscription to run it against. Something like:

"To synchronize sequence values after the upgrade, run `ALTER
SUBSCRIPTION ... REFRESH SEQUENCES` on each subscription at the target
server."

~~~

5.
+ specified, <application>pg_createsubscriber</application> creates a
+ publication using <link
linkend="sql-createpublication-params-for-all-tables">
+ <literal>FOR ALL TABLES</literal></link> and
+ <link linkend="sql-createpublication-params-for-all-sequences">
+ <literal>ALL SEQUENCES</literal></link>. Otherwise, it uses the
+ user-specified publication, which includes the objects defined by that
+ publication. If the <option>--publication</option> option is not
+ specified, the publication has the following name pattern:

SUGGESTION avoid repeating --publication by rearranging like below:

Create a publication and replication slot for each specified database
on the source server. If the <option>--publication</option> option is
specified, the user-specified publication is used, which includes the
objects defined by that publication. Otherwise,
<application>pg_createsubscriber</application> creates a publication
using <link linkend="sql-createpublication-params-for-all-tables">
<literal>FOR ALL TABLES</literal></link> and <link
linkend="sql-createpublication-params-for-all-sequences"> <literal>ALL
SEQUENCES</literal></link>. This generated publication has the
following name pattern:
<quote><literal>pg_createsubscriber_%u_%x</literal></quote>
(parameter:
database <parameter>oid</parameter>, random <parameter>int</parameter>).

======
src/bin/pg_basebackup/t/040_pg_createsubscriber.pl

6.
+$result = $node_p->safe_psql($db2,
+ "SELECT puballtables, puballsequences FROM pg_publication");
+is($result, qq(t|t),
+ "publication is created with both all-tables and all-sequences enabled");

6a. The query has no WHERE clause — if there are multiple publications
it could return multiple rows and give a misleading result. Should
filter by publication name, e.g. WHERE pubname LIKE
'pg_createsubscriber_%'.

~

6b. Should there also be a test that verifies a user-specified
publication is not modified (i.e. does not have ALL SEQUENCES added)
when --publication is specified? That would give better coverage of
the two code paths.

======
GENERAL

7.
The commit message says "Administrators can then run `ALTER
SUBSCRIPTION ... REFRESH SEQUENCES` after the upgrade" but the
documentation does not make this prominent enough. Given this is the
key action administrators need to take, it deserves a more prominent
note — perhaps a `<note>` or `<tip>` block rather than a brief
standalone paragraph.

(I think this is related with point 4)

~~~

8.
There is no mention anywhere in the patch of what happens to sequence
values if the administrator forgets to run REFRESH SEQUENCES before
promoting the subscriber. A note about the risk (duplicate key
violations, incorrect nextval results) would make the documentation
more complete and help administrators understand why this step
matters.

(Again, I think this is related with point 4)

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