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

Dear Peter,

Thank you for giving comments! PSA new version.

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

Fixed.

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

Sorry, fixed.

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

I was betrayed by pgindent because of the reason you pointed out.
Fixed.

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

Your analysis is correct. Added.

> .../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')

This is because getopt_long implemented by GNU can accept incomplete options if
collect one can be predicted from input. E.g. pg_upgrade on linux can accept
`--ve` as `--verbose`, whereas the binary built on Windows cannot.

Anyway, the difference was not my expectation. Fixed.

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

Fixed.

Additionally, I fixed two bugs which are detected by AddressSanitizer.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED