Dear Osumi-san, Thank you for reviewing! PSA new version.
> (1) > > + Decides the condition for exiting the walsender process. > + <literal>'wait_flush'</literal>, which is the default, the > walsender > + will wait for all the sent WALs to be flushed on the subscriber > side, > + before exiting the process. <literal>'immediate'</literal> will > exit > + without confirming the remote flush. This may break the consistency > + between publisher and subscriber, but it may be useful for a system > + that has a high-latency network to reduce the amount of time for > + shutdown. > > (1-1) > > The first part "exiting the walsender process" can be improved. > Probably, you can say "the exiting walsender process" or > "Decides the behavior of the walsender process at shutdown" instread. Fixed. Second idea was chosen. > (1-2) > > Also, the next sentence can be improved something like > "If the shutdown mode is wait_flush, which is the default, the > walsender waits for all the sent WALs to be flushed on the subscriber side. > If it is immediate, the walsender exits without confirming the remote flush". Fixed. > (1-3) > > We don't need to wrap wait_flush and immediate by single quotes > within the literal tag. This style was ported from the SNAPSHOT options part, so I decided to keep. > (2) > > + /* minapplydelay affects SHUTDOWN_MODE option */ > > I think we can move this comment to just above the 'if' condition > and combine it with the existing 'if' conditions comments. Moved and added some comments. > (3) 001_rep_changes.pl > > (3-1) Question > > In general, do we add this kind of check when we extend the protocol > (STREAM_REPLICATION command) > or add a new condition for apply worker exit ? > In case when we would like to know the restart of the walsender process in TAP > tests, > then could you tell me why the new test code matches the purpose of this > patch ? The replication command is not for normal user, so I think we don't have to test itself. The check that waits to restart the apply worker was added to improve the robustness. I think there is a possibility to fail the test when the apply worker recevies a transaction before it checks new subscription option. Now the failure can be avoided by confriming to reload pg_subscription and restart. > (3-2) > > + "Timed out while waiting for apply to restart after changing > min_apply_delay > to non-zero value"; > > Probably, we can partly change this sentence like below, because we check > walsender's pid. > FROM: "... while waiting for apply to restart..." > TO: "... while waiting for the walsender to restart..." Right, fixed. Best Regards, Hayato Kuroda FUJITSU LIMITED
v6-0001-Time-delayed-logical-replication-subscriber.patch
Description: v6-0001-Time-delayed-logical-replication-subscriber.patch
v6-0002-Extend-START_REPLICATION-command-to-accept-walsen.patch
Description: v6-0002-Extend-START_REPLICATION-command-to-accept-walsen.patch
