On Fri, Jan 26, 2024, at 4:55 AM, Hayato Kuroda (Fujitsu) wrote: > Again, thanks for updating the patch! There are my random comments for v9.
Thanks for checking v9. I already incorporated some of the points below into the next patch. Give me a couple of hours to include some important points. > 01. > I cannot find your replies for my comments#7 [1] but you reverted related > changes. > I'm not sure you are still considering it or you decided not to include > changes. > Can you clarify your opinion? > (It is needed because changes are huge so it quite affects other > developments...) It is still on my list. As I said in a previous email I'm having a hard time reviewing pieces from your 0002 patch because you include a bunch of things into one patch. > 02. > ``` > + <term><option>-t <replaceable > class="parameter">seconds</replaceable></option></term> > + <term><option>--timeout=<replaceable > class="parameter">seconds</replaceable></option></term> > ``` > > But source codes required `--recovery-timeout`. Please update either of them, Oops. Fixed. My preference is --recovery-timeout because someone can decide to include a --timeout option for this tool. > 03. > ``` > + * Create a new logical replica from a standby server > ``` > > Junwang pointed out to change here but the change was reverted [2] > Can you clarify your opinion as well? I'll review the documentation once I fix the code. Since the tool includes the *create* verb into its name, it seems strange use another verb (convert) in the description. Maybe we should just remove the word *new* and a long description explains the action is to turn the standby (physical replica) into a logical replica. > 04. > ``` > +/* > + * Is the source server ready for logical replication? If so, create the > + * publications and replication slots in preparation for logical replication. > + */ > +static bool > +setup_publisher(LogicalRepInfo *dbinfo) > ``` > > But this function verifies the source server. I felt they should be in the > different function. I split setup_publisher() into check_publisher() and setup_publisher(). > 05. > ``` > +/* > + * Is the target server ready for logical replication? > + */ > +static bool > +setup_subscriber(LogicalRepInfo *dbinfo) > ```` > > Actually, this function does not set up subscriber. It just verifies whether > the > target can become a subscriber, right? If should be renamed. I renamed setup_subscriber() -> check_subscriber(). > 06. > ``` > + atexit(cleanup_objects_atexit); > ``` > > The registration of the cleanup function is too early. This sometimes triggers > a core-dump. E.g., I forgot to apply the atexit() patch. > 07. > Missing a removal of publications on the standby. It was on my list to do. It will be in the next patch. > 08. > Missing registration of LogicalRepInfo in the typedefs.list. Done. > 09 > ``` > + <refsynopsisdiv> > + <cmdsynopsis> > + <command>pg_subscriber</command> > + <arg rep="repeat"><replaceable>option</replaceable></arg> > + </cmdsynopsis> > + </refsynopsisdiv> > ``` > > Can you reply my comment#2 [4]? I think mandatory options should be written. I included the mandatory options into the synopsis. > 10. > Just to confirm - will you implement start_standby/stop_standby functions in > next version? It is still on my list. -- Euler Taveira EDB https://www.enterprisedb.com/
