Re: [PoC] pg_upgrade: allow to upgrade publisher node

Here are a few more review comments for patch v3-0001.

======
doc/src/sgml/ref/pgupgrade.sgml

1.
+ <varlistentry>
+ <term><option>--include-logical-replication-slots</option></term>
+ <listitem>
+ <para>
+ Upgrade logical replication slots. Only permanent replication slots
+ included. Note that pg_upgrade does not check the installation of
+ plugins.
+ </para>
+ </listitem>
+ </varlistentry>

Missing word.

"Only permanent replication slots included." --> "Only permanent
replication slots are included."

======
src/bin/pg_dump/pg_dump.c

2. help

@@ -1119,6 +1145,8 @@ help(const char *progname)
printf(_(" --no-unlogged-table-data do not dump unlogged table data\n"));
printf(_(" --on-conflict-do-nothing add ON CONFLICT DO NOTHING
to INSERT commands\n"));
printf(_(" --quote-all-identifiers quote all identifiers, even
if not key words\n"));
+ printf(_(" --logical-replication-slots-only\n"
+ " dump only logical replication slots,
no schema or data\n"));
printf(_(" --rows-per-insert=NROWS number of rows per INSERT;
implies --inserts\n"));
A previous review comment ([1] #11b) seems to have been missed. This
help is misplaced. It should be in alphabetical order consistent with
all the other help.

======
src/bin/pg_dump/pg_dump.h

3. _LogicalReplicationSlotInfo

+/*
+ * The LogicalReplicationSlotInfo struct is used to represent replication
+ * slots.
+ * XXX: add more attrbutes if needed
+ */
+typedef struct _LogicalReplicationSlotInfo
+{
+ DumpableObject dobj;
+ char *plugin;
+ char *slottype;
+ char *twophase;
+} LogicalReplicationSlotInfo;
+

4a.
The indent of the 'LogicalReplicationSlotInfo' looks a bit strange,
unlike others in this file. Is it OK?

~

4b.
There was no typedefs.list file in this patch. Maybe the above
whitespace problem is a result of that omission.

======
.../pg_upgrade/t/003_logical_replication.pl

5.

+# Run pg_upgrade. pg_upgrade_output.d is removed at the end
+command_ok(
+ [
+ 'pg_upgrade', '--no-sync',
+ '-d', $old_publisher->data_dir,
+ '-D', $new_publisher->data_dir,
+ '-b', $bindir,
+ '-B', $bindir,
+ '-s', $new_publisher->host,
+ '-p', $old_publisher->port,
+ '-P', $new_publisher->port,
+ $mode, '--include-logical-replication-slot'
+ ],
+ 'run of pg_upgrade for new publisher');

5a.
How can this test even be working as-expected with those options?

Here it is passing option '--include-logical-replication-slot' but
AFAIK the proper option name everywhere else in this patch is
'--include-logical-replication-slots' (with the 's')

~

5b.
I'm not sure that "pg_upgrade for new publisher" makes sense.

It's more like "pg_upgrade of old publisher", or simply "pg_upgrade of
publisher"

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

Kind Regards,
Peter Smith.
Fujitsu Australia