RE: Exit walsender before confirming remote flush in logical replication

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