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/
