Dear Peter,
Thanks for giving comments. I want to reply some of them.
> > 17.
> >
> > "specifies promote"
> >
> > We can do double-quote for the word promote.
>
> The v30 patch has <literal>promote</literal>, which I think is adequate.
Opps. Actually I did look v29 patch while firstly reviewing. Sorry for noise.
>
> > 20.
> >
> > New options must be also documented as well. This helps not only users but
> also
> > reviewers.
> > (Sometimes we cannot identify that the implementation is intentinal or not.)
>
> Which ones are missing?
In v29, newly added options (publication/subscription/replication-slot) was not added.
Since they have been added, please ignore.
> > 21.
> >
> > Also, not sure the specification is good. I preferred to specify them by format
> > string. Because it can reduce the number of arguments and I cannot find use
> cases
> > which user want to control the name of objects.
> >
> > However, your approach has a benefit which users can easily identify the
> generated
> > objects by pg_createsubscriber. How do other think?
>
> I think listing them explicitly is better for the first version. It's
> simpler to implement and more flexible.
OK.
> > 22.
> >
> > ```
> > #define BASE_OUTPUT_DIR
> "pg_createsubscriber_output.d"
> > ```
> >
> > No one refers the define.
>
> This is gone in v30.
I wrote due to the above reason. Please ignore...
>
> > 31.
> >
> > ```
> > /* Create replication slot on publisher */
> > if (lsn)
> > pg_free(lsn);
> > ```
> >
> > I think allocating/freeing memory is not so efficient.
> > Can we add a flag to create_logical_replication_slot() for controlling the
> > returning value (NULL or duplicated string)? We can use the condition (i ==
> num_dbs-1)
> > as flag.
>
> Nothing is even using the return value of
> create_logical_replication_slot(). I think this can be removed altogether.
> > 37.
> >
> > ```
> > /* Register a function to clean up objects in case of failure */
> > atexit(cleanup_objects_atexit);
> > ```
> >
> > Sorry if we have already discussed. I think the registration can be moved just
> > before the boot of the standby. Before that, the callback will be no-op.
>
> But it can also stay where it is. What is the advantage of moving it later?
I thought we could reduce the risk of bugs. Previously some bugs were reported
because the registration is too early. However, this is not a strong opinion.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/