Dear Peter,
Thank you for checking. Then we can wait comments from others.
PSA modified version.
> 1.
> There were a couple of comments that I thought would appear less
> squished (aka more readable) if there was a blank line preceding the
> XXX.
>
> 1a. This one is in getLogicalReplicationSlots
>
> + /*
> + * Get replication slots.
> + *
> + * XXX: Which information must be extracted from old node? Currently three
> + * attributes are extracted because they are used by
> + * pg_create_logical_replication_slot().
> + * XXX: Do we have to support physical slots?
> + */
Added.
> 1b. This one is for the LogicalReplicationSlotInfo typedef
>
> +/*
> + * The LogicalReplicationSlotInfo struct is used to represent replication
> + * slots.
> + * XXX: add more attrbutes if needed
> + */
Added.
> BTW -- I just noticed there is a typo in that comment. /attrbutes/attributes/
Good finding, replaced.
> src/bin/pg_dump/pg_dump_sort.c
>
> 2. describeDumpableObject
>
> + case DO_LOGICAL_REPLICATION_SLOT:
> + snprintf(buf, bufsize,
> + "REPLICATION SLOT (ID %d NAME %s)",
> + obj->dumpId, obj->name);
> + return;
>
> Since everything else was changed to say logical replication slot,
> should this string be changed to "LOGICAL REPLICATION SLOT (ID %d NAME
> %s)"?
I missed to replace, changed.
> .../pg_upgrade/t/003_logical_replication.pl
>
> 3.
> Should the name of this TAP test file really be 03_logical_replication_slots.pl?
>
Hmm, not sure. Currently I renamed once according to your advice, but personally
another feature which allows to upgrade subscriber[1] should be tested in the same
perl file. That's why I named as "003_logical_replication.pl"
[1]: https://www.postgresql.org/message-id/20230217075433.u5mjly4d5cr4hcfe%40jrouhaud
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
