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