Horton

RE: Exit walsender before confirming remote flush in logical replication

Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>

View thread
From: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>
To: "Takamichi Osumi (Fujitsu)" <osumi.takamichi@fujitsu.com>
Cc: "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>, "sawada.mshk@gmail.com" <sawada.mshk@gmail.com>, "michael@paquier.xyz" <michael@paquier.xyz>, "peter.eisentraut@enterprisedb.com" <peter.eisentraut@enterprisedb.com>, "dilipbalaut@gmail.com" <dilipbalaut@gmail.com>, "andres@anarazel.de" <andres@anarazel.de>, "amit.kapila16@gmail.com" <amit.kapila16@gmail.com>, 'Kyotaro Horiguchi' <horikyota.ntt@gmail.com>
Date: 2023-02-09T10:11:10Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Add wal_sender_shutdown_timeout GUC to limit shutdown wait for replication

  2. pg_upgrade: Add --copy option

  3. Prevent possibility of panics during shutdown checkpoint.

  4. Support clean switchover.

Attachments

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