> On Aug 7, 2026, at 08:24, Peter Smith <[email protected]> wrote: > > Some minor review comments for patch v2. Nothing functional.
Thank you very much for reviewing.
>
> ======
> doc/src/sgml/ref/pg_createsubscriber.sgml
>
> 1.
> is reported. The order of the multiple subscription name switches must
> match the order of database switches. If this option is not specified,
> a generated name is assigned to the subscription name. This
> option cannot
> - be used together with <option>--all</option>.
> + be used together with <option>--all</option>. The same
> subscription name
> + can be used in different databases only when replication slot names
> are
> + specified with <option>--replication-slot</option>.
> </para>
>
> The new sentence LGTM. But my AI is pointing out that now the "-all"
> sentence sits wedged between the parts describing names (order of
> names, generated names, same names). It recommends moving the "--all"
> sentence like below:
>
> SUGGESTION
> The subscription name to set up the logical replication. This option
> cannot be used together with --all. Multiple subscriptions can be
> specified by writing multiple --subscription switches. The number of
> subscription names must match the number of specified databases,
> otherwise an error is reported. The order of the multiple subscription
> name switches must match the order of database switches. If this
> option is not specified, a generated name is assigned to the
> subscription name. The same subscription name can be used in
> different databases only when replication slot names are specified
> with --replication-slot.
>
Accepted.
>
> ======
> src/bin/pg_basebackup/pg_createsubscriber.c
>
> 2.
> int option_index;
> + bool duplicate_sub_name = false;
>
> Should it be plural?
>
> /duplicate_sub_name/duplicate_sub_names/
>
Accepted.
> ~~~
>
> 3.
> + if (simple_string_list_member(&opt.sub_names, optarg))
> + duplicate_sub_name = true;
>
> Having found at least one duplicate, you don't really need to keep checking.
>
> SUGGESTION
> if (!duplicate_sub_name)
> duplicate_sub_name = simple_string_list_member(&opt.sub_names, optarg);
>
> Anyway, the extra checking is cheap, so feel free to ignore this comment.
>
Okay, why not.
> ~~~
>
> 4.
> if (num_replslots > 0 && num_replslots != num_dbs)
> {
> pg_log_error("wrong number of replication slot names specified");
> pg_log_error_detail("The number of specified replication slot names
> (%d) must match the number of specified database names (%d).",
> num_replslots, num_dbs);
> exit(1);
> }
> if (duplicate_sub_name && num_replslots == 0)
> {
> pg_log_error("duplicate subscription names require replication slot names");
> pg_log_error_hint("Specify --replication-slot for each database.");
> exit(1);
> }
>
> The code LGTM, but would it be tidier to avoid multiple
> `num_replslots` checks by combining as a single if/else?
>
> SUGGESTION
> if (num_replslots == 0)
> {
> if (duplicate_sub_name)
> {
> pg_log_error ...
> }
> }
> else
> {
> if (num_replslots != num_dbs)
> {
> pg_log_error ...
> }
> }
>
Accepted.
PFA v3: addressed Peter’s comments.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
v3-0001-pg_createsubscriber-Allow-duplicate-subscription-.patch
Description: Binary data
