RE: [PoC] pg_upgrade: allow to upgrade publisher node - Mailing list pgsql-hackers

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