Horton
Menu

Thread

Commits

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

  1. Avoid blocking indefinitely while finishing walsender shutdown

  2. Add wal_sender_shutdown_timeout GUC to limit shutdown wait for replication

  3. pg_upgrade: Add --copy option

  4. Prevent possibility of panics during shutdown checkpoint.

  5. Support clean switchover.

  1. Exit walsender before confirming remote flush in logical replication

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2022-12-22T05:46:11Z

    Dear hackers,
(I added Amit as CC because we discussed in another thread)

This is a fork thread from time-delayed logical replication [1].
While discussing, we thought that we could extend the condition of walsender shutdown[2][3].

Currently, walsenders delay the shutdown request until confirming all sent data
are flushed on remote side. This condition was added in 985bd7[4], which is for
supporting clean switchover. Supposing that there is a primary-secondary
physical replication system, and do following steps. If any changes are come
while step 2 but the walsender does not confirm the remote flush, the reboot in
step 3 may be failed.

1. Stops primary server.
2. Promotes secondary to new primary.
3. Reboot (old)primary as new secondary.

In case of logical replication, however, we cannot support the use-case that
switches the role publisher <-> subscriber. Suppose same case as above, additional
transactions are committed while doing step2. To catch up such changes subscriber
must receive WALs related with trans, but it cannot be done because subscriber
cannot request WALs from the specific position. In the case, we must truncate all
data in new subscriber once, and then create new subscription with copy_data
= true.

Therefore, I think that we can ignore the condition for shutting down the
walsender in logical replication.

This change may be useful for time-delayed logical replication. The walsender
waits the shutdown until all changes are applied on subscriber, even if it is
delayed. This causes that publisher cannot be stopped if large delay-time is
specified.

PSA the minimal patch for that. I'm not sure whether WalSndCaughtUp should be
also omitted or not. It seems that changes may affect other parts like
WalSndWaitForWal(), but we can investigate more about it.

[1]: https://commitfest.postgresql.org/41/3581/
[2]: https://www.postgresql.org/message-id/TYAPR01MB58661BA3BF38E9798E59AE14F5E19%40TYAPR01MB5866.jpnprd01.prod.outlook.com
[3]: https://www.postgresql.org/message-id/CAA4eK1LyetktcphdRrufHac4t5DGyhsS2xG2DSOGb7OaOVcDVg%40mail.gmail.com
[4]: https://github.com/postgres/postgres/commit/985bd7d49726c9f178558491d31a570d47340459

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

  2. Re: Exit walsender before confirming remote flush in logical replication

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2022-12-22T11:59:34Z

    On Thu, Dec 22, 2022 at 11:16 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear hackers,
> (I added Amit as CC because we discussed in another thread)
>
> This is a fork thread from time-delayed logical replication [1].
> While discussing, we thought that we could extend the condition of walsender shutdown[2][3].
>
> Currently, walsenders delay the shutdown request until confirming all sent data
> are flushed on remote side. This condition was added in 985bd7[4], which is for
> supporting clean switchover. Supposing that there is a primary-secondary
> physical replication system, and do following steps. If any changes are come
> while step 2 but the walsender does not confirm the remote flush, the reboot in
> step 3 may be failed.
>
> 1. Stops primary server.
> 2. Promotes secondary to new primary.
> 3. Reboot (old)primary as new secondary.
>
> In case of logical replication, however, we cannot support the use-case that
> switches the role publisher <-> subscriber. Suppose same case as above, additional
> transactions are committed while doing step2. To catch up such changes subscriber
> must receive WALs related with trans, but it cannot be done because subscriber
> cannot request WALs from the specific position. In the case, we must truncate all
> data in new subscriber once, and then create new subscription with copy_data
> = true.
>
> Therefore, I think that we can ignore the condition for shutting down the
> walsender in logical replication.
>
> This change may be useful for time-delayed logical replication. The walsender
> waits the shutdown until all changes are applied on subscriber, even if it is
> delayed. This causes that publisher cannot be stopped if large delay-time is
> specified.

I think the current behaviour is an artifact of using the same WAL
sender code for both logical and physical replication.

I agree with you that the logical WAL sender need not wait for all the
WAL to be replayed downstream.

I have not reviewed the patch though.

-- 
Best Wishes,
Ashutosh Bapat

  3. Re: Exit walsender before confirming remote flush in logical replication

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-12-23T02:21:54Z

    At Thu, 22 Dec 2022 17:29:34 +0530, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote in 
> On Thu, Dec 22, 2022 at 11:16 AM Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
> > In case of logical replication, however, we cannot support the use-case that
> > switches the role publisher <-> subscriber. Suppose same case as above, additional
..
> > Therefore, I think that we can ignore the condition for shutting down the
> > walsender in logical replication.
...
> > This change may be useful for time-delayed logical replication. The walsender
> > waits the shutdown until all changes are applied on subscriber, even if it is
> > delayed. This causes that publisher cannot be stopped if large delay-time is
> > specified.
> 
> I think the current behaviour is an artifact of using the same WAL
> sender code for both logical and physical replication.

Yeah, I don't think we do that for the reason of switchover. On the
other hand I think the behavior was intentionally taken over since it
is thought as sensible alone. And I'm afraind that many people already
relies on that behavior.

> I agree with you that the logical WAL sender need not wait for all the
> WAL to be replayed downstream.

Thus I feel that it might be a bit outrageous to get rid of that
bahavior altogether because of a new feature stumbling on it.  I'm
fine doing that only in the apply_delay case, though.  However, I have
another concern that we are introducing the second exception for
XLogSendLogical in the common path.

I doubt that anyone wants to use synchronous logical replication with
apply_delay since the sender transaction is inevitablly affected back
by that delay.

Thus how about before entering an apply_delay, logrep worker sending a
kind of crafted feedback, which reports commit_data.end_lsn as
flushpos?  A little tweak is needed in send_feedback() but seems to
work..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  4. Re: Exit walsender before confirming remote flush in logical replication

    Amit Kapila <amit.kapila16@gmail.com> — 2022-12-23T05:56:21Z

    On Fri, Dec 23, 2022 at 7:51 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Thu, 22 Dec 2022 17:29:34 +0530, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote in
> > On Thu, Dec 22, 2022 at 11:16 AM Hayato Kuroda (Fujitsu)
> > <kuroda.hayato@fujitsu.com> wrote:
> > > In case of logical replication, however, we cannot support the use-case that
> > > switches the role publisher <-> subscriber. Suppose same case as above, additional
> ..
> > > Therefore, I think that we can ignore the condition for shutting down the
> > > walsender in logical replication.
> ...
> > > This change may be useful for time-delayed logical replication. The walsender
> > > waits the shutdown until all changes are applied on subscriber, even if it is
> > > delayed. This causes that publisher cannot be stopped if large delay-time is
> > > specified.
> >
> > I think the current behaviour is an artifact of using the same WAL
> > sender code for both logical and physical replication.
>
> Yeah, I don't think we do that for the reason of switchover. On the
> other hand I think the behavior was intentionally taken over since it
> is thought as sensible alone.
>

Do you see it was discussed somewhere? If so, can you please point to
that discussion?

> And I'm afraind that many people already
> relies on that behavior.
>

But OTOH, it can also be annoying for users to see some wait during
the shutdown which is actually not required.

> > I agree with you that the logical WAL sender need not wait for all the
> > WAL to be replayed downstream.
>
> Thus I feel that it might be a bit outrageous to get rid of that
> bahavior altogether because of a new feature stumbling on it.  I'm
> fine doing that only in the apply_delay case, though.  However, I have
> another concern that we are introducing the second exception for
> XLogSendLogical in the common path.
>
> I doubt that anyone wants to use synchronous logical replication with
> apply_delay since the sender transaction is inevitablly affected back
> by that delay.
>
> Thus how about before entering an apply_delay, logrep worker sending a
> kind of crafted feedback, which reports commit_data.end_lsn as
> flushpos?  A little tweak is needed in send_feedback() but seems to
> work..
>

How can we send commit_data.end_lsn before actually committing the
xact? I think this can lead to a problem because next time (say after
restart of walsender) server can skip sending the xact even if it is
not committed by the client.


-- 
With Regards,
Amit Kapila.

  5. RE: Exit walsender before confirming remote flush in logical replication

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2022-12-23T12:54:15Z

    Dear Horiguchi-san,

> Thus how about before entering an apply_delay, logrep worker sending a
> kind of crafted feedback, which reports commit_data.end_lsn as
> flushpos?  A little tweak is needed in send_feedback() but seems to
> work..

Thanks for replying! I tested your saying but it could not work well...

I made PoC based on the latest time-delayed patches [1] for non-streaming case.
Apply workers that are delaying applications send begin_data.final_lsn as recvpos and flushpos in send_feedback().

Followings were contents of the feedback message I got, and we could see that recv and flush were overwritten.

```
DEBUG:  sending feedback (force 1) to recv 0/1553638, write 0/1553550, flush 0/1553638
CONTEXT:  processing remote data for replication origin "pg_16390" during message type "BEGIN" in transaction 730, finished at 0/1553638
```

In terms of walsender, however, sentPtr seemed to be slightly larger than flushed position on subscriber.

```
(gdb) p MyWalSnd->sentPtr 
$2 = 22361760
(gdb) p MyWalSnd->flush
$3 = 22361656
(gdb) p *MyWalSnd
$4 = {pid = 28807, state = WALSNDSTATE_STREAMING, sentPtr = 22361760, needreload = false, write = 22361656, 
  flush = 22361656, apply = 22361424, writeLag = 20020343, flushLag = 20020343, applyLag = 20020343, 
  sync_standby_priority = 0, mutex = 0 '\000', latch = 0x7ff0350cbb94, replyTime = 725113263592095}
```

Therefore I could not shut down the publisher node when applications were delaying.
Do you have any opinions about them?

```
$ pg_ctl stop -D data_pub/
waiting for server to shut down............................................................... failed
pg_ctl: server does not shut down
```

[1]: https://www.postgresql.org/message-id/TYCPR01MB83730A3E21E921335F6EFA38EDE89@TYCPR01MB8373.jpnprd01.prod.outlook.com 

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

  6. RE: Exit walsender before confirming remote flush in logical replication

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2022-12-26T12:27:24Z

    Dear Horiguchi-san,

> > Thus how about before entering an apply_delay, logrep worker sending a
> > kind of crafted feedback, which reports commit_data.end_lsn as
> > flushpos?  A little tweak is needed in send_feedback() but seems to
> > work..
> 
> Thanks for replying! I tested your saying but it could not work well...
> 
> I made PoC based on the latest time-delayed patches [1] for non-streaming case.
> Apply workers that are delaying applications send begin_data.final_lsn as recvpos
> and flushpos in send_feedback().

Maybe I misunderstood what you said... I have also found that sentPtr is not the actual sent
position, but the starting point of the next WAL. You can see the comment below.

```
/*
 * How far have we sent WAL already? This is also advertised in
 * MyWalSnd->sentPtr.  (Actually, this is the next WAL location to send.)
 */
static XLogRecPtr sentPtr = InvalidXLogRecPtr;
```

We must use end_lsn for crafting messages to cheat the walsender, but such records
are included in COMMIT, not in BEGIN for the non-streaming case.
But if workers are delayed in apply_handle_commit(), will they hold locks for database
objects for a long time and it causes another issue.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

  7. Re: Exit walsender before confirming remote flush in logical replication

    Dilip Kumar <dilipbalaut@gmail.com> — 2022-12-27T07:40:05Z

    On Thu, Dec 22, 2022 at 11:16 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

> In case of logical replication, however, we cannot support the use-case that
> switches the role publisher <-> subscriber. Suppose same case as above, additional
> transactions are committed while doing step2. To catch up such changes subscriber
> must receive WALs related with trans, but it cannot be done because subscriber
> cannot request WALs from the specific position. In the case, we must truncate all
> data in new subscriber once, and then create new subscription with copy_data
> = true.
>
> Therefore, I think that we can ignore the condition for shutting down the
> walsender in logical replication.
>
+1 for the idea.

- * Note that if we determine that there's still more data to send, this
- * function will return control to the caller.
+ * Note that if we determine that there's still more data to send or we are in
+ * the physical replication more, this function will return control to the
+ * caller.

I think in this comment you meant to say

1. "or we are in physical replication mode and all WALs are not yet replicated"
2. Typo /replication more/replication mode

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

  8. RE: Exit walsender before confirming remote flush in logical replication

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2022-12-27T08:14:13Z

    Dear Dilip,

Thanks for checking my proposal!

> - * Note that if we determine that there's still more data to send, this
> - * function will return control to the caller.
> + * Note that if we determine that there's still more data to send or we are in
> + * the physical replication more, this function will return control to the
> + * caller.
> 
> I think in this comment you meant to say
> 
> 1. "or we are in physical replication mode and all WALs are not yet replicated"
> 2. Typo /replication more/replication mode

Firstly I considered 2, but I thought 1 seemed to be better.
PSA the updated patch.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

  9. Re: Exit walsender before confirming remote flush in logical replication

    Amit Kapila <amit.kapila16@gmail.com> — 2022-12-27T09:20:48Z

    On Tue, Dec 27, 2022 at 1:44 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Thanks for checking my proposal!
>
> > - * Note that if we determine that there's still more data to send, this
> > - * function will return control to the caller.
> > + * Note that if we determine that there's still more data to send or we are in
> > + * the physical replication more, this function will return control to the
> > + * caller.
> >
> > I think in this comment you meant to say
> >
> > 1. "or we are in physical replication mode and all WALs are not yet replicated"
> > 2. Typo /replication more/replication mode
>
> Firstly I considered 2, but I thought 1 seemed to be better.
> PSA the updated patch.
>

I think even for logical replication we should check whether there is
any pending WAL (via pq_is_send_pending()) to be sent. Otherwise, what
is the point to send the done message? Also, the caller of
WalSndDone() already has that check which is another reason why I
can't see why you didn't have the same check in function WalSndDone().

BTW, even after fixing this, I think logical replication will behave
differently when due to some reason (like time-delayed replication)
send buffer gets full and walsender is not able to send data. I think
this will be less of an issue with physical replication because there
is a separate walreceiver process to flush the WAL which doesn't wait
but the same is not true for logical replication. Do you have any
thoughts on this matter?

-- 
With Regards,
Amit Kapila.

  10. Re: Exit walsender before confirming remote flush in logical replication

    Amit Kapila <amit.kapila16@gmail.com> — 2022-12-27T09:25:07Z

    On Tue, Dec 27, 2022 at 2:50 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Dec 27, 2022 at 1:44 PM Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
> >
> > Thanks for checking my proposal!
> >
> > > - * Note that if we determine that there's still more data to send, this
> > > - * function will return control to the caller.
> > > + * Note that if we determine that there's still more data to send or we are in
> > > + * the physical replication more, this function will return control to the
> > > + * caller.
> > >
> > > I think in this comment you meant to say
> > >
> > > 1. "or we are in physical replication mode and all WALs are not yet replicated"
> > > 2. Typo /replication more/replication mode
> >
> > Firstly I considered 2, but I thought 1 seemed to be better.
> > PSA the updated patch.
> >
>
> I think even for logical replication we should check whether there is
> any pending WAL (via pq_is_send_pending()) to be sent. Otherwise, what
> is the point to send the done message? Also, the caller of
> WalSndDone() already has that check which is another reason why I
> can't see why you didn't have the same check in function WalSndDone().
>
> BTW, even after fixing this, I think logical replication will behave
> differently when due to some reason (like time-delayed replication)
> send buffer gets full and walsender is not able to send data. I think
> this will be less of an issue with physical replication because there
> is a separate walreceiver process to flush the WAL which doesn't wait
> but the same is not true for logical replication. Do you have any
> thoughts on this matter?
>

In logical replication, it can happen today as well without
time-delayed replication. Basically, say apply worker is waiting to
acquire some lock that is already acquired by some backend then it
will have the same behavior. I have not verified this, so you may want
to check it once.

-- 
With Regards,
Amit Kapila.

  11. RE: Exit walsender before confirming remote flush in logical replication

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2022-12-28T02:47:56Z

    Dear Amit,

> > Firstly I considered 2, but I thought 1 seemed to be better.
> > PSA the updated patch.
> >
> 
> I think even for logical replication we should check whether there is
> any pending WAL (via pq_is_send_pending()) to be sent. Otherwise, what
> is the point to send the done message? Also, the caller of
> WalSndDone() already has that check which is another reason why I
> can't see why you didn't have the same check in function WalSndDone().

I did not have strong opinion around here. Fixed.

> BTW, even after fixing this, I think logical replication will behave
> differently when due to some reason (like time-delayed replication)
> send buffer gets full and walsender is not able to send data. I think
> this will be less of an issue with physical replication because there
> is a separate walreceiver process to flush the WAL which doesn't wait
> but the same is not true for logical replication. Do you have any
> thoughts on this matter?

Yes, it may happen even if this work is done. And your analysis is correct that
the receive buffer is rarely full in physical replication because walreceiver
immediately flush WALs.
I think this is an architectural problem. Maybe we have assumed that the decoded
WALs are consumed in as short time. I do not have good idea, but one approach is
introducing a new process logical-walreceiver.  It will record the decoded WALs to
the persistent storage and workers consume and then remove them. It may have huge
impact for other features and should not be accepted...


Best Regards,
Hayato Kuroda
FUJITSU LIMITED

  12. RE: Exit walsender before confirming remote flush in logical replication

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2022-12-28T02:49:53Z

    Dear Amit,

> In logical replication, it can happen today as well without
> time-delayed replication. Basically, say apply worker is waiting to
> acquire some lock that is already acquired by some backend then it
> will have the same behavior. I have not verified this, so you may want
> to check it once.

Right, I could reproduce the scenario with following steps.

1. Construct pub -> sub logical replication system with streaming = off.
2. Define a table on both nodes.

```
CREATE TABLE tbl (id int PRIMARY KEY);
```

3. Execute concurrent transactions.

Tx-1 (on subscriber)
BEGIN;
INSERT INTO tbl SELECT i FROM generate_series(1, 5000) s(i);

	Tx-2 (on publisher)
	INSERT INTO tbl SELECT i FROM generate_series(1, 5000) s(i);

4. Try to shutdown publisher but it will be failed.

```
$ pg_ctl stop -D publisher
waiting for server to shut down............................................................... failed
pg_ctl: server does not shut down
```

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

  13. Re: Exit walsender before confirming remote flush in logical replication

    Amit Kapila <amit.kapila16@gmail.com> — 2022-12-28T03:56:29Z

    On Wed, Dec 28, 2022 at 8:19 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> > In logical replication, it can happen today as well without
> > time-delayed replication. Basically, say apply worker is waiting to
> > acquire some lock that is already acquired by some backend then it
> > will have the same behavior. I have not verified this, so you may want
> > to check it once.
>
> Right, I could reproduce the scenario with following steps.
>
> 1. Construct pub -> sub logical replication system with streaming = off.
> 2. Define a table on both nodes.
>
> ```
> CREATE TABLE tbl (id int PRIMARY KEY);
> ```
>
> 3. Execute concurrent transactions.
>
> Tx-1 (on subscriber)
> BEGIN;
> INSERT INTO tbl SELECT i FROM generate_series(1, 5000) s(i);
>
>         Tx-2 (on publisher)
>         INSERT INTO tbl SELECT i FROM generate_series(1, 5000) s(i);
>
> 4. Try to shutdown publisher but it will be failed.
>
> ```
> $ pg_ctl stop -D publisher
> waiting for server to shut down............................................................... failed
> pg_ctl: server does not shut down
> ```

Thanks for the verification. BTW, do you think we should document this
either with time-delayed replication or otherwise unless this is
already documented?

Another thing we can investigate here why do we need to ensure that
there is no pending send before shutdown.

-- 
With Regards,
Amit Kapila.

  14. RE: Exit walsender before confirming remote flush in logical replication

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2022-12-28T09:15:41Z

    Dear Amit,

> Thanks for the verification. BTW, do you think we should document this
> either with time-delayed replication or otherwise unless this is
> already documented?

I think this should be documented at "Shutting Down the Server" section in runtime.sgml
or logical-replicaiton.sgml, but I cannot find. It will be included in next version.

> Another thing we can investigate here why do we need to ensure that
> there is no pending send before shutdown.

I have not done yet about it, will continue next year.
It seems that walsenders have been sending all data before shutting down since ea5516,
e0b581 and 754baa. 
There were many threads related with streaming replication, so I could not pin
the specific message that written in the commit message of ea5516.

I have also checked some wiki pages [1][2], but I could not find any design about it.

[1]: https://wiki.postgresql.org/wiki/Streaming_Replication
[2]: https://wiki.postgresql.org/wiki/Synchronous_Replication_9/2010_Proposal

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

  15. Re: Exit walsender before confirming remote flush in logical replication

    Amit Kapila <amit.kapila16@gmail.com> — 2023-01-13T11:11:08Z

    On Wed, Dec 28, 2022 at 8:18 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Amit,
>
> > > Firstly I considered 2, but I thought 1 seemed to be better.
> > > PSA the updated patch.
> > >
> >
> > I think even for logical replication we should check whether there is
> > any pending WAL (via pq_is_send_pending()) to be sent. Otherwise, what
> > is the point to send the done message? Also, the caller of
> > WalSndDone() already has that check which is another reason why I
> > can't see why you didn't have the same check in function WalSndDone().
>
> I did not have strong opinion around here. Fixed.
>
> > BTW, even after fixing this, I think logical replication will behave
> > differently when due to some reason (like time-delayed replication)
> > send buffer gets full and walsender is not able to send data. I think
> > this will be less of an issue with physical replication because there
> > is a separate walreceiver process to flush the WAL which doesn't wait
> > but the same is not true for logical replication. Do you have any
> > thoughts on this matter?
>
> Yes, it may happen even if this work is done. And your analysis is correct that
> the receive buffer is rarely full in physical replication because walreceiver
> immediately flush WALs.
>

Okay, but what happens in the case of physical replication when
synchronous_commit = remote_apply? In that case, won't it ensure that
apply has also happened? If so, then shouldn't the time delay feature
also cause a similar problem for physical replication as well?

-- 
With Regards,
Amit Kapila.

  16. Re: Exit walsender before confirming remote flush in logical replication

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2023-01-16T01:28:23Z

    At Wed, 28 Dec 2022 09:15:41 +0000, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com> wrote in 
> > Another thing we can investigate here why do we need to ensure that
> > there is no pending send before shutdown.
> 
> I have not done yet about it, will continue next year.
> It seems that walsenders have been sending all data before shutting down since ea5516,
> e0b581 and 754baa. 
> There were many threads related with streaming replication, so I could not pin
> the specific message that written in the commit message of ea5516.
> 
> I have also checked some wiki pages [1][2], but I could not find any design about it.
> 
> [1]: https://wiki.postgresql.org/wiki/Streaming_Replication
> [2]: https://wiki.postgresql.org/wiki/Synchronous_Replication_9/2010_Proposal

If I'm grabbing the discussion here correctly, in my memory, it is
because: physical replication needs all records that have written on
primary are written on standby for switchover to succeed. It is
annoying that normal shutdown occasionally leads to switchover
failure. Thus WalSndDone explicitly waits for remote flush/write
regardless of the setting of synchronous_commit. Thus apply delay
doesn't affect shutdown (AFAICS), and that is sufficient since all the
records will be applied at the next startup.

In logical replication apply preceeds write and flush so we have no
indication whether a record is "replicated" to standby by other than
apply LSN. On the other hand, logical recplication doesn't have a
business with switchover so that assurarance is useless. Thus I think
we can (practically) ignore apply_lsn at shutdown. It seems subtly
irregular, though.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  17. Re: Exit walsender before confirming remote flush in logical replication

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2023-01-16T01:31:53Z

    At Fri, 13 Jan 2023 16:41:08 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in 
> Okay, but what happens in the case of physical replication when
> synchronous_commit = remote_apply? In that case, won't it ensure that
> apply has also happened? If so, then shouldn't the time delay feature
> also cause a similar problem for physical replication as well?

As written in another mail, WalSndDone doesn't honor
synchronous_commit. In other words, AFAIS walsender finishes not
waiting remote_apply.  The unapplied recods will be applied at the
next startup.

I didn't confirmed that behavior for myself, though..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  18. RE: Exit walsender before confirming remote flush in logical replication

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2023-01-16T11:08:30Z

    Dear Horiguchi-san, Amit,

> At Fri, 13 Jan 2023 16:41:08 +0530, Amit Kapila <amit.kapila16@gmail.com>
> wrote in
> > Okay, but what happens in the case of physical replication when
> > synchronous_commit = remote_apply? In that case, won't it ensure that
> > apply has also happened? If so, then shouldn't the time delay feature
> > also cause a similar problem for physical replication as well?
> 
> As written in another mail, WalSndDone doesn't honor
> synchronous_commit. In other words, AFAIS walsender finishes not
> waiting remote_apply.  The unapplied recods will be applied at the
> next startup.
> 
> I didn't confirmed that behavior for myself, though..

If Amit wanted to say about the case that sending data is pending in physical
replication, the walsender cannot stop. But this is not related with the
synchronous_commit: it is caused because it must sweep all pending data before
shutting down. We can reproduce the situation with:

1. build streaming replication system
2. kill -STOP $walreceiver
3. insert data to primary server
4. try to stop the primary server

If what you said was not related with pending data, walsender can be stopped even
if the synchronous_commit = remote_apply. As Horiguchi-san said, such a condition
is not written in WalSndDone() [1]. I think the parameter synchronous_commit does
not affect walsender process so well. It just define when backend returns the
result to client.

I could check by following steps:

1. built streaming replication system. PSA the script to follow that.

Primary config.

```
synchronous_commit = 'remote_apply'
synchronous_standby_names = 'secondary'
```

Secondary config.

```
recovery_min_apply_delay = 1d
primary_conninfo = 'user=postgres port=$port_N1 application_name=secondary'
hot_standby = on
```

2. inserted data to primary. This waited the remote apply

psql -U postgres -p $port_primary -c "INSERT INTO tbl SELECT generate_series(1, 5000)"

3. Stopped the primary server from another terminal. It could be done.
The terminal on step2 said like:

```
WARNING:  canceling the wait for synchronous replication and terminating connection due to administrator command
DETAIL:  The transaction has already committed locally, but might not have been replicated to the standby.
server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.
connection to server was lost
```

[1]: https://github.com/postgres/postgres/blob/master/src/backend/replication/walsender.c#L3121

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

  19. RE: Exit walsender before confirming remote flush in logical replication

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2023-01-16T11:09:24Z

    Dear Horiguchi-san,

> If I'm grabbing the discussion here correctly, in my memory, it is
> because: physical replication needs all records that have written on
> primary are written on standby for switchover to succeed. It is
> annoying that normal shutdown occasionally leads to switchover
> failure. Thus WalSndDone explicitly waits for remote flush/write
> regardless of the setting of synchronous_commit.

AFAIK the condition (sentPtr == replicatedPtr) seemed to be introduced for the purpose[1].
You meant to say that the conditon (!pq_is_send_pending()) has same motivation, right?

> Thus apply delay
> doesn't affect shutdown (AFAICS), and that is sufficient since all the
> records will be applied at the next startup.

I was not clear the word "next startup", but I agreed that we can shut down the
walsender in case of recovery_min_apply_delay > 0 and synchronous_commit = remote_apply.
The startup process will be not terminated even if the primary crashes, so I
think the process will apply the transaction sooner or later.

> In logical replication apply preceeds write and flush so we have no
> indication whether a record is "replicated" to standby by other than
> apply LSN. On the other hand, logical recplication doesn't have a
> business with switchover so that assurarance is useless. Thus I think
> we can (practically) ignore apply_lsn at shutdown. It seems subtly
> irregular, though.

Another consideration is that the condition (!pq_is_send_pending()) ensures that
there are no pending messages, including other packets. Currently we force walsenders
to clean up all messages before shutting down, even if it is a keepalive one.
I cannot have any problems caused by this, but I can keep the condition in case of
logical replication.

I updated the patch accordingly. Also, I found that the previous version
did not work well in case of streamed transactions. When a streamed transaction
is committed on publisher but the application is delayed on subscriber, the
process sometimes waits until there is no pending write. This is done in
ProcessPendingWrites(). I added another termination path in the function.

[1]: https://github.com/postgres/postgres/commit/985bd7d49726c9f178558491d31a570d47340459

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

  20. Re: Exit walsender before confirming remote flush in logical replication

    Amit Kapila <amit.kapila16@gmail.com> — 2023-01-17T09:11:00Z

    On Mon, Jan 16, 2023 at 4:39 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> > In logical replication apply preceeds write and flush so we have no
> > indication whether a record is "replicated" to standby by other than
> > apply LSN. On the other hand, logical recplication doesn't have a
> > business with switchover so that assurarance is useless. Thus I think
> > we can (practically) ignore apply_lsn at shutdown. It seems subtly
> > irregular, though.
>
> Another consideration is that the condition (!pq_is_send_pending()) ensures that
> there are no pending messages, including other packets. Currently we force walsenders
> to clean up all messages before shutting down, even if it is a keepalive one.
> I cannot have any problems caused by this, but I can keep the condition in case of
> logical replication.
>

Let me try to summarize the discussion till now. The problem we are
trying to solve here is to allow a shutdown to complete when walsender
is not able to send the entire WAL. Currently, in such cases, the
shutdown fails. As per our current understanding, this can happen when
(a) walreceiver/walapply process is stuck (not able to receive more
WAL) due to locks or some other reason; (b) a long time delay has been
configured to apply the WAL (we don't yet have such a feature for
logical replication but the discussion for same is in progress).

Both reasons mostly apply to logical replication because there is no
separate walreceiver process whose job is to just flush the WAL. In
logical replication, the process that receives the WAL also applies
it. So, while applying it can stuck for a long time waiting for some
heavy-weight lock to be released by some other long-running
transaction by the backend. Similarly, if the user has configured a
large value of time-delayed apply, it can lead to a network buffer
full between walsender and receive/process.

The condition to allow the shutdown to wait for all WAL to be sent has
two parts: (a) it confirms that there is no pending WAL to be sent;
(b) it confirms all the WAL sent has been flushed by the client. As
per our understanding, both these conditions are to allow clean
switchover/failover which seems to be useful only for physical
replication. The logical replication doesn't provide such
functionality.

The proposed patch tries to eliminate condition (b) for logical
replication in the hopes that the same will allow the shutdown to be
complete in most cases. There is no specific reason discussed to not
do (a) for logical replication.

Now, to proceed here we have the following options: (1) Fix (b) as
proposed by the patch and document the risks related to (a); (2) Fix
both (a) and (b); (3) Do nothing and document that users need to
unblock the subscribers to complete the shutdown.

Thoughts?

-- 
With Regards,
Amit Kapila.

  21. RE: Exit walsender before confirming remote flush in logical replication

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2023-01-19T08:37:47Z

    Dear Amit, hackers,

> Let me try to summarize the discussion till now. The problem we are
> trying to solve here is to allow a shutdown to complete when walsender
> is not able to send the entire WAL. Currently, in such cases, the
> shutdown fails. As per our current understanding, this can happen when
> (a) walreceiver/walapply process is stuck (not able to receive more
> WAL) due to locks or some other reason; (b) a long time delay has been
> configured to apply the WAL (we don't yet have such a feature for
> logical replication but the discussion for same is in progress).

Thanks for summarizing.
While analyzing stuck, I noticed that there are two types of shutdown failures.
They could be characterized by the back trace. They are shown at the bottom.

Type i)
The walsender executes WalSndDone(), but cannot satisfy the condition.
It means that all WALs have been sent to the subscriber but have not flushed;
sentPtr is not the same as replicatedPtr. This stuck can happen when the delayed
transaction is small or streamed.

Type ii)
The walsender cannot execute WalSndDone(), stacks at ProcessPendingWrites().
It means that when the send buffer becomes full while replicating a transaction;
pq_is_send_pending() returns true and the walsender cannot break the loop.
This stuck can happen when the delayed transaction is large, but it is not a streamed one.

If we choose modification (1), we can only fix type (i) because pending WALs cause
the failure. IIUC if we want to shut down walsender processes even if (ii), we must
choose (2) and additional fixes are needed.

Based on the above, I prefer modification (2) because it can rescue more cases. Thoughts?
PSA the patch for it. It is almost the same as the previous version, but the comments are updated.


Appendinx:

The backtrace for type i)

```
#0  WalSndDone (send_data=0x87f825 <XLogSendLogical>) at ../../PostgreSQL-Source-Dev/src/backend/replication/walsender.c:3111
#1  0x000000000087ed1d in WalSndLoop (send_data=0x87f825 <XLogSendLogical>) at ../../PostgreSQL-Source-Dev/src/backend/replication/walsender.c:2525
#2  0x000000000087d40a in StartLogicalReplication (cmd=0x1f49030) at ../../PostgreSQL-Source-Dev/src/backend/replication/walsender.c:1320
#3  0x000000000087df29 in exec_replication_command (
    cmd_string=0x1f15498 "START_REPLICATION SLOT \"sub\" LOGICAL 0/0 (proto_version '4', streaming 'on', origin 'none', publication_names '\"pub\"')")
    at ../../PostgreSQL-Source-Dev/src/backend/replication/walsender.c:1830
#4  0x000000000091b032 in PostgresMain (dbname=0x1f4c938 "postgres", username=0x1f4c918 "postgres")
    at ../../PostgreSQL-Source-Dev/src/backend/tcop/postgres.c:4561
#5  0x000000000085390b in BackendRun (port=0x1f3d0b0) at ../../PostgreSQL-Source-Dev/src/backend/postmaster/postmaster.c:4437
#6  0x000000000085322c in BackendStartup (port=0x1f3d0b0) at ../../PostgreSQL-Source-Dev/src/backend/postmaster/postmaster.c:4165
#7  0x000000000084f7a2 in ServerLoop () at ../../PostgreSQL-Source-Dev/src/backend/postmaster/postmaster.c:1762
#8  0x000000000084f0a2 in PostmasterMain (argc=3, argv=0x1f0ff30) at ../../PostgreSQL-Source-Dev/src/backend/postmaster/postmaster.c:1452
#9  0x000000000074a4d6 in main (argc=3, argv=0x1f0ff30) at ../../PostgreSQL-Source-Dev/src/backend/main/main.c:200
```

The backtrace for type ii)

```
#0  ProcessPendingWrites () at ../../PostgreSQL-Source-Dev/src/backend/replication/walsender.c:1438
#1  0x000000000087d635 in WalSndWriteData (ctx=0x1429ce8, lsn=22406440, xid=731, last_write=true)
    at ../../PostgreSQL-Source-Dev/src/backend/replication/walsender.c:1405
#2  0x0000000000888420 in OutputPluginWrite (ctx=0x1429ce8, last_write=true) at ../../PostgreSQL-Source-Dev/src/backend/replication/logical/logical.c:669
#3  0x00007f022dfe43a7 in pgoutput_change (ctx=0x1429ce8, txn=0x1457d40, relation=0x7f0245075268, change=0x1460ef8)
    at ../../PostgreSQL-Source-Dev/src/backend/replication/pgoutput/pgoutput.c:1491
#4  0x0000000000889125 in change_cb_wrapper (cache=0x142bcf8, txn=0x1457d40, relation=0x7f0245075268, change=0x1460ef8)
    at ../../PostgreSQL-Source-Dev/src/backend/replication/logical/logical.c:1077
#5  0x000000000089507c in ReorderBufferApplyChange (rb=0x142bcf8, txn=0x1457d40, relation=0x7f0245075268, change=0x1460ef8, streaming=false)
    at ../../PostgreSQL-Source-Dev/src/backend/replication/logical/reorderbuffer.c:1969
#6  0x0000000000895866 in ReorderBufferProcessTXN (rb=0x142bcf8, txn=0x1457d40, commit_lsn=23060624, snapshot_now=0x1440150, command_id=0, streaming=false)
    at ../../PostgreSQL-Source-Dev/src/backend/replication/logical/reorderbuffer.c:2245
#7  0x0000000000896348 in ReorderBufferReplay (txn=0x1457d40, rb=0x142bcf8, xid=731, commit_lsn=23060624, end_lsn=23060672, commit_time=727353664342177, 
    origin_id=0, origin_lsn=0) at ../../PostgreSQL-Source-Dev/src/backend/replication/logical/reorderbuffer.c:2675
#8  0x00000000008963d0 in ReorderBufferCommit (rb=0x142bcf8, xid=731, commit_lsn=23060624, end_lsn=23060672, commit_time=727353664342177, origin_id=0, 
    origin_lsn=0) at ../../PostgreSQL-Source-Dev/src/backend/replication/logical/reorderbuffer.c:2699
#9  0x00000000008842c7 in DecodeCommit (ctx=0x1429ce8, buf=0x7ffcf03731a0, parsed=0x7ffcf0372fa0, xid=731, two_phase=false)
    at ../../PostgreSQL-Source-Dev/src/backend/replication/logical/decode.c:682
#10 0x0000000000883667 in xact_decode (ctx=0x1429ce8, buf=0x7ffcf03731a0) at ../../PostgreSQL-Source-Dev/src/backend/replication/logical/decode.c:216
#11 0x000000000088338b in LogicalDecodingProcessRecord (ctx=0x1429ce8, record=0x142a080)
    at ../../PostgreSQL-Source-Dev/src/backend/replication/logical/decode.c:119
#12 0x000000000087f8c7 in XLogSendLogical () at ../../PostgreSQL-Source-Dev/src/backend/replication/walsender.c:3060
#13 0x000000000087ec5a in WalSndLoop (send_data=0x87f825 <XLogSendLogical>) at ../../PostgreSQL-Source-Dev/src/backend/replication/walsender.c:2490
...
```


Best Regards,
Hayato Kuroda
FUJITSU LIMITED

  22. Re: Exit walsender before confirming remote flush in logical replication

    Amit Kapila <amit.kapila16@gmail.com> — 2023-01-20T10:45:27Z

    On Tue, Jan 17, 2023 at 2:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> Let me try to summarize the discussion till now. The problem we are
> trying to solve here is to allow a shutdown to complete when walsender
> is not able to send the entire WAL. Currently, in such cases, the
> shutdown fails. As per our current understanding, this can happen when
> (a) walreceiver/walapply process is stuck (not able to receive more
> WAL) due to locks or some other reason; (b) a long time delay has been
> configured to apply the WAL (we don't yet have such a feature for
> logical replication but the discussion for same is in progress).
>
> Both reasons mostly apply to logical replication because there is no
> separate walreceiver process whose job is to just flush the WAL. In
> logical replication, the process that receives the WAL also applies
> it. So, while applying it can stuck for a long time waiting for some
> heavy-weight lock to be released by some other long-running
> transaction by the backend.
>

While checking the commits and email discussions in this area, I came
across the email [1] from Michael where something similar seems to
have been discussed. Basically, whether the early shutdown of
walsender can prevent a switchover between publisher and subscriber
but that part was never clearly answered in that email chain. It might
be worth reading the entire discussion [2]. That discussion finally
lead to the following commit:

commit c6c333436491a292d56044ed6e167e2bdee015a2
Author: Andres Freund <andres@anarazel.de>
Date:   Mon Jun 5 18:53:41 2017 -0700

    Prevent possibility of panics during shutdown checkpoint.

    When the checkpointer writes the shutdown checkpoint, it checks
    afterwards whether any WAL has been written since it started and
    throws a PANIC if so.  At that point, only walsenders are still
    active, so one might think this could not happen, but walsenders can
    also generate WAL, for instance in BASE_BACKUP and logical decoding
    related commands (e.g. via hint bits).  So they can trigger this panic
    if such a command is run while the shutdown checkpoint is being
    written.

    To fix this, divide the walsender shutdown into two phases.  First,
    checkpointer, itself triggered by postmaster, sends a
    PROCSIG_WALSND_INIT_STOPPING signal to all walsenders.  If the backend
    is idle or runs an SQL query this causes the backend to shutdown, if
    logical replication is in progress all existing WAL records are
    processed followed by a shutdown.
...
...

Here, as mentioned in the commit, we are trying to ensure that before
checkpoint writes its shutdown WAL record, we ensure that "if logical
replication is in progress all existing WAL records are processed
followed by a shutdown.". I think even before this commit, we try to
send the entire WAL before shutdown but not completely sure. There was
no discussion on what happens if the logical walreceiver/walapply
process is waiting on some heavy-weight lock and the network socket
buffer is full due to which walsender is not able to process its WAL.
Is it okay for shutdown to fail in such a case as it is happening now,
or shall we somehow detect that and shut down the walsender, or we
just allow logical walsender to always exit immediately as soon as the
shutdown signal came?

Note: I have added some of the people involved in the previous
thread's [2] discussion in the hope that they can share their
thoughts.

[1] - https://www.postgresql.org/message-id/CAB7nPqR3icaA%3DqMv_FuU8YVYH3KUrNMnq_OmCfkzxCHC4fox8w%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/CAHGQGwEsttg9P9LOOavoc9d6VB1zVmYgfBk%3DLjsk-UL9cEf-eA%40mail.gmail.com

-- 
With Regards,
Amit Kapila.

  23. Re: Exit walsender before confirming remote flush in logical replication

    Dilip Kumar <dilipbalaut@gmail.com> — 2023-01-23T05:39:14Z

    On Fri, Jan 20, 2023 at 4:15 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Jan 17, 2023 at 2:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > Let me try to summarize the discussion till now. The problem we are
> > trying to solve here is to allow a shutdown to complete when walsender
> > is not able to send the entire WAL. Currently, in such cases, the
> > shutdown fails. As per our current understanding, this can happen when
> > (a) walreceiver/walapply process is stuck (not able to receive more
> > WAL) due to locks or some other reason; (b) a long time delay has been
> > configured to apply the WAL (we don't yet have such a feature for
> > logical replication but the discussion for same is in progress).
> >
> > Both reasons mostly apply to logical replication because there is no
> > separate walreceiver process whose job is to just flush the WAL. In
> > logical replication, the process that receives the WAL also applies
> > it. So, while applying it can stuck for a long time waiting for some
> > heavy-weight lock to be released by some other long-running
> > transaction by the backend.
> >
>
> While checking the commits and email discussions in this area, I came
> across the email [1] from Michael where something similar seems to
> have been discussed. Basically, whether the early shutdown of
> walsender can prevent a switchover between publisher and subscriber
> but that part was never clearly answered in that email chain. It might
> be worth reading the entire discussion [2]. That discussion finally
> lead to the following commit:


Right, in the thread the question is raised about whether it makes
sense for logical replication to send all WALs but there is no
conclusion on that.  But I think this patch is mainly about resolving
the PANIC due to extra WAL getting generated by walsender during
checkpoint processing and that's the reason the behavior of sending
all the WAL is maintained but only the extra WAL generation stopped
(before shutdown checkpoint can proceed) using this new state

>
> commit c6c333436491a292d56044ed6e167e2bdee015a2
> Author: Andres Freund <andres@anarazel.de>
> Date:   Mon Jun 5 18:53:41 2017 -0700
>
>     Prevent possibility of panics during shutdown checkpoint.
>
>     When the checkpointer writes the shutdown checkpoint, it checks
>     afterwards whether any WAL has been written since it started and
>     throws a PANIC if so.  At that point, only walsenders are still
>     active, so one might think this could not happen, but walsenders can
>     also generate WAL, for instance in BASE_BACKUP and logical decoding
>     related commands (e.g. via hint bits).  So they can trigger this panic
>     if such a command is run while the shutdown checkpoint is being
>     written.
>
>     To fix this, divide the walsender shutdown into two phases.  First,
>     checkpointer, itself triggered by postmaster, sends a
>     PROCSIG_WALSND_INIT_STOPPING signal to all walsenders.  If the backend
>     is idle or runs an SQL query this causes the backend to shutdown, if
>     logical replication is in progress all existing WAL records are
>     processed followed by a shutdown.
> ...
> ...
>
> Here, as mentioned in the commit, we are trying to ensure that before
> checkpoint writes its shutdown WAL record, we ensure that "if logical
> replication is in progress all existing WAL records are processed
> followed by a shutdown.". I think even before this commit, we try to
> send the entire WAL before shutdown but not completely sure.


Yes, I think that there is no change in that behavior.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

  24. RE: Exit walsender before confirming remote flush in logical replication

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2023-01-27T04:11:53Z

    Dear Dilip, hackers,

Thanks for giving your opinion. I analyzed the relation with the given commit,
and I thought I could keep my patch. How do you think?

# Abstract

* Some modifications should be needed.
* We cannot rollback the shutdown if walsenders are stuck
* We don't have a good way to detect stuck

# Discussion

Compared to physical replication, it is likely to happen that logical replication is stuck.
I think the risk should be avoided as much as possible by fixing codes.
Then, if it leads another failure, we can document the caution to users.

While shutting down the server, checkpointer sends SIGUSR1 signal to wansenders.
It is done after being exited other processes, so we cannot raise ERROR and rollback
the operation if checkpointer recognize the process stuck at that time.

We don't have any features that postmaster can check  whether this node is
publisher or not. So if we want to add the mechanism that can check the health
of walsenders before shutting down, we must do that at the top of
process_pm_shutdown_request() even if we are not in logical replication.
I think it affects the basis of postgres largely, and in the first place,
PostgreSQL does not have a mechanism to check the health of process.

Therefore, I want to adopt the approach that walsender itself exits immediately when they get signals.

## About patch - Were fixes correct?

In ProcessPendingWrites(), my patch, wansender calls WalSndDone() when it gets
SIGUSR1 signal. I think this should be. From the patch [1]:

```
@@ -1450,6 +1450,10 @@ ProcessPendingWrites(void)
                /* Try to flush pending output to the client */
                if (pq_flush_if_writable() != 0)
                        WalSndShutdown();
+
+               /* If we got shut down requested, try to exit the process */
+               if (got_STOPPING)
+                       WalSndDone(XLogSendLogical);
        }
 
        /* reactivate latch so WalSndLoop knows to continue */
```


Per my analysis, in case of logical replication, walsenders exit with following
steps. Note that logical walsender does not receive SIGUSR2 signal, set flag by
themselves instead:

1. postmaster sends shutdown requests to checkpointer
2. checkpointer sends SIGUSR1 to walsenders and wait
3. when walsenders accept SIGUSR1, they turn got_SIGUSR1 on.
4. walsenders consume all WALs. @XLogSendLogical
5. walsenders turn got_SIGUSR2 on by themselves @XLogSendLogical
6. walsenders recognize the flag is on, so call WalSndDone() @ WalSndLoop
7. proc_exit(0)
8. checkpoitner writes shutdown record
...

Type (i) stuck, I reported in -hackers[1], means that processes stop at step 6
and Type (ii) stuck means that processes stop at 4. In step4, got_SIGUSR2 is never set to on, so
we must use got_STOPPING flag.

[1]: https://www.postgresql.org/message-id/TYCPR01MB58701A47F35FED0A2B399662F5C49@TYCPR01MB5870.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

  25. Re: Exit walsender before confirming remote flush in logical replication

    Masahiko Sawada <sawada.mshk@gmail.com> — 2023-02-01T08:38:36Z

    On Fri, Jan 20, 2023 at 7:45 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Jan 17, 2023 at 2:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > Let me try to summarize the discussion till now. The problem we are
> > trying to solve here is to allow a shutdown to complete when walsender
> > is not able to send the entire WAL. Currently, in such cases, the
> > shutdown fails. As per our current understanding, this can happen when
> > (a) walreceiver/walapply process is stuck (not able to receive more
> > WAL) due to locks or some other reason; (b) a long time delay has been
> > configured to apply the WAL (we don't yet have such a feature for
> > logical replication but the discussion for same is in progress).
> >
> > Both reasons mostly apply to logical replication because there is no
> > separate walreceiver process whose job is to just flush the WAL. In
> > logical replication, the process that receives the WAL also applies
> > it. So, while applying it can stuck for a long time waiting for some
> > heavy-weight lock to be released by some other long-running
> > transaction by the backend.
> >
>
> While checking the commits and email discussions in this area, I came
> across the email [1] from Michael where something similar seems to
> have been discussed. Basically, whether the early shutdown of
> walsender can prevent a switchover between publisher and subscriber
> but that part was never clearly answered in that email chain. It might
> be worth reading the entire discussion [2]. That discussion finally
> lead to the following commit:
>
> commit c6c333436491a292d56044ed6e167e2bdee015a2
> Author: Andres Freund <andres@anarazel.de>
> Date:   Mon Jun 5 18:53:41 2017 -0700
>
>     Prevent possibility of panics during shutdown checkpoint.
>
>     When the checkpointer writes the shutdown checkpoint, it checks
>     afterwards whether any WAL has been written since it started and
>     throws a PANIC if so.  At that point, only walsenders are still
>     active, so one might think this could not happen, but walsenders can
>     also generate WAL, for instance in BASE_BACKUP and logical decoding
>     related commands (e.g. via hint bits).  So they can trigger this panic
>     if such a command is run while the shutdown checkpoint is being
>     written.
>
>     To fix this, divide the walsender shutdown into two phases.  First,
>     checkpointer, itself triggered by postmaster, sends a
>     PROCSIG_WALSND_INIT_STOPPING signal to all walsenders.  If the backend
>     is idle or runs an SQL query this causes the backend to shutdown, if
>     logical replication is in progress all existing WAL records are
>     processed followed by a shutdown.
> ...
> ...
>
> Here, as mentioned in the commit, we are trying to ensure that before
> checkpoint writes its shutdown WAL record, we ensure that "if logical
> replication is in progress all existing WAL records are processed
> followed by a shutdown.". I think even before this commit, we try to
> send the entire WAL before shutdown but not completely sure. There was
> no discussion on what happens if the logical walreceiver/walapply
> process is waiting on some heavy-weight lock and the network socket
> buffer is full due to which walsender is not able to process its WAL.
> Is it okay for shutdown to fail in such a case as it is happening now,
> or shall we somehow detect that and shut down the walsender, or we
> just allow logical walsender to always exit immediately as soon as the
> shutdown signal came?

+1 to eliminate condition (b) for logical replication.

Regarding (a), as Amit mentioned before[1], I think we should check if
pq_is_send_pending() is false. Otherwise, we will end up terminating
the WAL stream without the done message. Which will lead to an error
message "ERROR:  could not receive data from WAL stream: server closed
the connection unexpectedly" on the subscriber even at a clean
shutdown. In a case where pq_is_send_pending() doesn't become false
for a long time, (e.g., the network socket buffer got full due to the
apply worker waiting on a lock), I think users should unblock it by
themselves. Or it might be practically better to shutdown the
subscriber first in the logical replication case, unlike the physical
replication case. I've not studied the time-delayed logical
replication patch yet, though.

Regards,

[1] https://www.postgresql.org/message-id/CAA4eK1%2BpD654%2BXnrPugYueh7Oh22EBGTr6dA_fS0%2BgPiHayG9A%40mail.gmail.com

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

  26. Re: Exit walsender before confirming remote flush in logical replication

    Amit Kapila <amit.kapila16@gmail.com> — 2023-02-01T09:28:14Z

    On Wed, Feb 1, 2023 at 2:09 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Fri, Jan 20, 2023 at 7:45 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Jan 17, 2023 at 2:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > Let me try to summarize the discussion till now. The problem we are
> > > trying to solve here is to allow a shutdown to complete when walsender
> > > is not able to send the entire WAL. Currently, in such cases, the
> > > shutdown fails. As per our current understanding, this can happen when
> > > (a) walreceiver/walapply process is stuck (not able to receive more
> > > WAL) due to locks or some other reason; (b) a long time delay has been
> > > configured to apply the WAL (we don't yet have such a feature for
> > > logical replication but the discussion for same is in progress).
> > >
> > > Both reasons mostly apply to logical replication because there is no
> > > separate walreceiver process whose job is to just flush the WAL. In
> > > logical replication, the process that receives the WAL also applies
> > > it. So, while applying it can stuck for a long time waiting for some
> > > heavy-weight lock to be released by some other long-running
> > > transaction by the backend.
> > >
...
...
>
> +1 to eliminate condition (b) for logical replication.
>
> Regarding (a), as Amit mentioned before[1], I think we should check if
> pq_is_send_pending() is false.
>

Sorry, but your suggestion is not completely clear to me. Do you mean
to say that for logical replication, we shouldn't wait for all the WAL
to be successfully replicated but we should ensure to inform the
subscriber that XLOG streaming is done (by ensuring
pq_is_send_pending() is false and by calling EndCommand, pq_flush())?

> Otherwise, we will end up terminating
> the WAL stream without the done message. Which will lead to an error
> message "ERROR:  could not receive data from WAL stream: server closed
> the connection unexpectedly" on the subscriber even at a clean
> shutdown.
>

But will that be a problem? As per docs of shutdown [1] ( “Smart” mode
disallows new connections, then waits for all existing clients to
disconnect. If the server is in hot standby, recovery and streaming
replication will be terminated once all clients have disconnected.),
there is no such guarantee. I see that it is required for the
switchover in physical replication to ensure that all the WAL is sent
and replicated but we don't need that for logical replication.

> In a case where pq_is_send_pending() doesn't become false
> for a long time, (e.g., the network socket buffer got full due to the
> apply worker waiting on a lock), I think users should unblock it by
> themselves. Or it might be practically better to shutdown the
> subscriber first in the logical replication case, unlike the physical
> replication case.
>

Yeah, will users like such a dependency? And what will they gain by doing so?


[1] - https://www.postgresql.org/docs/devel/app-pg-ctl.html

-- 
With Regards,
Amit Kapila.

  27. Re: Exit walsender before confirming remote flush in logical replication

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2023-02-02T04:34:23Z

    At Wed, 1 Feb 2023 14:58:14 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in 
> On Wed, Feb 1, 2023 at 2:09 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > Otherwise, we will end up terminating
> > the WAL stream without the done message. Which will lead to an error
> > message "ERROR:  could not receive data from WAL stream: server closed
> > the connection unexpectedly" on the subscriber even at a clean
> > shutdown.
> >
> 
> But will that be a problem? As per docs of shutdown [1] ( “Smart” mode
> disallows new connections, then waits for all existing clients to
> disconnect. If the server is in hot standby, recovery and streaming
> replication will be terminated once all clients have disconnected.),
> there is no such guarantee. I see that it is required for the
> switchover in physical replication to ensure that all the WAL is sent
> and replicated but we don't need that for logical replication.

+1

Since publisher is not aware of apply-delay (by this patch), as a
matter of fact publisher seems gone before sending EOS in that
case. The error message is correctly describing that situation.

> > In a case where pq_is_send_pending() doesn't become false
> > for a long time, (e.g., the network socket buffer got full due to the
> > apply worker waiting on a lock), I think users should unblock it by
> > themselves. Or it might be practically better to shutdown the
> > subscriber first in the logical replication case, unlike the physical
> > replication case.
> >
> 
> Yeah, will users like such a dependency? And what will they gain by doing so?

If PostgreSQL required such kind of special care about shutdown at
facing a trouble to keep replication consistency, that won't be
acceptable. The current time-delayed logical replication can be seen
as a kind of intentional continuous large network lag in this
aspect. And I think the consistency is guaranteed even in such cases.

On the other hand I don't think the almost all people care about the
exact progress when facing such troubles, as far as replication
consistently is maintained. IMHO that is also true for the
logical-delayed-replication case.

> [1] - https://www.postgresql.org/docs/devel/app-pg-ctl.html

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  28. Re: Exit walsender before confirming remote flush in logical replication

    Masahiko Sawada <sawada.mshk@gmail.com> — 2023-02-02T05:17:55Z

    On Wed, Feb 1, 2023 at 6:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Feb 1, 2023 at 2:09 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Fri, Jan 20, 2023 at 7:45 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Tue, Jan 17, 2023 at 2:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > Let me try to summarize the discussion till now. The problem we are
> > > > trying to solve here is to allow a shutdown to complete when walsender
> > > > is not able to send the entire WAL. Currently, in such cases, the
> > > > shutdown fails. As per our current understanding, this can happen when
> > > > (a) walreceiver/walapply process is stuck (not able to receive more
> > > > WAL) due to locks or some other reason; (b) a long time delay has been
> > > > configured to apply the WAL (we don't yet have such a feature for
> > > > logical replication but the discussion for same is in progress).
> > > >
> > > > Both reasons mostly apply to logical replication because there is no
> > > > separate walreceiver process whose job is to just flush the WAL. In
> > > > logical replication, the process that receives the WAL also applies
> > > > it. So, while applying it can stuck for a long time waiting for some
> > > > heavy-weight lock to be released by some other long-running
> > > > transaction by the backend.
> > > >
> ...
> ...
> >
> > +1 to eliminate condition (b) for logical replication.
> >
> > Regarding (a), as Amit mentioned before[1], I think we should check if
> > pq_is_send_pending() is false.
> >
>
> Sorry, but your suggestion is not completely clear to me. Do you mean
> to say that for logical replication, we shouldn't wait for all the WAL
> to be successfully replicated but we should ensure to inform the
> subscriber that XLOG streaming is done (by ensuring
> pq_is_send_pending() is false and by calling EndCommand, pq_flush())?

Yes.

>
> > Otherwise, we will end up terminating
> > the WAL stream without the done message. Which will lead to an error
> > message "ERROR:  could not receive data from WAL stream: server closed
> > the connection unexpectedly" on the subscriber even at a clean
> > shutdown.
> >
>
> But will that be a problem? As per docs of shutdown [1] ( “Smart” mode
> disallows new connections, then waits for all existing clients to
> disconnect. If the server is in hot standby, recovery and streaming
> replication will be terminated once all clients have disconnected.),
> there is no such guarantee.

In smart shutdown case, the walsender doesn't exit until it can flush
the done message, no?

> I see that it is required for the
> switchover in physical replication to ensure that all the WAL is sent
> and replicated but we don't need that for logical replication.

It won't be a problem in practice in terms of logical replication. But
I'm concerned that this error could confuse users. Is there any case
where the client gets such an error at the smart shutdown?

>
> > In a case where pq_is_send_pending() doesn't become false
> > for a long time, (e.g., the network socket buffer got full due to the
> > apply worker waiting on a lock), I think users should unblock it by
> > themselves. Or it might be practically better to shutdown the
> > subscriber first in the logical replication case, unlike the physical
> > replication case.
> >
>
> Yeah, will users like such a dependency? And what will they gain by doing so?

IIUC there is no difference between smart shutdown and fast shutdown
in logical replication walsender, but reading the doc[1], it seems to
me that in the smart shutdown mode, the server stops existing sessions
normally. For example, If the client is psql that gets stuck for some
reason and the network buffer gets full, the smart shutdown waits for
a backend process to send all results to the client. I think the
logical replication walsender should follow this behavior for
consistency. One idea is to distinguish smart shutdown and fast
shutdown also in logical replication walsender so that we disconnect
even without the done message in fast shutdown mode, but I'm not sure
it's worthwhile.

Regards,

[1] https://www.postgresql.org/docs/devel/server-shutdown.html

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

  29. Re: Exit walsender before confirming remote flush in logical replication

    Amit Kapila <amit.kapila16@gmail.com> — 2023-02-02T05:51:54Z

    On Thu, Feb 2, 2023 at 10:48 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Feb 1, 2023 at 6:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> >
> > > In a case where pq_is_send_pending() doesn't become false
> > > for a long time, (e.g., the network socket buffer got full due to the
> > > apply worker waiting on a lock), I think users should unblock it by
> > > themselves. Or it might be practically better to shutdown the
> > > subscriber first in the logical replication case, unlike the physical
> > > replication case.
> > >
> >
> > Yeah, will users like such a dependency? And what will they gain by doing so?
>
> IIUC there is no difference between smart shutdown and fast shutdown
> in logical replication walsender, but reading the doc[1], it seems to
> me that in the smart shutdown mode, the server stops existing sessions
> normally. For example, If the client is psql that gets stuck for some
> reason and the network buffer gets full, the smart shutdown waits for
> a backend process to send all results to the client. I think the
> logical replication walsender should follow this behavior for
> consistency. One idea is to distinguish smart shutdown and fast
> shutdown also in logical replication walsender so that we disconnect
> even without the done message in fast shutdown mode, but I'm not sure
> it's worthwhile.
>

The main problem we want to solve here is to avoid shutdown failing in
case walreceiver/applyworker is busy waiting for some lock or for some
other reason as shown in the email [1]. I haven't tested it but if
such a problem doesn't exist in smart shutdown mode then probably we
can allow walsender to wait till all the data is sent. We can once
investigate what it takes to introduce shutdown mode knowledge for
logical walsender. OTOH, the docs for smart shutdown says "If the
server is in hot standby, recovery and streaming replication will be
terminated once all clients have disconnected." which to me indicates
that it is okay to terminate logical replication connections even in
smart mode.

[1] - https://www.postgresql.org/message-id/TYAPR01MB58669CB06F6657ABCEFE6555F5F29%40TYAPR01MB5866.jpnprd01.prod.outlook.com

-- 
With Regards,
Amit Kapila.

  30. Re: Exit walsender before confirming remote flush in logical replication

    Amit Kapila <amit.kapila16@gmail.com> — 2023-02-02T05:53:30Z

    On Thu, Feb 2, 2023 at 10:04 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Wed, 1 Feb 2023 14:58:14 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in
> > On Wed, Feb 1, 2023 at 2:09 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > Otherwise, we will end up terminating
> > > the WAL stream without the done message. Which will lead to an error
> > > message "ERROR:  could not receive data from WAL stream: server closed
> > > the connection unexpectedly" on the subscriber even at a clean
> > > shutdown.
> > >
> >
> > But will that be a problem? As per docs of shutdown [1] ( “Smart” mode
> > disallows new connections, then waits for all existing clients to
> > disconnect. If the server is in hot standby, recovery and streaming
> > replication will be terminated once all clients have disconnected.),
> > there is no such guarantee. I see that it is required for the
> > switchover in physical replication to ensure that all the WAL is sent
> > and replicated but we don't need that for logical replication.
>
> +1
>
> Since publisher is not aware of apply-delay (by this patch), as a
> matter of fact publisher seems gone before sending EOS in that
> case. The error message is correctly describing that situation.
>

This can happen even without apply-delay patch. For example, when
apply process is waiting on some lock.

-- 
With Regards,
Amit Kapila.

  31. RE: Exit walsender before confirming remote flush in logical replication

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2023-02-03T12:08:48Z

    Dear Amit, Sawada-san,

> > IIUC there is no difference between smart shutdown and fast shutdown
> > in logical replication walsender, but reading the doc[1], it seems to
> > me that in the smart shutdown mode, the server stops existing sessions
> > normally. For example, If the client is psql that gets stuck for some
> > reason and the network buffer gets full, the smart shutdown waits for
> > a backend process to send all results to the client. I think the
> > logical replication walsender should follow this behavior for
> > consistency. One idea is to distinguish smart shutdown and fast
> > shutdown also in logical replication walsender so that we disconnect
> > even without the done message in fast shutdown mode, but I'm not sure
> > it's worthwhile.
> >
> 
> The main problem we want to solve here is to avoid shutdown failing in
> case walreceiver/applyworker is busy waiting for some lock or for some
> other reason as shown in the email [1]. I haven't tested it but if
> such a problem doesn't exist in smart shutdown mode then probably we
> can allow walsender to wait till all the data is sent.

Based on the idea, I made a PoC patch to introduce the smart shutdown to walsenders.
PSA 0002 patch. 0001 is not changed from v5.
When logical walsenders got shutdown request but their send buffer is full due to
the delay, they will:

* wait to complete to send data to subscriber if we are in smart shutdown mode
* exit immediately if we are in fast shutdown mode

Note that in both case, walsender does not wait the remote flush of WALs.

For implementing that, I added new attribute to WalSndCtlData that indicates the
shutdown status. Basically it is zero, but it will be changed by postmaster when
it gets request.


Best Regards,
Hayato Kuroda
FUJITSU LIMITED

  32. Re: Exit walsender before confirming remote flush in logical replication

    Amit Kapila <amit.kapila16@gmail.com> — 2023-02-04T11:04:10Z

    On Fri, Feb 3, 2023 at 5:38 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Amit, Sawada-san,
>
> > > IIUC there is no difference between smart shutdown and fast shutdown
> > > in logical replication walsender, but reading the doc[1], it seems to
> > > me that in the smart shutdown mode, the server stops existing sessions
> > > normally. For example, If the client is psql that gets stuck for some
> > > reason and the network buffer gets full, the smart shutdown waits for
> > > a backend process to send all results to the client. I think the
> > > logical replication walsender should follow this behavior for
> > > consistency. One idea is to distinguish smart shutdown and fast
> > > shutdown also in logical replication walsender so that we disconnect
> > > even without the done message in fast shutdown mode, but I'm not sure
> > > it's worthwhile.
> > >
> >
> > The main problem we want to solve here is to avoid shutdown failing in
> > case walreceiver/applyworker is busy waiting for some lock or for some
> > other reason as shown in the email [1].
> >

For this problem isn't using -t (timeout) avoid it? So, if there is a
pending WAL, users can always use -t option to allow the shutdown to
complete. Now, I agree that it is not very clear how much time to
specify but a user has some option to allow the shutdown to complete.
I am not telling that teaching walsenders about shutdown modes is
completely a bad idea but it doesn't seem necessary to allow shutdowns
to complete.

-- 
With Regards,
Amit Kapila.

  33. Re: Exit walsender before confirming remote flush in logical replication

    Andres Freund <andres@anarazel.de> — 2023-02-04T13:01:55Z

    Hi,

On 2023-02-02 11:21:54 +0530, Amit Kapila wrote:
> The main problem we want to solve here is to avoid shutdown failing in
> case walreceiver/applyworker is busy waiting for some lock or for some
> other reason as shown in the email [1].

Isn't handling this part of the job of wal_sender_timeout?


I don't at all agree that it's ok to just stop replicating changes
because we're blocked on network IO. The patch justifies this with:

> Currently, at shutdown, walsender processes wait to send all pending data and
> ensure the all data is flushed in remote node. This mechanism was added by
> 985bd7 for supporting clean switch over, but such use-case cannot be supported
> for logical replication. This commit remove the blocking in the case.

and at the start of the thread with:

> In case of logical replication, however, we cannot support the use-case that
> switches the role publisher <-> subscriber. Suppose same case as above, additional
> transactions are committed while doing step2. To catch up such changes subscriber
> must receive WALs related with trans, but it cannot be done because subscriber
> cannot request WALs from the specific position. In the case, we must truncate all
> data in new subscriber once, and then create new subscription with copy_data
> = true.

But that seems a too narrow view to me. Imagine you want to decomission
the current primary, and instead start to use the logical standby as the
primary. For that you'd obviously want to replicate the last few
changes. But with the proposed change, that'd be hard to ever achieve.

Note that even disallowing any writes on the logical primary would make
it hard to be sure that everything is replicated, because autovacuum,
bgwriter, checkpointer all can continue to write WAL. Without being able
to check that the last LSN has indeed been sent out, how do you know
that you didn't miss something?

Greetings,

Andres Freund

  34. Re: Exit walsender before confirming remote flush in logical replication

    Amit Kapila <amit.kapila16@gmail.com> — 2023-02-06T04:29:19Z

    On Sat, Feb 4, 2023 at 6:31 PM Andres Freund <andres@anarazel.de> wrote:
>
> On 2023-02-02 11:21:54 +0530, Amit Kapila wrote:
> > The main problem we want to solve here is to avoid shutdown failing in
> > case walreceiver/applyworker is busy waiting for some lock or for some
> > other reason as shown in the email [1].
>
> Isn't handling this part of the job of wal_sender_timeout?
>

In some cases, it is not clear whether we can handle it by
wal_sender_timeout. Consider a case of a time-delayed replica where
the applyworker will keep sending some response/alive message so that
walsender doesn't timeout in that (during delay) period. In that case,
because walsender won't timeout, the shutdown will fail (with the
failed message) even though it will be complete after the walsender is
able to send all the WAL and shutdown. The time-delayed replica patch
is still under discussion [1]. Also, for large values of
wal_sender_timeout, it will wait till the walsender times out and can
return with a failed message.

>
> I don't at all agree that it's ok to just stop replicating changes
> because we're blocked on network IO. The patch justifies this with:
>
> > Currently, at shutdown, walsender processes wait to send all pending data and
> > ensure the all data is flushed in remote node. This mechanism was added by
> > 985bd7 for supporting clean switch over, but such use-case cannot be supported
> > for logical replication. This commit remove the blocking in the case.
>
> and at the start of the thread with:
>
> > In case of logical replication, however, we cannot support the use-case that
> > switches the role publisher <-> subscriber. Suppose same case as above, additional
> > transactions are committed while doing step2. To catch up such changes subscriber
> > must receive WALs related with trans, but it cannot be done because subscriber
> > cannot request WALs from the specific position. In the case, we must truncate all
> > data in new subscriber once, and then create new subscription with copy_data
> > = true.
>
> But that seems a too narrow view to me. Imagine you want to decomission
> the current primary, and instead start to use the logical standby as the
> primary. For that you'd obviously want to replicate the last few
> changes. But with the proposed change, that'd be hard to ever achieve.
>

I think that can still be achieved with the idea being discussed which
is to keep allowing sending the WAL for smart shutdown mode but not
for other modes(fast, immediate). I don't know whether it is a good
idea or not but Kuroda-San has produced a POC patch for it. We can
instead choose to improve our docs related to shutdown to explain a
bit more about the shutdown's interaction with (logical and physical)
replication. As of now, it says: (“Smart” mode disallows new
connections, then waits for all existing clients to disconnect. If the
server is in hot standby, recovery and streaming replication will be
terminated once all clients have disconnected.)[2]. Here, it is not
clear that shutdown will wait for sending and flushing all the WALs.
The information for fast and immediate modes is even lesser which
makes it more difficult to understand what kind of behavior is
expected in those modes.

[1] - https://commitfest.postgresql.org/42/3581/
[2] - https://www.postgresql.org/docs/devel/app-pg-ctl.html

-- 
With Regards,
Amit Kapila.

  35. Re: Exit walsender before confirming remote flush in logical replication

    Andres Freund <andres@anarazel.de> — 2023-02-06T05:03:03Z

    Hi, 

On February 5, 2023 8:29:19 PM PST, Amit Kapila <amit.kapila16@gmail.com> wrote:
>On Sat, Feb 4, 2023 at 6:31 PM Andres Freund <andres@anarazel.de> wrote:
>>
>> On 2023-02-02 11:21:54 +0530, Amit Kapila wrote:
>> > The main problem we want to solve here is to avoid shutdown failing in
>> > case walreceiver/applyworker is busy waiting for some lock or for some
>> > other reason as shown in the email [1].
>>
>> Isn't handling this part of the job of wal_sender_timeout?
>>
>
>In some cases, it is not clear whether we can handle it by
>wal_sender_timeout. Consider a case of a time-delayed replica where
>the applyworker will keep sending some response/alive message so that
>walsender doesn't timeout in that (during delay) period. In that case,
>because walsender won't timeout, the shutdown will fail (with the
>failed message) even though it will be complete after the walsender is
>able to send all the WAL and shutdown. The time-delayed replica patch
>is still under discussion [1]. Also, for large values of
>wal_sender_timeout, it will wait till the walsender times out and can
>return with a failed message.
>
>>
>> I don't at all agree that it's ok to just stop replicating changes
>> because we're blocked on network IO. The patch justifies this with:
>>
>> > Currently, at shutdown, walsender processes wait to send all pending data and
>> > ensure the all data is flushed in remote node. This mechanism was added by
>> > 985bd7 for supporting clean switch over, but such use-case cannot be supported
>> > for logical replication. This commit remove the blocking in the case.
>>
>> and at the start of the thread with:
>>
>> > In case of logical replication, however, we cannot support the use-case that
>> > switches the role publisher <-> subscriber. Suppose same case as above, additional
>> > transactions are committed while doing step2. To catch up such changes subscriber
>> > must receive WALs related with trans, but it cannot be done because subscriber
>> > cannot request WALs from the specific position. In the case, we must truncate all
>> > data in new subscriber once, and then create new subscription with copy_data
>> > = true.
>>
>> But that seems a too narrow view to me. Imagine you want to decomission
>> the current primary, and instead start to use the logical standby as the
>> primary. For that you'd obviously want to replicate the last few
>> changes. But with the proposed change, that'd be hard to ever achieve.
>>
>
>I think that can still be achieved with the idea being discussed which
>is to keep allowing sending the WAL for smart shutdown mode but not
>for other modes(fast, immediate). I don't know whether it is a good
>idea or not but Kuroda-San has produced a POC patch for it. We can
>instead choose to improve our docs related to shutdown to explain a
>bit more about the shutdown's interaction with (logical and physical)
>replication. As of now, it says: (“Smart” mode disallows new
>connections, then waits for all existing clients to disconnect. If the
>server is in hot standby, recovery and streaming replication will be
>terminated once all clients have disconnected.)[2]. Here, it is not
>clear that shutdown will wait for sending and flushing all the WALs.
>The information for fast and immediate modes is even lesser which
>makes it more difficult to understand what kind of behavior is
>expected in those modes.
>
>[1] - https://commitfest.postgresql.org/42/3581/
>[2] - https://www.postgresql.org/docs/devel/app-pg-ctl.html
>

Smart shutdown is practically unusable. I don't think it makes sense to tie behavior of walsender to it in any way. 


Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

  36. Re: Exit walsender before confirming remote flush in logical replication

    Amit Kapila <amit.kapila16@gmail.com> — 2023-02-06T06:53:54Z

    On Mon, Feb 6, 2023 at 10:33 AM Andres Freund <andres@anarazel.de> wrote:
>
> On February 5, 2023 8:29:19 PM PST, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >>
> >> But that seems a too narrow view to me. Imagine you want to decomission
> >> the current primary, and instead start to use the logical standby as the
> >> primary. For that you'd obviously want to replicate the last few
> >> changes. But with the proposed change, that'd be hard to ever achieve.
> >>
> >
> >I think that can still be achieved with the idea being discussed which
> >is to keep allowing sending the WAL for smart shutdown mode but not
> >for other modes(fast, immediate). I don't know whether it is a good
> >idea or not but Kuroda-San has produced a POC patch for it. We can
> >instead choose to improve our docs related to shutdown to explain a
> >bit more about the shutdown's interaction with (logical and physical)
> >replication. As of now, it says: (“Smart” mode disallows new
> >connections, then waits for all existing clients to disconnect. If the
> >server is in hot standby, recovery and streaming replication will be
> >terminated once all clients have disconnected.)[2]. Here, it is not
> >clear that shutdown will wait for sending and flushing all the WALs.
> >The information for fast and immediate modes is even lesser which
> >makes it more difficult to understand what kind of behavior is
> >expected in those modes.
> >
> >[1] - https://commitfest.postgresql.org/42/3581/
> >[2] - https://www.postgresql.org/docs/devel/app-pg-ctl.html
> >
>
> Smart shutdown is practically unusable. I don't think it makes sense to tie behavior of walsender to it in any way.
>

So, we have the following options: (a) do nothing for this; (b)
clarify the current behavior in docs. Any suggestions?

-- 
With Regards,
Amit Kapila.

  37. Re: Exit walsender before confirming remote flush in logical replication

    Andres Freund <andres@anarazel.de> — 2023-02-06T20:34:49Z

    Hi,

On 2023-02-06 12:23:54 +0530, Amit Kapila wrote:
> On Mon, Feb 6, 2023 at 10:33 AM Andres Freund <andres@anarazel.de> wrote:
> > Smart shutdown is practically unusable. I don't think it makes sense to tie behavior of walsender to it in any way.
> >
> 
> So, we have the following options: (a) do nothing for this; (b)
> clarify the current behavior in docs. Any suggestions?

b) seems good.

I also think it'd make sense to improve this on a code-level. Just not in the
wholesale way discussed so far.

How about we make it an option in START_REPLICATION? Delayed logical rep can
toggle that on by default.

Greetings,

Andres Freund

  38. Re: Exit walsender before confirming remote flush in logical replication

    Amit Kapila <amit.kapila16@gmail.com> — 2023-02-07T03:30:13Z

    On Tue, Feb 7, 2023 at 2:04 AM Andres Freund <andres@anarazel.de> wrote:
>
> On 2023-02-06 12:23:54 +0530, Amit Kapila wrote:
> > On Mon, Feb 6, 2023 at 10:33 AM Andres Freund <andres@anarazel.de> wrote:
> > > Smart shutdown is practically unusable. I don't think it makes sense to tie behavior of walsender to it in any way.
> > >
> >
> > So, we have the following options: (a) do nothing for this; (b)
> > clarify the current behavior in docs. Any suggestions?
>
> b) seems good.
>
> I also think it'd make sense to improve this on a code-level. Just not in the
> wholesale way discussed so far.
>
> How about we make it an option in START_REPLICATION? Delayed logical rep can
> toggle that on by default.
>

Works for me. So, when this option is set in START_REPLICATION
message, walsender will set some flag and allow itself to exit at
shutdown without waiting for WAL to be sent?

-- 
With Regards,
Amit Kapila.

  39. Re: Exit walsender before confirming remote flush in logical replication

    Andres Freund <andres@anarazel.de> — 2023-02-07T04:49:57Z

    Hi,

On 2023-02-07 09:00:13 +0530, Amit Kapila wrote:
> On Tue, Feb 7, 2023 at 2:04 AM Andres Freund <andres@anarazel.de> wrote:
> > How about we make it an option in START_REPLICATION? Delayed logical rep can
> > toggle that on by default.

> Works for me. So, when this option is set in START_REPLICATION
> message, walsender will set some flag and allow itself to exit at
> shutdown without waiting for WAL to be sent?

Yes. I think that might be useful in other situations as well, but we don't
need to make those configurable initially. But I imagine it'd be useful to set
things up so that non-HA physical replicas don't delay shutdown, particularly
if they're geographically far away.

Greetings,

Andres Freund

  40. RE: Exit walsender before confirming remote flush in logical replication

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2023-02-07T14:41:13Z

    Dear Andres, Amit,

> On 2023-02-07 09:00:13 +0530, Amit Kapila wrote:
> > On Tue, Feb 7, 2023 at 2:04 AM Andres Freund <andres@anarazel.de> wrote:
> > > How about we make it an option in START_REPLICATION? Delayed logical
> rep can
> > > toggle that on by default.
> 
> > Works for me. So, when this option is set in START_REPLICATION
> > message, walsender will set some flag and allow itself to exit at
> > shutdown without waiting for WAL to be sent?
> 
> Yes. I think that might be useful in other situations as well, but we don't
> need to make those configurable initially. But I imagine it'd be useful to set
> things up so that non-HA physical replicas don't delay shutdown, particularly
> if they're geographically far away.

Based on the discussion, I made a patch for adding a walsender option
exit_before_confirming to the START_STREAMING replication command. It can be
used for both physical and logical replication. I made the patch with
extendibility - it allows adding further options.
And better naming are very welcome.

For physical replication, the grammar was slightly changed like a logical one.
It can now accept options but currently, only one option is allowed. And it is
not used in normal streaming replication. For logical replication, the option is
combined with options for the output plugin. Of course, we can modify the API to
better style.

0001 patch was ported from time-delayed logical replication thread[1], which uses
the added option. When the min_apply_delay option is specified and publisher seems
to be PG16 or later, the apply worker sends a START_REPLICATION query with
exit_before_confirming = true. And the worker will reboot and send START_REPLICATION
again when min_apply_delay is changed from zero to a non-zero value or non-zero to zero.

Note that I removed version number because the approach is completely changed.

[1]: https://www.postgresql.org/message-id/TYCPR01MB8373BA483A6D2C924C600968EDDB9@TYCPR01MB8373.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

  41. RE: Exit walsender before confirming remote flush in logical replication

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2023-02-07T16:07:07Z

    > Dear Andres, Amit,
> 
> > On 2023-02-07 09:00:13 +0530, Amit Kapila wrote:
> > > On Tue, Feb 7, 2023 at 2:04 AM Andres Freund <andres@anarazel.de> wrote:
> > > > How about we make it an option in START_REPLICATION? Delayed logical
> > rep can
> > > > toggle that on by default.
> >
> > > Works for me. So, when this option is set in START_REPLICATION
> > > message, walsender will set some flag and allow itself to exit at
> > > shutdown without waiting for WAL to be sent?
> >
> > Yes. I think that might be useful in other situations as well, but we don't
> > need to make those configurable initially. But I imagine it'd be useful to set
> > things up so that non-HA physical replicas don't delay shutdown, particularly
> > if they're geographically far away.
> 
> Based on the discussion, I made a patch for adding a walsender option
> exit_before_confirming to the START_STREAMING replication command. It can
> be
> used for both physical and logical replication. I made the patch with
> extendibility - it allows adding further options.
> And better naming are very welcome.
> 
> For physical replication, the grammar was slightly changed like a logical one.
> It can now accept options but currently, only one option is allowed. And it is
> not used in normal streaming replication. For logical replication, the option is
> combined with options for the output plugin. Of course, we can modify the API to
> better style.
> 
> 0001 patch was ported from time-delayed logical replication thread[1], which
> uses
> the added option. When the min_apply_delay option is specified and publisher
> seems
> to be PG16 or later, the apply worker sends a START_REPLICATION query with
> exit_before_confirming = true. And the worker will reboot and send
> START_REPLICATION
> again when min_apply_delay is changed from zero to a non-zero value or non-zero
> to zero.
> 
> Note that I removed version number because the approach is completely changed.
> 
> [1]:
> https://www.postgresql.org/message-id/TYCPR01MB8373BA483A6D2C924C60
> 0968EDDB9@TYCPR01MB8373.jpnprd01.prod.outlook.com

I noticed that previous ones are rejected by cfbot, even if they passed on my environment...
PSA fixed version.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

  42. RE: Exit walsender before confirming remote flush in logical replication

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2023-02-07T17:08:54Z

    > I noticed that previous ones are rejected by cfbot, even if they passed on my
> environment...
> PSA fixed version.

While analyzing more, I found the further bug that forgets initialization.
PSA new version that could be passed automated tests on my github repository.
Sorry for noise.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

  43. Re: Exit walsender before confirming remote flush in logical replication

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2023-02-08T02:27:17Z

    I agree to the direction and thanks for the patch.

At Tue, 7 Feb 2023 17:08:54 +0000, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com> wrote in 
> > I noticed that previous ones are rejected by cfbot, even if they passed on my
> > environment...
> > PSA fixed version.
> 
> While analyzing more, I found the further bug that forgets initialization.
> PSA new version that could be passed automated tests on my github repository.
> Sorry for noise.

0002:

This patch doesn't seem to offer a means to change the default
walsender behavior.  We need a subscription option named like
"walsender_exit_mode" to do that.


+ConsumeWalsenderOptions(List *options, WalSndData *data)

I wonder if it is the right design to put options for different things
into a single list. I rather choose to embed the walsender option in
the syntax than needing this function.

K_START_REPLICATION opt_slot opt_physical RECPTR opt_timeline opt_shutdown_mode

K_START_REPLICATION K_SLOTIDENT K_LOGICAL RECPTR opt_shutdown_mode plugin_options

where opt_shutdown_mode would be like "SHUTDOWN_MODE immediate".


======
If we go with the current design, I think it is better to share the
option list rule between the logical and physical START_REPLCIATION
commands.

I'm not sure I like the option syntax
"exit_before_confirming=<Boolean>". I imagin that other options may
come in future. Thus, how about "walsender_shutdown_mode=<mode>",
where the mode is one of "wait_flush"(default) and "immediate"?


+typedef struct
+{
+	bool		exit_before_confirming;
+} WalSndData;

Data doesn't seem to represent the variable. Why not WalSndOptions?


-		!equal(newsub->publications, MySubscription->publications))
+		!equal(newsub->publications, MySubscription->publications) ||
+		(newsub->minapplydelay > 0 && MySubscription->minapplydelay == 0) ||
+		(newsub->minapplydelay == 0 && MySubscription->minapplydelay > 0))

 I slightly prefer the following expression (Others may disagree:p):

  ((newsub->minapplydelay == 0) != (MySubscription->minapplydelay == 0))

 And I think we need a comment for the term. For example,

  /* minapplydelay affects START_REPLICATION option exit_before_confirming */


+ * Reads all entrly of the list and consume if needed.
s/entrly/entries/ ?
...

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  44. Re: Exit walsender before confirming remote flush in logical replication

    Amit Kapila <amit.kapila16@gmail.com> — 2023-02-08T05:36:08Z

    On Wed, Feb 8, 2023 at 7:57 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> I agree to the direction and thanks for the patch.
>
> At Tue, 7 Feb 2023 17:08:54 +0000, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com> wrote in
> > > I noticed that previous ones are rejected by cfbot, even if they passed on my
> > > environment...
> > > PSA fixed version.
> >
> > While analyzing more, I found the further bug that forgets initialization.
> > PSA new version that could be passed automated tests on my github repository.
> > Sorry for noise.
>
> 0002:
>
> This patch doesn't seem to offer a means to change the default
> walsender behavior.  We need a subscription option named like
> "walsender_exit_mode" to do that.
>

I don't think at this stage we need a subscription-level option, we
can extend it later if this is really useful for users. For now, we
can set this new option when min_apply_delay > 0.

>
> +ConsumeWalsenderOptions(List *options, WalSndData *data)
>
> I wonder if it is the right design to put options for different things
> into a single list. I rather choose to embed the walsender option in
> the syntax than needing this function.
>
> K_START_REPLICATION opt_slot opt_physical RECPTR opt_timeline opt_shutdown_mode
>
> K_START_REPLICATION K_SLOTIDENT K_LOGICAL RECPTR opt_shutdown_mode plugin_options
>
> where opt_shutdown_mode would be like "SHUTDOWN_MODE immediate".
>

The other option could have been that we just add it as a
plugin_option for logical replication but it doesn't seem to match
with the other plugin options. I think it would be better to have it
as a separate option something like opt_shutdown_immediate and extend
the logical replication syntax for now. We can later extend physical
replication syntax when we want to expose such an option via physical
replication.

-- 
With Regards,
Amit Kapila.

  45. RE: Exit walsender before confirming remote flush in logical replication

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2023-02-08T07:59:09Z

    Dear Amit,

Thanks for giving comments!

> >
> > 0002:
> >
> > This patch doesn't seem to offer a means to change the default
> > walsender behavior.  We need a subscription option named like
> > "walsender_exit_mode" to do that.
> >
> 
> I don't think at this stage we need a subscription-level option, we
> can extend it later if this is really useful for users. For now, we
> can set this new option when min_apply_delay > 0.

Agreed. I wanted to keep the feature closed for PG16 and then will extend if needed.

> >
> > +ConsumeWalsenderOptions(List *options, WalSndData *data)
> >
> > I wonder if it is the right design to put options for different things
> > into a single list. I rather choose to embed the walsender option in
> > the syntax than needing this function.
> >
> > K_START_REPLICATION opt_slot opt_physical RECPTR opt_timeline
> opt_shutdown_mode
> >
> > K_START_REPLICATION K_SLOTIDENT K_LOGICAL RECPTR
> opt_shutdown_mode plugin_options
> >
> > where opt_shutdown_mode would be like "SHUTDOWN_MODE immediate".
> >
> 
> The other option could have been that we just add it as a
> plugin_option for logical replication but it doesn't seem to match
> with the other plugin options. I think it would be better to have it
> as a separate option something like opt_shutdown_immediate and extend
> the logical replication syntax for now. We can later extend physical
> replication syntax when we want to expose such an option via physical
> replication.

The main intention for us is to shut down logical walsenders. Therefore, same as above,
I want to develop the feature for logical replication once and then try to extend if we want.
TBH I think adding physicalrep support seems not to be so hard,
but I want to keep the patch smaller.

The new patch will be attached soon in another mail.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

  46. RE: Exit walsender before confirming remote flush in logical replication

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2023-02-08T08:01:24Z

    Dear Horiguchi-san,

Thank you for checking the patch! PSA new version.

> 0002:
> 
> This patch doesn't seem to offer a means to change the default
> walsender behavior.  We need a subscription option named like
> "walsender_exit_mode" to do that.

As I said in another mail[1], I'm thinking the feature does not have to be
used alone for now.

> +ConsumeWalsenderOptions(List *options, WalSndData *data)
> 
> I wonder if it is the right design to put options for different things
> into a single list. I rather choose to embed the walsender option in
> the syntax than needing this function.
> 
> K_START_REPLICATION opt_slot opt_physical RECPTR opt_timeline
> opt_shutdown_mode
> 
> K_START_REPLICATION K_SLOTIDENT K_LOGICAL RECPTR
> opt_shutdown_mode plugin_options
> 
> where opt_shutdown_mode would be like "SHUTDOWN_MODE immediate".

Right, the option handling was quite bad. I added new syntax opt_shutdown_mode
to logical replication. And many codes were modified accordingly.
Note that based on the other discussion, I removed codes
for supporting physical replication but tried to keep the extensibility.

> ======
> If we go with the current design, I think it is better to share the
> option list rule between the logical and physical START_REPLCIATION
> commands.
> 
> I'm not sure I like the option syntax
> "exit_before_confirming=<Boolean>". I imagin that other options may
> come in future. Thus, how about "walsender_shutdown_mode=<mode>",
> where the mode is one of "wait_flush"(default) and "immediate"?

Seems better, I changed to from boolean to enumeration.

> +typedef struct
> +{
> +	bool		exit_before_confirming;
> +} WalSndData;
> 
> Data doesn't seem to represent the variable. Why not WalSndOptions?

This is inspired by PGOutputData, but I prefer your idea. Fixed.

> -		!equal(newsub->publications, MySubscription->publications))
> +		!equal(newsub->publications, MySubscription->publications) ||
> +		(newsub->minapplydelay > 0 &&
> MySubscription->minapplydelay == 0) ||
> +		(newsub->minapplydelay == 0 &&
> MySubscription->minapplydelay > 0))
> 
>  I slightly prefer the following expression (Others may disagree:p):
> 
>   ((newsub->minapplydelay == 0) != (MySubscription->minapplydelay == 0))

I think conditions for the same parameter should be aligned one line,
So your posted seems better. Fixed.

> 
>  And I think we need a comment for the term. For example,
> 
>   /* minapplydelay affects START_REPLICATION option exit_before_confirming
> */

Added just above the condition.

> + * Reads all entrly of the list and consume if needed.
> s/entrly/entries/ ?
> ...

This part is no longer needed.

[1]: https://www.postgresql.org/message-id/TYAPR01MB5866D3EC780D251953BDE7FAF5D89%40TYAPR01MB5866.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

  47. RE: Exit walsender before confirming remote flush in logical replication

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2023-02-08T09:47:08Z

    > 
> Dear Horiguchi-san,
> 
> Thank you for checking the patch! PSA new version.

PSA rebased patch that supports updated time-delayed patch[1].

[1]: https://www.postgresql.org/message-id/TYAPR01MB5866C11DAF8AB04F3CC181D3F5D89@TYAPR01MB5866.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

  48. RE: Exit walsender before confirming remote flush in logical replication

    Takamichi Osumi (Fujitsu) <osumi.takamichi@fujitsu.com> — 2023-02-09T05:50:05Z

    On Wednesday, February 8, 2023 6:47 PM Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> wrote:
> PSA rebased patch that supports updated time-delayed patch[1].
Hi,

Thanks for creating the patch ! Minor review comments on v5-0002.

(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.

(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".

(1-3)

We don't need to wrap wait_flush and immediate by single quotes
within the literal tag.

(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.

(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 ?

(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..."


Best Regards,
	Takamichi Osumi

  49. Re: Exit walsender before confirming remote flush in logical replication

    Masahiko Sawada <sawada.mshk@gmail.com> — 2023-02-09T08:33:57Z

    Hi,

On Wed, Feb 8, 2023 at 6:47 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> >
> > Dear Horiguchi-san,
> >
> > Thank you for checking the patch! PSA new version.
>
> PSA rebased patch that supports updated time-delayed patch[1].
>

Thank you for the patch! Here are some comments on v5 patch:

+/*
+ * Options for controlling the behavior of the walsender. Options can be
+ * specified in the START_STREAMING replication command. Currently only one
+ * option is allowed.
+ */
+typedef struct
+{
+        WalSndShutdownMode shutdown_mode;
+} WalSndOptions;
+
+static WalSndOptions *my_options = NULL;

I'm not sure we need to have it as a struct at this stage since we
support only one option. I wonder if we can have one value, say
shutdown_mode, and we can make it a struct when we really need it.
Even if we use WalSndOptions struct, I don't think we need to
dynamically allocate it. Since a walsender can start logical
replication multiple times in principle, my_options is not freed.

---
+/*
+ * Parse given shutdown mode.
+ *
+ * Currently two values are accepted - "wait_flush" and "immediate"
+ */
+static void
+ParseShutdownMode(char *shutdownmode)
+{
+        if (pg_strcasecmp(shutdownmode, "wait_flush") == 0)
+                my_options->shutdown_mode = WALSND_SHUTDOWN_MODE_WAIT_FLUSH;
+        else if (pg_strcasecmp(shutdownmode, "immediate") == 0)
+                my_options->shutdown_mode = WALSND_SHUTDOWN_MODE_IMMIDEATE;
+        else
+                ereport(ERROR,
+                                errcode(ERRCODE_SYNTAX_ERROR),
+                                errmsg("SHUTDOWN_MODE requires
\"wait_flush\" or \"immediate\""));
+}

I think we should make the error message consistent with other enum
parameters. How about the message like:

ERROR:  invalid value shutdown mode: "%s"

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

  50. RE: Exit walsender before confirming remote flush in logical replication

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2023-02-09T10:11:10Z

    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

  51. RE: Exit walsender before confirming remote flush in logical replication

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2023-02-09T10:12:03Z

    Dear Sawada-san,

Thank you for reviewing!

> +/*
> + * Options for controlling the behavior of the walsender. Options can be
> + * specified in the START_STREAMING replication command. Currently only one
> + * option is allowed.
> + */
> +typedef struct
> +{
> +        WalSndShutdownMode shutdown_mode;
> +} WalSndOptions;
> +
> +static WalSndOptions *my_options = NULL;
> 
> I'm not sure we need to have it as a struct at this stage since we
> support only one option. I wonder if we can have one value, say
> shutdown_mode, and we can make it a struct when we really need it.
> Even if we use WalSndOptions struct, I don't think we need to
> dynamically allocate it. Since a walsender can start logical
> replication multiple times in principle, my_options is not freed.

+1, removed the structure.

> ---
> +/*
> + * Parse given shutdown mode.
> + *
> + * Currently two values are accepted - "wait_flush" and "immediate"
> + */
> +static void
> +ParseShutdownMode(char *shutdownmode)
> +{
> +        if (pg_strcasecmp(shutdownmode, "wait_flush") == 0)
> +                my_options->shutdown_mode =
> WALSND_SHUTDOWN_MODE_WAIT_FLUSH;
> +        else if (pg_strcasecmp(shutdownmode, "immediate") == 0)
> +                my_options->shutdown_mode =
> WALSND_SHUTDOWN_MODE_IMMIDEATE;
> +        else
> +                ereport(ERROR,
> +                                errcode(ERRCODE_SYNTAX_ERROR),
> +                                errmsg("SHUTDOWN_MODE requires
> \"wait_flush\" or \"immediate\""));
> +}
> 
> I think we should make the error message consistent with other enum
> parameters. How about the message like:
> 
> ERROR:  invalid value shutdown mode: "%s"

Modified like enum parameters and hint message was also provided.

New patch is attached in [1].

[1]: https://www.postgresql.org/message-id/TYAPR01MB586683FC450662990E356A0EF5D99%40TYAPR01MB5866.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

  52. Re: Exit walsender before confirming remote flush in logical replication

    Peter Smith <smithpb2250@gmail.com> — 2023-02-10T07:10:53Z

    Here are my review comments for the v6-0002 patch.

======
Commit Message

1.
This commit extends START_REPLICATION to accept SHUTDOWN_MODE term. Currently,
it works well only for logical replication.

~

1a.
"to accept SHUTDOWN term" --> "to include a SHUTDOWN_MODE clause."

~

1b.
"it works well only for..." --> do you mean "it is currently
implemented only for..."

~~~

2.
When 'wait_flush', which is the default, is specified, the walsender will wait
for all the sent WALs to be flushed on the subscriber side, before exiting the
process. 'immediate' 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. This may be useful to shut down the publisher even when the
worker is stuck.

~

SUGGESTION
The shutdown modes are:

1) 'wait_flush' (the default). In this mode, the walsender will wait
for all the sent WALs to be flushed on the subscriber side, before
exiting the process.

2) 'immediate'. In this mode, the walsender will exit without
confirming the remote flush. This may break the consistency between
publisher and subscriber. This mode might be useful for a system that
has a high-latency network (to reduce the amount of time for
shutdown), or to allow the shutdown of the publisher even when the
worker is stuck.

======
doc/src/sgml/protocol.sgml

3.
+       <varlistentry>
+        <term><literal>SHUTDOWN_MODE { 'wait_flush' | 'immediate'
}</literal></term>
+        <listitem>
+         <para>
+          Decides the behavior of the walsender process at shutdown. If the
+          shutdown mode is <literal>'wait_flush'</literal>, which is the
+          default, the walsender waits for all the sent WALs to be flushed
+          on the subscriber side. If it is <literal>'immediate'</literal>,
+          the walsender exits without confirming the remote flush.
+         </para>
+        </listitem>
+       </varlistentry>

The synopsis said:
[ SHUTDOWN_MODE shutdown_mode ]

But then the 'shutdown_mode' term was never mentioned again (??).
Instead it says:
SHUTDOWN_MODE { 'wait_flush' | 'immediate' }

IMO the detailed explanation should not say SHUTDOWN_MODE again. It
should be writtenmore  like this:

SUGGESTION
shutdown_mode

Determines the behavior of the walsender process at shutdown. If
shutdown_mode is 'wait_flush', the walsender waits for all the sent
WALs to be flushed on the subscriber side. This is the default when
SHUTDOWN_MODE is not specified.

If shutdown_mode is 'immediate', the walsender exits without
confirming the remote flush.

======
.../libpqwalreceiver/libpqwalreceiver.c

4.
+ /* Add SHUTDOWN_MODE option if needed */
+ if (options->shutdown_mode &&
+ PQserverVersion(conn->streamConn) >= 160000)
+ appendStringInfo(&cmd, " SHUTDOWN_MODE '%s'",
+ options->shutdown_mode);

Maybe you can expand on the meaning of "if needed".

SUGGESTION
Add SHUTDOWN_MODE clause if needed (i.e. if not using the default shutdown_mode)

======
src/backend/replication/logical/worker.c

5. maybe_reread_subscription

+ *
+ * minapplydelay affects SHUTDOWN_MODE option. 'immediate' shutdown mode
+ * will be specified if it is set to non-zero, otherwise default mode will
+ * be set.

Reworded this comment slightly and give a reference to ApplyWorkerMain.

SUGGESTION
Time-delayed logical replication affects the SHUTDOWN_MODE clause. The
'immediate' shutdown mode will be specified if min_apply_delay is
non-zero, otherwise the default shutdown mode will be used. See
ApplyWorkerMain.

~~~

6. ApplyWorkerMain
+ /*
+ * time-delayed logical replication does not support tablesync
+ * workers, so only the leader apply worker can request walsenders to
+ * exit before confirming remote flush.
+ */

"time-delayed" --> "Time-delayed"

======
src/backend/replication/repl_gram.y

7.
@@ -91,6 +92,7 @@ Node *replication_parse_result;
 %type <boolval> opt_temporary
 %type <list> create_slot_options create_slot_legacy_opt_list
 %type <defelt> create_slot_legacy_opt
+%type <str> opt_shutdown_mode

The tab alignment seemed not quite right. Not 100% sure.

~~~

8.
@@ -270,20 +272,22 @@ start_replication:
  cmd->slotname = $2;
  cmd->startpoint = $4;
  cmd->timeline = $5;
+ cmd->shutdownmode = NULL;
  $$ = (Node *) cmd;
  }

It seemed a bit inconsistent. E.g. the cmd->options member was not set
for physical replication, so why then set this member?

Alternatively, maybe should set cmd->options = NULL here as well?

======
src/backend/replication/walsender.c

9.
+/* Indicator for specifying the shutdown mode */
+typedef enum
+{
+ WALSND_SHUTDOWN_MODE_WAIT_FLUSH = 0,
+ WALSND_SHUTDOWN_MODE_IMMIDEATE
+} WalSndShutdownMode;

~

9a.
"Indicator for specifying" (??). How about just saying: "Shutdown modes"

~

9b.
Typo: WALSND_SHUTDOWN_MODE_IMMIDEATE ==> WALSND_SHUTDOWN_MODE_IMMEDIATE

~

9c.
AFAICT the fact that the first enum value is assigned 0 is not really
of importance. If that is correct, then IMO maybe it's better to
remove the "= 0" because the explicit assignment made me expect that
it had special meaning, and then it was confusing when I could not
find a reason.

~~~

10. ProcessPendingWrites

+ /*
+ * In this function, there is a possibility that the walsender is
+ * stuck. It is caused when the opposite worker is stuck and then the
+ * send-buffer of the walsender becomes full. Therefore, we must add
+ * an additional path for shutdown for immediate shutdown mode.
+ */
+ if (shutdown_mode == WALSND_SHUTDOWN_MODE_IMMIDEATE &&
+ got_STOPPING)
+ WalSndDone(XLogSendLogical);

10a.
Can this comment say something like "receiving worker" instead of
"opposite worker"?

SUGGESTION
This can happen when the receiving worker is stuck, and then the
send-buffer of the walsender...

~

10b.
IMO it makes more sense to check this around the other way. E.g. we
don't care what is the shutdown_mode value unless got_STOPPING is
true.

SUGGESTION
if (got_STOPPING && (shutdown_mode == WALSND_SHUTDOWN_MODE_IMMEDIATE))

~~~

11. WalSndDone

+ * If we are in the immediate shutdown mode, flush location and output
+ * buffer is not checked. This may break the consistency between nodes,
+ * but it may be useful for the system that has high-latency network to
+ * reduce the amount of time for shutdown.

Add some quotes for the mode.

SUGGESTION
'immediate' shutdown mode

~~~

12.
+/*
+ * Check options for walsender itself and set flags accordingly.
+ *
+ * Currently only one option is accepted.
+ */
+static void
+CheckWalSndOptions(const StartReplicationCmd *cmd)
+{
+ if (cmd->shutdownmode)
+ ParseShutdownMode(cmd->shutdownmode);
+}
+
+/*
+ * Parse given shutdown mode.
+ *
+ * Currently two values are accepted - "wait_flush" and "immediate"
+ */
+static void
+ParseShutdownMode(char *shutdownmode)
+{
+ if (pg_strcasecmp(shutdownmode, "wait_flush") == 0)
+ shutdown_mode = WALSND_SHUTDOWN_MODE_WAIT_FLUSH;
+ else if (pg_strcasecmp(shutdownmode, "immediate") == 0)
+ shutdown_mode = WALSND_SHUTDOWN_MODE_IMMIDEATE;
+ else
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("invalid value for shutdown mode: \"%s\"", shutdownmode),
+ errhint("Available values: wait_flush, immediate."));
+}

IMO the ParseShutdownMode function seems unnecessary because it's not
really "parsing" anything and it is only called in one place. I
suggest wrapping everything into the CheckWalSndOptions function. The
end result is still only a simple function:

SUGGESTION

static void
CheckWalSndOptions(const StartReplicationCmd *cmd)
{
if (cmd->shutdownmode)
{
char *mode = cmd->shutdownmode;

if (pg_strcasecmp(mode, "wait_flush") == 0)
shutdown_mode = WALSND_SHUTDOWN_MODE_WAIT_FLUSH;
else if (pg_strcasecmp(mode, "immediate") == 0)
shutdown_mode = WALSND_SHUTDOWN_MODE_IMMEDIATE;

else
ereport(ERROR,
errcode(ERRCODE_SYNTAX_ERROR),
errmsg("invalid value for shutdown mode: \"%s\"", mode),
errhint("Available values: wait_flush, immediate."));
}
}

======
src/include/replication/walreceiver.h

13.
@@ -170,6 +170,7 @@ typedef struct
  * false if physical stream.  */
  char    *slotname; /* Name of the replication slot or NULL. */
  XLogRecPtr startpoint; /* LSN of starting point. */
+ char    *shutdown_mode; /* Name of specified shutdown name */

  union
  {
~

13a.
Typo (shutdown name?)

SUGGESTION
/* The specified shutdown mode string, or NULL. */

~

13b.
Because they have the same member names I kept confusing this option
shutdown_mode with the other enum also called shutdown_mode.

I wonder if is it possible to call this one something like
'shutdown_mode_str' to make reading the code easier?

~

13c.
Is this member in the right place? AFAIK this is not even implemented
for physical replication. e.g. Why isn't this new member part of the
'logical' sub-structure in the union?

======
src/test/subscription/t/001_rep_changes.pl

14.
-# Set min_apply_delay parameter to 3 seconds
+# Check restart on changing min_apply_delay to 3 seconds
 my $delay = 3;
 $node_subscriber->safe_psql('postgres',
  "ALTER SUBSCRIPTION tap_sub_renamed SET (min_apply_delay = '${delay}s')");
+$node_publisher->poll_query_until('postgres',
+ "SELECT pid != $oldpid FROM pg_stat_replication WHERE
application_name = 'tap_sub_renamed' AND state = 'streaming';"
+  )
+  or die
+  "Timed out while waiting for the walsender to restart after
changing min_apply_delay to non-zero value";

IIUC this test is for verifying that a new walsender worker was
started if the delay was changed from 0 to non-zero. E.g. I think it
is for it is testing your new logic of the maybe_reread_subscription.

Probably more complete testing also needs to check the other scenarios:
* min_apply_delay from one non-zero value to another non-zero value
--> verify a new worker is NOT started.
* change min_apply_delay from non-zero to zero --> verify a new worker
IS started

------
Kind Regards,
Peter Smith.
Fujitsu Australia

  53. RE: Exit walsender before confirming remote flush in logical replication

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2023-02-10T11:54:01Z

    Dear Peter,

Thank you for reviewing! PSA new version.

> ======
> Commit Message
> 
> 1.
> This commit extends START_REPLICATION to accept SHUTDOWN_MODE term.
> Currently,
> it works well only for logical replication.
> 
> ~
> 
> 1a.
> "to accept SHUTDOWN term" --> "to include a SHUTDOWN_MODE clause."

Fixed.

> 1b.
> "it works well only for..." --> do you mean "it is currently
> implemented only for..."

Fixed.

> 2.
> When 'wait_flush', which is the default, is specified, the walsender will wait
> for all the sent WALs to be flushed on the subscriber side, before exiting the
> process. 'immediate' 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. This may be useful to shut down the publisher even when the
> worker is stuck.
> 
> ~
> 
> SUGGESTION
> The shutdown modes are:
> 
> 1) 'wait_flush' (the default). In this mode, the walsender will wait
> for all the sent WALs to be flushed on the subscriber side, before
> exiting the process.
> 
> 2) 'immediate'. In this mode, the walsender will exit without
> confirming the remote flush. This may break the consistency between
> publisher and subscriber. This mode might be useful for a system that
> has a high-latency network (to reduce the amount of time for
> shutdown), or to allow the shutdown of the publisher even when the
> worker is stuck.
> 
> ======
> doc/src/sgml/protocol.sgml
> 
> 3.
> +       <varlistentry>
> +        <term><literal>SHUTDOWN_MODE { 'wait_flush' | 'immediate'
> }</literal></term>
> +        <listitem>
> +         <para>
> +          Decides the behavior of the walsender process at shutdown. If the
> +          shutdown mode is <literal>'wait_flush'</literal>, which is the
> +          default, the walsender waits for all the sent WALs to be flushed
> +          on the subscriber side. If it is <literal>'immediate'</literal>,
> +          the walsender exits without confirming the remote flush.
> +         </para>
> +        </listitem>
> +       </varlistentry>
> 
> The synopsis said:
> [ SHUTDOWN_MODE shutdown_mode ]
> 
> But then the 'shutdown_mode' term was never mentioned again (??).
> Instead it says:
> SHUTDOWN_MODE { 'wait_flush' | 'immediate' }
> 
> IMO the detailed explanation should not say SHUTDOWN_MODE again. It
> should be writtenmore  like this:
> 
> SUGGESTION
> shutdown_mode
> 
> Determines the behavior of the walsender process at shutdown. If
> shutdown_mode is 'wait_flush', the walsender waits for all the sent
> WALs to be flushed on the subscriber side. This is the default when
> SHUTDOWN_MODE is not specified.
> 
> If shutdown_mode is 'immediate', the walsender exits without
> confirming the remote flush.

Fixed.

> .../libpqwalreceiver/libpqwalreceiver.c
> 
> 4.
> + /* Add SHUTDOWN_MODE option if needed */
> + if (options->shutdown_mode &&
> + PQserverVersion(conn->streamConn) >= 160000)
> + appendStringInfo(&cmd, " SHUTDOWN_MODE '%s'",
> + options->shutdown_mode);
> 
> Maybe you can expand on the meaning of "if needed".
> 
> SUGGESTION
> Add SHUTDOWN_MODE clause if needed (i.e. if not using the default
> shutdown_mode)

Fixed, but not completely same as your suggestion.

> src/backend/replication/logical/worker.c
> 
> 5. maybe_reread_subscription
> 
> + *
> + * minapplydelay affects SHUTDOWN_MODE option. 'immediate' shutdown
> mode
> + * will be specified if it is set to non-zero, otherwise default mode will
> + * be set.
> 
> Reworded this comment slightly and give a reference to ApplyWorkerMain.
> 
> SUGGESTION
> Time-delayed logical replication affects the SHUTDOWN_MODE clause. The
> 'immediate' shutdown mode will be specified if min_apply_delay is
> non-zero, otherwise the default shutdown mode will be used. See
> ApplyWorkerMain.

Fixed.

> 6. ApplyWorkerMain
> + /*
> + * time-delayed logical replication does not support tablesync
> + * workers, so only the leader apply worker can request walsenders to
> + * exit before confirming remote flush.
> + */
> 
> "time-delayed" --> "Time-delayed"

Fixed.

> src/backend/replication/repl_gram.y
> 
> 7.
> @@ -91,6 +92,7 @@ Node *replication_parse_result;
>  %type <boolval> opt_temporary
>  %type <list> create_slot_options create_slot_legacy_opt_list
>  %type <defelt> create_slot_legacy_opt
> +%type <str> opt_shutdown_mode
> 
> The tab alignment seemed not quite right. Not 100% sure.

Fixed accordingly.

> 8.
> @@ -270,20 +272,22 @@ start_replication:
>   cmd->slotname = $2;
>   cmd->startpoint = $4;
>   cmd->timeline = $5;
> + cmd->shutdownmode = NULL;
>   $$ = (Node *) cmd;
>   }
> 
> It seemed a bit inconsistent. E.g. the cmd->options member was not set
> for physical replication, so why then set this member?
> 
> Alternatively, maybe should set cmd->options = NULL here as well?

Removed. I checked makeNode() macro, found that palloc0fast() is called there.
This means that we do not have to initialize unused attributes.

> src/backend/replication/walsender.c
> 
> 9.
> +/* Indicator for specifying the shutdown mode */
> +typedef enum
> +{
> + WALSND_SHUTDOWN_MODE_WAIT_FLUSH = 0,
> + WALSND_SHUTDOWN_MODE_IMMIDEATE
> +} WalSndShutdownMode;
> 
> ~
> 
> 9a.
> "Indicator for specifying" (??). How about just saying: "Shutdown modes"

Fixed.

> 9b.
> Typo: WALSND_SHUTDOWN_MODE_IMMIDEATE ==>
> WALSND_SHUTDOWN_MODE_IMMEDIATE

Replaced.

> 9c.
> AFAICT the fact that the first enum value is assigned 0 is not really
> of importance. If that is correct, then IMO maybe it's better to
> remove the "= 0" because the explicit assignment made me expect that
> it had special meaning, and then it was confusing when I could not
> find a reason.

Removed. This was added for skipping the initialization for previous version,
but no longer needed.

> 10. ProcessPendingWrites
> 
> + /*
> + * In this function, there is a possibility that the walsender is
> + * stuck. It is caused when the opposite worker is stuck and then the
> + * send-buffer of the walsender becomes full. Therefore, we must add
> + * an additional path for shutdown for immediate shutdown mode.
> + */
> + if (shutdown_mode == WALSND_SHUTDOWN_MODE_IMMIDEATE &&
> + got_STOPPING)
> + WalSndDone(XLogSendLogical);
> 
> 10a.
> Can this comment say something like "receiving worker" instead of
> "opposite worker"?
> 
> SUGGESTION
> This can happen when the receiving worker is stuck, and then the
> send-buffer of the walsender...

Changed.

> 10b.
> IMO it makes more sense to check this around the other way. E.g. we
> don't care what is the shutdown_mode value unless got_STOPPING is
> true.
> 
> SUGGESTION
> if (got_STOPPING && (shutdown_mode ==
> WALSND_SHUTDOWN_MODE_IMMEDIATE))

Changed.

> 11. WalSndDone
> 
> + * If we are in the immediate shutdown mode, flush location and output
> + * buffer is not checked. This may break the consistency between nodes,
> + * but it may be useful for the system that has high-latency network to
> + * reduce the amount of time for shutdown.
> 
> Add some quotes for the mode.
> 
> SUGGESTION
> 'immediate' shutdown mode

Changed.

> 12.
> +/*
> + * Check options for walsender itself and set flags accordingly.
> + *
> + * Currently only one option is accepted.
> + */
> +static void
> +CheckWalSndOptions(const StartReplicationCmd *cmd)
> +{
> + if (cmd->shutdownmode)
> + ParseShutdownMode(cmd->shutdownmode);
> +}
> +
> +/*
> + * Parse given shutdown mode.
> + *
> + * Currently two values are accepted - "wait_flush" and "immediate"
> + */
> +static void
> +ParseShutdownMode(char *shutdownmode)
> +{
> + if (pg_strcasecmp(shutdownmode, "wait_flush") == 0)
> + shutdown_mode = WALSND_SHUTDOWN_MODE_WAIT_FLUSH;
> + else if (pg_strcasecmp(shutdownmode, "immediate") == 0)
> + shutdown_mode = WALSND_SHUTDOWN_MODE_IMMIDEATE;
> + else
> + ereport(ERROR,
> + errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("invalid value for shutdown mode: \"%s\"", shutdownmode),
> + errhint("Available values: wait_flush, immediate."));
> +}
> 
> IMO the ParseShutdownMode function seems unnecessary because it's not
> really "parsing" anything and it is only called in one place. I
> suggest wrapping everything into the CheckWalSndOptions function. The
> end result is still only a simple function:
> 
> SUGGESTION
> 
> static void
> CheckWalSndOptions(const StartReplicationCmd *cmd)
> {
> if (cmd->shutdownmode)
> {
> char *mode = cmd->shutdownmode;
> 
> if (pg_strcasecmp(mode, "wait_flush") == 0)
> shutdown_mode = WALSND_SHUTDOWN_MODE_WAIT_FLUSH;
> else if (pg_strcasecmp(mode, "immediate") == 0)
> shutdown_mode = WALSND_SHUTDOWN_MODE_IMMEDIATE;
> 
> else
> ereport(ERROR,
> errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("invalid value for shutdown mode: \"%s\"", mode),
> errhint("Available values: wait_flush, immediate."));
> }
> }

Removed.

> ======
> src/include/replication/walreceiver.h
> 
> 13.
> @@ -170,6 +170,7 @@ typedef struct
>   * false if physical stream.  */
>   char    *slotname; /* Name of the replication slot or NULL. */
>   XLogRecPtr startpoint; /* LSN of starting point. */
> + char    *shutdown_mode; /* Name of specified shutdown name */
> 
>   union
>   {
> ~
> 
> 13a.
> Typo (shutdown name?)
> 
> SUGGESTION
> /* The specified shutdown mode string, or NULL. */

Fixed.

> 13b.
> Because they have the same member names I kept confusing this option
> shutdown_mode with the other enum also called shutdown_mode.
> 
> I wonder if is it possible to call this one something like
> 'shutdown_mode_str' to make reading the code easier?

Changed.

> 13c.
> Is this member in the right place? AFAIK this is not even implemented
> for physical replication. e.g. Why isn't this new member part of the
> 'logical' sub-structure in the union?

I remained for future extendibility, but it seemed not to be needed. Moved.

> ======
> src/test/subscription/t/001_rep_changes.pl
> 
> 14.
> -# Set min_apply_delay parameter to 3 seconds
> +# Check restart on changing min_apply_delay to 3 seconds
>  my $delay = 3;
>  $node_subscriber->safe_psql('postgres',
>   "ALTER SUBSCRIPTION tap_sub_renamed SET (min_apply_delay =
> '${delay}s')");
> +$node_publisher->poll_query_until('postgres',
> + "SELECT pid != $oldpid FROM pg_stat_replication WHERE
> application_name = 'tap_sub_renamed' AND state = 'streaming';"
> +  )
> +  or die
> +  "Timed out while waiting for the walsender to restart after
> changing min_apply_delay to non-zero value";
> 
> IIUC this test is for verifying that a new walsender worker was
> started if the delay was changed from 0 to non-zero. E.g. I think it
> is for it is testing your new logic of the maybe_reread_subscription.
> 
> Probably more complete testing also needs to check the other scenarios:
> * min_apply_delay from one non-zero value to another non-zero value
> --> verify a new worker is NOT started.
> * change min_apply_delay from non-zero to zero --> verify a new worker
> IS started

Hmm. These tests do not improve the coverage, so not sure we should test or not.
Moreover, IIUC we do not have a good way to verify that the worker does not restart.
Even if the old pid is remained in the pg_stat_replication, there is a possibility
that walsender exits after that.  So currently I added only the case that change
min_apply_delay from non-zero to zero.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

  54. Re: Exit walsender before confirming remote flush in logical replication

    Amit Kapila <amit.kapila16@gmail.com> — 2023-02-10T12:15:01Z

    On Fri, Feb 10, 2023 at 5:24 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>

Can't we have this option just as a bool (like shutdown_immediate)?
Why do we want to keep multiple modes?

-- 
With Regards,
Amit Kapila.

  55. RE: Exit walsender before confirming remote flush in logical replication

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2023-02-10T12:40:43Z

    Dear Amit,

> Can't we have this option just as a bool (like shutdown_immediate)?
> Why do we want to keep multiple modes?

Of course we can use boolean instead, but current style is motivated by the post[1].
This allows to add another option in future, whereas I do not have idea now.

I want to ask other reviewers which one is better...

[1]: https://www.postgresql.org/message-id/20230208.112717.1140830361804418505.horikyota.ntt%40gmail.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

  56. Re: Exit walsender before confirming remote flush in logical replication

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2023-02-13T01:56:51Z

    At Fri, 10 Feb 2023 12:40:43 +0000, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com> wrote in 
> Dear Amit,
> 
> > Can't we have this option just as a bool (like shutdown_immediate)?
> > Why do we want to keep multiple modes?
> 
> Of course we can use boolean instead, but current style is motivated by the post[1].
> This allows to add another option in future, whereas I do not have idea now.
> 
> I want to ask other reviewers which one is better...
> 
> [1]: https://www.postgresql.org/message-id/20230208.112717.1140830361804418505.horikyota.ntt%40gmail.com

IMHO I vaguely don't like that we lose a means to specify the default
behavior here. And I'm not sure we definitely don't need other than
flush and immedaite for both physical and logical replication. If it's
not the case, I don't object to make it a Boolean.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  57. Re: Exit walsender before confirming remote flush in logical replication

    Amit Kapila <amit.kapila16@gmail.com> — 2023-02-13T02:57:01Z

    On Mon, Feb 13, 2023 at 7:26 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Fri, 10 Feb 2023 12:40:43 +0000, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com> wrote in
> > Dear Amit,
> >
> > > Can't we have this option just as a bool (like shutdown_immediate)?
> > > Why do we want to keep multiple modes?
> >
> > Of course we can use boolean instead, but current style is motivated by the post[1].
> > This allows to add another option in future, whereas I do not have idea now.
> >
> > I want to ask other reviewers which one is better...
> >
> > [1]: https://www.postgresql.org/message-id/20230208.112717.1140830361804418505.horikyota.ntt%40gmail.com
>
> IMHO I vaguely don't like that we lose a means to specify the default
> behavior here. And I'm not sure we definitely don't need other than
> flush and immedaite for both physical and logical replication.
>

If we can think of any use case that requires its extension then it
makes sense to make it a non-boolean option but otherwise, let's keep
things simple by having a boolean option.

> If it's
> not the case, I don't object to make it a Boolean.
>

Thanks.

-- 
With Regards,
Amit Kapila.

  58. Re: Exit walsender before confirming remote flush in logical replication

    Peter Smith <smithpb2250@gmail.com> — 2023-02-13T04:09:59Z

    Here are some comments for patch v7-0002.

======
Commit Message

1.
This commit extends START_REPLICATION to accept SHUTDOWN_MODE clause. It is
currently implemented only for logical replication.

~

"to accept SHUTDOWN_MODE clause." --> "to accept a SHUTDOWN_MODE clause."

======
doc/src/sgml/protocol.sgml

2.
START_REPLICATION SLOT slot_name LOGICAL XXX/XXX [ SHUTDOWN_MODE {
'wait_flush' | 'immediate' } ] [ ( option_name [ option_value ] [,
...] ) ]

~

IMO this should say shutdown_mode as it did before:
START_REPLICATION SLOT slot_name LOGICAL XXX/XXX [ SHUTDOWN_MODE
shutdown_mode ] [ ( option_name [ option_value ] [, ...] ) ]

~~~

3.
+       <varlistentry>
+        <term><literal>shutdown_mode</literal></term>
+        <listitem>
+         <para>
+          Determines the behavior of the walsender process at shutdown. If
+          shutdown_mode is <literal>'wait_flush'</literal>, the walsender waits
+          for all the sent WALs to be flushed on the subscriber side. This is
+          the default when SHUTDOWN_MODE is not specified. If shutdown_mode is
+          <literal>'immediate'</literal>, the walsender exits without
+          confirming the remote flush.
+         </para>
+        </listitem>
+       </varlistentry>

Is the font of the "shutdown_mode" correct? I expected it to be like
the others (e.g. slot_name)

======
src/backend/replication/walsender.c

4.
+static void
+CheckWalSndOptions(const StartReplicationCmd *cmd)
+{
+ if (cmd->shutdownmode)
+ {
+ char    *mode = cmd->shutdownmode;
+
+ if (pg_strcasecmp(mode, "wait_flush") == 0)
+ shutdown_mode = WALSND_SHUTDOWN_MODE_WAIT_FLUSH;
+ else if (pg_strcasecmp(mode, "immediate") == 0)
+ shutdown_mode = WALSND_SHUTDOWN_MODE_IMMEDIATE;
+ else
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("invalid value for shutdown mode: \"%s\"", mode),
+ errhint("Available values: wait_flush, immediate."));
+ }
+
+}

Unnecessary extra whitespace at end of the function.

======
src/include/nodes/replnodes.

5.
@@ -83,6 +83,7 @@ typedef struct StartReplicationCmd
  char    *slotname;
  TimeLineID timeline;
  XLogRecPtr startpoint;
+ char    *shutdownmode;
  List    *options;
 } StartReplicationCmd;

IMO I those the last 2 members should have a comment something like:
/* Only for logical replication */

because that will make it more clear why sometimes they are assigned
and sometimes they are not.

======
src/include/replication/walreceiver.h

6.
Should the protocol version be bumped (and documented) now that the
START REPLICATION supports a new extended syntax? Or is that done only
for messages sent by pgoutput?

------
Kind Regards,
Peter Smith.
Fujitsu Australia.

  59. Re: Exit walsender before confirming remote flush in logical replication

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2023-02-14T01:05:40Z

    At Mon, 13 Feb 2023 08:27:01 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in 
> On Mon, Feb 13, 2023 at 7:26 AM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> >
> > IMHO I vaguely don't like that we lose a means to specify the default
> > behavior here. And I'm not sure we definitely don't need other than
> > flush and immedaite for both physical and logical replication.
> >
> 
> If we can think of any use case that requires its extension then it
> makes sense to make it a non-boolean option but otherwise, let's keep
> things simple by having a boolean option.

What do you think about the need for explicitly specifying the
default?  I'm fine with specifying the default using a single word,
such as WAIT_FOR_REMOTE_FLUSH.

> > If it's
> > not the case, I don't object to make it a Boolean.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  60. Re: Exit walsender before confirming remote flush in logical replication

    Andres Freund <andres@anarazel.de> — 2023-02-14T01:13:43Z

    On 2023-02-14 10:05:40 +0900, Kyotaro Horiguchi wrote:
> What do you think about the need for explicitly specifying the
> default?  I'm fine with specifying the default using a single word,
> such as WAIT_FOR_REMOTE_FLUSH.

We obviously shouldn't force the option to be present. Why would we want to
break existing clients unnecessarily?  Without it the behaviour should be
unchanged from today's.

  61. Re: Exit walsender before confirming remote flush in logical replication

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2023-02-15T06:20:59Z

    At Mon, 13 Feb 2023 17:13:43 -0800, Andres Freund <andres@anarazel.de> wrote in 
> On 2023-02-14 10:05:40 +0900, Kyotaro Horiguchi wrote:
> > What do you think about the need for explicitly specifying the
> > default?  I'm fine with specifying the default using a single word,
> > such as WAIT_FOR_REMOTE_FLUSH.
> 
> We obviously shouldn't force the option to be present. Why would we want to
> break existing clients unnecessarily?  Without it the behaviour should be
> unchanged from today's.

I didn't suggest making the option mandatory. I just suggested
providing a way to specify the default value explicitly, like in the
recent commit 746915c686.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  62. Re: Exit walsender before confirming remote flush in logical replication

    Greg Sabino Mullane <htamfids@gmail.com> — 2024-09-17T12:29:56Z

    Thanks for everyone's work on this, I am very interested in it getting into
a release. What is the status of this?

My use case is Patroni - when it needs to do a failover, it shuts down the
primary. However, large transactions can cause it to stay in the "shutting
down" state for a long time, which means your entire HA system is now
non-functional. I like the idea of a new flag. I'll test this out soon if
the original authors want to make a rebased patch. This thread is old, so
if I don't hear back in a bit, I'll create and test a new one myself. :)

Cheers,
Greg

  63. Re: Exit walsender before confirming remote flush in logical replication

    Vitaly Davydov <v.davydov@postgrespro.ru> — 2025-09-26T11:10:02Z

    Dear Greg, All

I'm interested in walsender shutdown mode option as well. Unexpected waiting for
wal sender shutdown in fast mode creates some problems to follow a SLA. I also
propose to create a GUC which can be used to set the shutdown mode globally.
Once, the original author was not responded for some time, I like the idea to
create a new separate patch for walsender shutdown mode based on the work in
this thread. I'm ready to participate in patch preparation and testing.

With best regards,
Vitaly

On Tuesday, September 17, 2024 15:29 MSK, Greg Sabino Mullane <htamfids@gmail.com> wrote:

> Thanks for everyone's work on this, I am very interested in it getting into
> a release. What is the status of this?
> 
> My use case is Patroni - when it needs to do a failover, it shuts down the
> primary. However, large transactions can cause it to stay in the "shutting
> down" state for a long time, which means your entire HA system is now
> non-functional. I like the idea of a new flag. I'll test this out soon if
> the original authors want to make a rebased patch. This thread is old, so
> if I don't hear back in a bit, I'll create and test a new one myself. :)
> 
> Cheers,
> Greg

  64. Re: Exit walsender before confirming remote flush in logical replication

    Andrey Silitskiy <a.silitskiy@postgrespro.ru> — 2025-11-18T10:32:01Z

    Dear pgsql-hackers,

I am also interested in solving this problem, so I suggest a patch which
is based on Hayato's work shared earlier.

The problem we are solving is that the logical walsender processes currently
do not allow postgres to shut down until receiver side confirms the flush of
all data. In case of logical replication, this is not necessary. This 
can lead
to an undesirable shutdown delay if, for example, apply worker is 
waiting for
any locks to be released.

I agree with the opinion that the default behavior of the system should 
not be
changed, as some clients may rely on the current behavior. But instead of
the START_REPLICATION parameter I propose a GUC parameter on the sender that
controls the walsender shutdown mode for all logical walsenders.the First,
the START_REPLICATION parameter places responsibility for choosing the 
sender’s
shutdown semantics on the receiver side. Second, per-subscriber settings 
do not
solve the problematic operational case where many walsenders exist: if 
even one
of N walsender processes remains configured non-immediate, the publisher can
still be blocked. In other words, setting immediate for most subscribers but
missing one does not fix the global inability to shut down.

I also attach a tap test that reproduces the apply-worker's waiting for the
release of lock and the successful shutdown of publisher in immediate 
walsender
shutdown mode.

Best Regards,
Andrey

  65. Re: Exit walsender before confirming remote flush in logical replication

    Fujii Masao <masao.fujii@gmail.com> — 2025-11-19T11:46:25Z

    On Tue, Nov 18, 2025 at 7:32 PM Andrey Silitskiy
<a.silitskiy@postgrespro.ru> wrote:
>
> Dear pgsql-hackers,
>
> I am also interested in solving this problem, so I suggest a patch which
> is based on Hayato's work shared earlier.

+1
Thanks for the patch!

+{ name => 'logical_wal_sender_shutdown_mode', type => 'enum', context
=> 'PGC_SIGHUP', group => 'REPLICATION_SENDING',

How about using PGC_USERSET instead of PGC_SIGHUP, similar to
wal_sender_timeout?
That would allow setting logical_wal_sender_shutdown_mode per walsender
by assigning it to the logical replication user on the publisher and specifying
that user in the CONNECTION clause of CREATE SUBSCRIPTION command. For example:

    # publisher
    =# ALTER ROLE testuser SET logical_wal_sender_shutdown_mode TO 'immediate';

    # subscriber
    =# CREATE SUBSCRIPTION ... CONNECTION '... user=testuser' ...;

Even if the publisher's postgresql.conf sets logical_wal_sender_shutdown_mode to
'wait_flush', the per-role setting would take effect for that connection.
This gives users per-connection control, just like with parameters such as
wal_sender_timeout.

Also if the patch I proposed in [1] is committed, the same per-connection
control could be done directly via CREATE SUBSCRIPTION:

    # subscriber
    =# CREATE SUBSCRIPTOIN ... CONNECTION '... options=''-c
logical_wal_sender_shutdown_mode=immediate'''


+        Specifies the mode in which logical walsender process will terminate
+        after receival of shutdown request. Valid values are
+        <literal>wait_flush</literal> and <literal>immediate</literal>.
+        Default value is <literal>wait_flush</literal>.

Shouldn't physical replication walsenders also honor this parameter?
For example, the immediate mode seems useful for physical walsenders connected
from a very remote standby (e.g., DR site). Thought?

Regards,

[1] https://postgr.es/m/CAHGQGwGYV+-abbKwdrM2UHUe-JYOFWmsrs6=QicyJO-j+-Widw@mail.gmail.com

-- 
Fujii Masao

  66. Re: Exit walsender before confirming remote flush in logical replication

    Andrey Silitskiy <a.silitskiy@postgrespro.ru> — 2025-11-20T07:05:05Z

    On Wed, Nov 19, 2025 at 8:46 PM Fujii Masao
<masao(dot)fujii(at)gmail(dot)com> wrote:
 > How about using PGC_USERSET instead of PGC_SIGHUP, similar to
 > wal_sender_timeout?

Dear Fujii, thanks for the review!

Current version of the patch suggests changing the shutdown mode of
logical senders globally for the server. As I wrote above: patch
excludes receiver's side decision whether the sender is allowed to hang
on shutdown. In addition, it provides simpler administration of a system.
But I'm ready to hear other opinions on this matter.

 > Shouldn't physical replication walsenders also honor this parameter?
 > For example, the immediate mode seems useful for physical walsenders 
connected
 > from a very remote standby (e.g., DR site). Thought?

As discussed earlier, physical replication is more sensitive to data
divergence and there is no problem with apply_worker and backend lock
conflict, which makes the use-case more narrow.

By the way, does anyone find the name of IMMEDIATE mode too similar to
the "pg_ctl stop" mode and a little confusing? Initially, I planned
to call this mode WALSND_SHUTDOWN_MODE_FORCED instead of
WALSND_SHUTDOWN_MODE_IMMEDIATE.

Best Regards,
Andrey Silitskiy

  67. Re: Exit walsender before confirming remote flush in logical replication

    Fujii Masao <masao.fujii@gmail.com> — 2025-11-23T14:46:24Z

    On Thu, Nov 20, 2025 at 4:05 PM Andrey Silitskiy
<a.silitskiy@postgrespro.ru> wrote:
>
> On Wed, Nov 19, 2025 at 8:46 PM Fujii Masao
> <masao(dot)fujii(at)gmail(dot)com> wrote:
>  > How about using PGC_USERSET instead of PGC_SIGHUP, similar to
>  > wal_sender_timeout?
>
> Dear Fujii, thanks for the review!
>
> Current version of the patch suggests changing the shutdown mode of
> logical senders globally for the server. As I wrote above: patch
> excludes receiver's side decision whether the sender is allowed to hang
> on shutdown. In addition, it provides simpler administration of a system.

Even with PGC_USERSET instead of PGC_SIGHUP, we can still control
the shutdown mode globally by setting it in postgresql.conf. The difference
is that PGC_USERSET also allows per–replication-user overrides when needed,
which gives users more flexibility without losing the ability to
set a server-wide setting, I think.


> As discussed earlier, physical replication is more sensitive to data
> divergence and there is no problem with apply_worker and backend lock
> conflict, which makes the use-case more narrow.

I think there are valid use cases for applying this setting to
physical replication as well. For example, please consider a system
that has generated a large amount of WAL due to bulk loading,
and a remote standby with a slow or low-bandwidth network link.
In such a case, some would think an immediate shutdown could be desirable
rather than waiting a long time for all outstanding WAL to be sent.

Of course, misconfiguring this parameter for physical replication could
lead to serious issues. So if we decide to apply it to physical walsenders,
the docs might need to clearly explain the risks so that users can make
informed decisions, like we've already done for other parameters like fsync,
full_page_writes, etc.

Regards,

-- 
Fujii Masao

  68. Re: Exit walsender before confirming remote flush in logical replication

    Andrey Silitskiy <a.silitskiy@postgrespro.ru> — 2025-11-27T10:19:13Z

    On Nov 23, 2025 at 11:46 PM Fujii Masao
<masao(dot)fujii(at)gmail(dot)com> wrote:
 > The difference is that PGC_USERSET also allows per–replication-user
 > overrides when needed, which gives users more flexibility without
 > losing the ability to set a server-wide setting, I think.
 > ...
 > I think there are valid use cases for applying this setting to
 > physical replication as well.
Thanks for the comments. I agree, this parameter also seems usable
for physical replication, if you use it with caution. In this case,
it really becomes useful to be able to configure a parameter for
each connection. I have added these changes to my patch.

Also, earlier I did not mention another difference between my patch
and those discussed earlier. Previously, even in immediate mode,
WalSndCaughtUp flag was checked before calling WalSndDone,
and this made it impossible to shut down even in immediate mode
with WalSndCaughtUp = false when the server has full output buffers.
This does not happen in the current patch implementation. I added
an additional test case for this situation.

Regards,
Andrey Silitskiy

  69. Re: Exit walsender before confirming remote flush in logical replication

    Alexander Korotkov <aekorotkov@gmail.com> — 2026-01-03T00:31:51Z

    Hi, Andrey!

On Thu, Nov 27, 2025 at 12:19 PM Andrey Silitskiy
<a.silitskiy@postgrespro.ru> wrote:
> On Nov 23, 2025 at 11:46 PM Fujii Masao
> <masao(dot)fujii(at)gmail(dot)com> wrote:
>  > The difference is that PGC_USERSET also allows per–replication-user
>  > overrides when needed, which gives users more flexibility without
>  > losing the ability to set a server-wide setting, I think.
>  > ...
>  > I think there are valid use cases for applying this setting to
>  > physical replication as well.
> Thanks for the comments. I agree, this parameter also seems usable
> for physical replication, if you use it with caution. In this case,
> it really becomes useful to be able to configure a parameter for
> each connection. I have added these changes to my patch.
>
> Also, earlier I did not mention another difference between my patch
> and those discussed earlier. Previously, even in immediate mode,
> WalSndCaughtUp flag was checked before calling WalSndDone,
> and this made it impossible to shut down even in immediate mode
> with WalSndCaughtUp = false when the server has full output buffers.
> This does not happen in the current patch implementation. I added
> an additional test case for this situation.

Thank you for reviving this thread.  I think it is reasonable to move
control over the walsender shutdown behavior to the primary server.  I
see an analogy with synchronous_commit and synchronous_standby_names.
Primary decides which standbys wait and which way to wait for them.
Similarly, the primary should decide who to wait on the shutdown.

I would like to make a couple of suggestions for the patch.
1) I think it's useful to tune particular standbys/subscribers to
specify the walsender shutdown mode.  It was possible in the patch by
Hayato Kuroda, and it would be a pity to lose.  I suggest implementing
the walsender shutdown mode as a replication slot option.
2) Given that walsender shutdown mode would be a replication slot
option, I propose to rename GUC to default_wal_sender_shutdown_mode.
Also, given we would more likely need to wait for a flush during
streaming replication, I would suggest following modes: immediate,
wait_for_flush_streaming_only, wait_for_flush.  The new intermediate
option would make walsender wait for a flush only for physical
standbys but not for logical subscribers.

What do you think?

------
Regards,
Alexander Korotkov
Supabase

  70. Re: Exit walsender before confirming remote flush in logical replication

    Fujii Masao <masao.fujii@gmail.com> — 2026-01-09T07:47:23Z

    On Sat, Jan 3, 2026 at 9:32 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> Hi, Andrey!
>
> On Thu, Nov 27, 2025 at 12:19 PM Andrey Silitskiy
> <a.silitskiy@postgrespro.ru> wrote:
> > On Nov 23, 2025 at 11:46 PM Fujii Masao
> > <masao(dot)fujii(at)gmail(dot)com> wrote:
> >  > The difference is that PGC_USERSET also allows per–replication-user
> >  > overrides when needed, which gives users more flexibility without
> >  > losing the ability to set a server-wide setting, I think.
> >  > ...
> >  > I think there are valid use cases for applying this setting to
> >  > physical replication as well.
> > Thanks for the comments. I agree, this parameter also seems usable
> > for physical replication, if you use it with caution. In this case,
> > it really becomes useful to be able to configure a parameter for
> > each connection. I have added these changes to my patch.
> >
> > Also, earlier I did not mention another difference between my patch
> > and those discussed earlier. Previously, even in immediate mode,
> > WalSndCaughtUp flag was checked before calling WalSndDone,
> > and this made it impossible to shut down even in immediate mode
> > with WalSndCaughtUp = false when the server has full output buffers.
> > This does not happen in the current patch implementation. I added
> > an additional test case for this situation.
>
> Thank you for reviving this thread.  I think it is reasonable to move
> control over the walsender shutdown behavior to the primary server.  I
> see an analogy with synchronous_commit and synchronous_standby_names.
> Primary decides which standbys wait and which way to wait for them.
> Similarly, the primary should decide who to wait on the shutdown.
>
> I would like to make a couple of suggestions for the patch.
> 1) I think it's useful to tune particular standbys/subscribers to
> specify the walsender shutdown mode.  It was possible in the patch by
> Hayato Kuroda, and it would be a pity to lose.  I suggest implementing
> the walsender shutdown mode as a replication slot option.

Even with the proposed patch, this can already be done by setting
wal_sender_shutdown_mode in primary_conninfo for physical
replication, or in the CONNECTION clause of CREATE SUBSCRIPTION for
logical replication. For example:

    CREATE SUBSCRIPTION ... CONNECTION 'options=''-c
wal_sender_shutdown_mode=immediate''' ...

This allows wal_sender_shutdown_mode in postgresql.conf on the
primary or publisher to act as the default, while different values can
be specified per replication connection via primary_conninfo or the
CONNECTION clause. Thought?

Regards,

-- 
Fujii Masao

  71. Re: Exit walsender before confirming remote flush in logical replication

    Fujii Masao <masao.fujii@gmail.com> — 2026-01-09T08:04:42Z

    On Thu, Nov 27, 2025 at 7:19 PM Andrey Silitskiy
<a.silitskiy@postgrespro.ru> wrote:
>
> On Nov 23, 2025 at 11:46 PM Fujii Masao
> <masao(dot)fujii(at)gmail(dot)com> wrote:
>  > The difference is that PGC_USERSET also allows per–replication-user
>  > overrides when needed, which gives users more flexibility without
>  > losing the ability to set a server-wide setting, I think.
>  > ...
>  > I think there are valid use cases for applying this setting to
>  > physical replication as well.
> Thanks for the comments. I agree, this parameter also seems usable
> for physical replication, if you use it with caution. In this case,
> it really becomes useful to be able to configure a parameter for
> each connection. I have added these changes to my patch.

Thanks for updating the patch!

+ /* Try to inform receiver that XLOG streaming is done */
+ SetQueryCompletion(&qc, CMDTAG_COPY, 0);
+ EndCommand(&qc, DestRemote, false);
+
+ /*
+ * Note that the output buffer may be full during immediate shutdown of
+ * walsender. If pq_flush() is called at that time, the walsender process
+ * will be stuck. Therefore, call pq_flush_if_writable() instead. Successfull
+ * receival of done message in immediate shutdown mode is not guaranteed.
+ */
+ pq_flush_if_writable();

Why do we need to send a "done" message to the receiver here?
Since delivery isn't guaranteed in immediate mode, it seems of limited value.

If it isn't necessary, do we need WalSndDoneImmediate() at all,
or could we just reuse WalSndShutdown() for immediate mode?


For the immediate mode, would it make sense to log that the walsender is
terminating in immediate mode and that WAL replication may be incomplete,
so users can more easily understand what happened?

Regards,

-- 
Fujii Masao

  72. Re: Exit walsender before confirming remote flush in logical replication

    Andrey Silitskiy <a.silitskiy@postgrespro.ru> — 2026-01-15T07:06:49Z

    Hi, Alexander! Thanks for your comments!

On Jan 3, 2026 at 2:32 AM  Alexander Korotkov
<aekorotkov(at)gmail(dot)com> wrote:
 > I think it is reasonable to move control over the walsender
 > shutdown behavior to the primary server.  I see an analogy with
 > synchronous_commit and synchronous_standby_names. Primary decides
 > which standbys wait and which way to wait for them. Similarly,
 > the primary should decide who to wait on the shutdown.

With the current patch, the walsender process termination mode is set
on the primary server using a GUC variable, and clients who do not want
to wait for a data flush to any of the replicas can configure this
parameter for specific replicas (wal_sender_timeout is currently working
in a similar way, which was also discussed in the thread [1]).

 > I propose to rename GUC to default_wal_sender_shutdown_mode.
 > Also, given we would more likely need to wait for a flush during
 > streaming replication, I would suggest following modes: immediate,
 > wait_for_flush_streaming_only, wait_for_flush.  The new intermediate
 > option would make walsender wait for a flush only for physical
 > standbys but not for logical subscribers.

Currently, in this patch, it is also possible to configure the server
so that it waits only for physical replicas. The administrator is given
the opportunity to flexibly change the default wait_flush mode for each
replica individually. In this case, you do not need to add this feature
inside modes of the proposed GUC default_wal_sender_shutdown_mode, but
you can save place for a potential semantically new mode besides wait_flush
and immediate (for example, waiting for the flush only for a certain period
of time, and then terminating the process).

[1]: 
https://www.postgresql.org/message-id/flat/0A3221C70F24FB45833433255569204D1FAAD3AE%40G01JPEXMBYT05

  73. RE: Exit walsender before confirming remote flush in logical replication

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2026-01-15T09:48:35Z

    Dear Andrey,

Sorry for being late I've missed the thread. I read the patch but the test may
contain some issues.

1.
Please update meson.build, otherwise it won't be run by Meson/Ninja builds.

2.
Missing Copyright, please add below at the top of the file.

```
# Copyright (c) 2026, PostgreSQL Global Development Group
```

3.
```
# run concurrent transaction on publisher and commit
$out = $publisher->safe_psql('postgres', 'BEGIN; INSERT INTO pub_test VALUES (0); COMMIT;');
ok($out eq "", "Concurrent transaction was committed on publisher");
```

IIUC, safe_psql() ensures the command is succeeded. Isn't it enough to use below?

```
$publisher->safe_psql('postgres', 'INSERT INTO pub_test VALUES (0);');
```

4.
```
# test publisher shutdown
ok_with_timeout(5, sub { $publisher->stop('fast') },
                "Successfull fast shutdown of server with empty output buffers");
```

I think we can just use stop() because it internally runs `pg_ctl stop` and
that command waits till the wait is finished by default. I feel it is dangerous
to determine timeout to 5sec because the test can work on very poor environment.


This means `run_with_timeout`, `ok_with_timeout` are not needed anymore.

5.
```
$subscriber->wait_for_subscription_sync($publisher, 'sub_all');
```

I think this at line 114 should be wait_for_catchup().


6.
```
# generate big amount of wal records for locked table
$out = $publisher->safe_psql('postgres', 'BEGIN; INSERT INTO pub_test SELECT i from generate_series(1, 20000) s(i); COMMIT;');
ok($out eq "", "Inserts into locked table successfully generated");
```

Same thing can be said as 3. This means `$out` may not be needed anymore.

Also, not sure, how can we ensure the buffer is full here? Also, even if we have
the way to check, the size may be quite platform depending.
I think it may be better to test both streaming and logical replication instead
of testing empty/full output buffer. Thought?
(In this case the test can be put in recovery/ subdir)

Best regards,
Hayato Kuroda
FUJITSU LIMITED

  74. Re: Exit walsender before confirming remote flush in logical replication

    Andrey Silitskiy <a.silitskiy@postgrespro.ru> — 2026-01-17T15:46:24Z

    Dear Hayato,
Thanks for your comments! Updated the patch.

On Jan 15, 2026 at 11:48 AM  Hayato Kuroda
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
 > I think we can just use stop() because it internally runs `pg_ctl stop`
 > and that command waits till the wait is finished by default. I feel it
 > is dangerous to determine timeout to 5sec because the test can work on
 > very poor environment.

ok_with_timeout was added because it allows to output a more reasonable
log in case of a problem from this thread: "Failed test 'Successful fast
shutdown of server with empty output buffers (timed out after 5 seconds)'"
instead of the usual "pg_ctl stop failed".  But now I noticed that the
standard timeout  is triggered earlier, and when setting a timeout in this
function greater than the standard PGCTLTIMEOUT, only "pg_ctl stop failed"
will be written. Perhaps it is reasonable to remove these functions.

 > Also, not sure, how can we ensure the buffer is full here? Also, even
 > if we have the way to check, the size may be quite platform depending.
 > I think it may be better to test both streaming and logical replication
 > instead of testing empty/full output buffer. Thought?

Initially, a second test case was added to show that previous patches
did not fix the problem of hanging in case of full buffers. I agree that
it may depend on the platform, but I can't think of a way to guarantee this,
even though the test case seems useful for checking the new mode.
Test contains only the case of logical replication, since so far I'm not
sure how to reproduce guaranteed flush delay on a physical replica in the
test. Any ideas?

Regards,
Andrey Silitskiy

  75. Re: Exit walsender before confirming remote flush in logical replication

    Andrey Silitskiy <a.silitskiy@postgrespro.ru> — 2026-01-17T16:20:46Z

    On Jan 9, 2026 at 10:04 AM Fujii Masao
<masao(dot)fujii(at)gmail(dot)com> wrote:
 > Why do we need to send a "done" message to the receiver here?
 > Since delivery isn't guaranteed in immediate mode, it seems of limited
 > value.

It seems to me that it is better to send a message in cases where it is
possible, soas notto raiseerrorsonthe subscriberduringa cleanshutdown.
And when this is not possible, exit the process without waiting.

 > For the immediate mode, would it make sense to log that the walsender is
 > terminating in immediate mode and that WAL replication may be incomplete,
 > so users can more easily understand what happened?

Added to the latest patch.

Regards,
Andrey Silitskiy

  76. Re: Exit walsender before confirming remote flush in logical replication

    Fujii Masao <masao.fujii@gmail.com> — 2026-01-19T12:41:32Z

    On Sun, Jan 18, 2026 at 1:20 AM Andrey Silitskiy
<a.silitskiy@postgrespro.ru> wrote:
>
> On Jan 9, 2026 at 10:04 AM Fujii Masao
> <masao(dot)fujii(at)gmail(dot)com> wrote:
> > Why do we need to send a "done" message to the receiver here?
> > Since delivery isn't guaranteed in immediate mode, it seems of limited
> > value.
>
> It seems to me that it is better to send a message in cases where it is
> possible, so as not to raise errors on the subscriber during a clean shutdown.
> And when this is not possible, exit the process without waiting.
>
> > For the immediate mode, would it make sense to log that the walsender is
> > terminating in immediate mode and that WAL replication may be incomplete,
> > so users can more easily understand what happened?
>
> Added to the latest patch.

Thanks for updating the patch!

cfbot is reporting a test failure. Could you please look into it and
fix the issue?
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F6234

Regards,

-- 
Fujii Masao

  77. Re: Exit walsender before confirming remote flush in logical replication

    Vitaly Davydov <v.davydov@postgrespro.ru> — 2026-01-20T17:03:55Z

    Dear Hackers,

I think, I reproduced test fails. The test fails because walsender is in
waiting state in WalSndDoneImmediate -> ereport with the following stack (see
below). It seems, it tries to send the message to the replica and flush it, but
the replica is hung.

#0  0x00007a4b37f2a037 in epoll_wait
#1  0x000056855317a2e8 in WaitEventSetWaitBlock
#2  WaitEventSetWait
#3  0x0000568552feea8e in secure_write
#4  0x0000568552ff5666 in internal_flush_buffer
#5  0x0000568552ff5966 in internal_flush
#6  socket_flush ()
#7  socket_flush ()
#8  0x00005685532ff1b3 in send_message_to_frontend (edata=<optimized out>)
#9  EmitErrorReport ()
#10 0x00005685532ff6dd in errfinish
#11 0x000056855312cc9c in WalSndDoneImmediate () at walsender.c:3625

I would propose to remove the ereport call from WalSndDoneImmediate.

With best regards,
Vitaly

On 1/19/26 15:41, Fujii Masao wrote:
> On Sun, Jan 18, 2026 at 1:20 AM Andrey Silitskiy
> <a.silitskiy@postgrespro.ru> wrote:
>>
>> On Jan 9, 2026 at 10:04 AM Fujii Masao
>> <masao(dot)fujii(at)gmail(dot)com> wrote:
>>> Why do we need to send a "done" message to the receiver here?
>>> Since delivery isn't guaranteed in immediate mode, it seems of limited
>>> value.
>>
>> It seems to me that it is better to send a message in cases where it is
>> possible, so as not to raise errors on the subscriber during a clean shutdown.
>> And when this is not possible, exit the process without waiting.
>>
>>> For the immediate mode, would it make sense to log that the walsender is
>>> terminating in immediate mode and that WAL replication may be incomplete,
>>> so users can more easily understand what happened?
>>
>> Added to the latest patch.
> 
> Thanks for updating the patch!
> 
> cfbot is reporting a test failure. Could you please look into it and
> fix the issue?
> https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F6234
> 
> Regards,
>

  78. Re: Exit walsender before confirming remote flush in logical replication

    Andrey Silitskiy <a.silitskiy@postgrespro.ru> — 2026-01-21T15:11:47Z

    Dear Vitaliy,
Thanks for reproducing the error.

The problem occurred during a test case with a full output buffer after
adding ereport about the termination of walsender process in immediate mode.
ereport sends message by default if replica is interested in this logging
level(WARNING level fit this description). And since the output buffer was
already full, the message could not be sent and the process could not exit.

Changed the logging level to LOG_SERVER_ONLY to exclude sending a message
to the replica during immediate shutdown.

In my environment, this error was not reproduced due to the fact that
buffers did not have time to fill up because pg_ctl stop was called
immediately after data was inserted and the buffers did not have time to
fill up and ereport did not hang.

I added a wait for sent_lsn from pg_stat_replication to stop advancing
in the test before server shutdown to give walsender time to fill output
buffers.

Regards,
Andrey Silitskiy

  79. Re: Exit walsender before confirming remote flush in logical replication

    Fujii Masao <masao.fujii@gmail.com> — 2026-01-26T14:08:39Z

    On Thu, Jan 22, 2026 at 12:11 AM Andrey Silitskiy
<a.silitskiy@postgrespro.ru> wrote:
>
> Dear Vitaliy,
> Thanks for reproducing the error.
>
> The problem occurred during a test case with a full output buffer after
> adding ereport about the termination of walsender process in immediate mode.
> ereport sends message by default if replica is interested in this logging
> level(WARNING level fit this description). And since the output buffer was
> already full, the message could not be sent and the process could not exit.
>
> Changed the logging level to LOG_SERVER_ONLY to exclude sending a message
> to the replica during immediate shutdown.

You changed the log level used by ereport() in WalSndDoneImmediate() to
LOG_SERVER_ONLY, but proc_exit() can still emit messages at other levels.
That could result in attempts to send additional messages to the standby.
To avoid this, should whereToSendOutput be reset, as is done
in WalSndShutdown()?


> In my environment, this error was not reproduced due to the fact that
> buffers did not have time to fill up because pg_ctl stop was called
> immediately after data was inserted and the buffers did not have time to
> fill up and ereport did not hang.
>
> I added a wait for sent_lsn from pg_stat_replication to stop advancing
> in the test before server shutdown to give walsender time to fill output
> buffers.

Thanks for addressing the issue and updating the patch!


There seems also an alternative way to implement immediate shutdown of
walsenders: if wal_sender_shutdown_mode is set to immediate, the postmaster
could send SIGTERM to walsenders, just as it does to backends. In that case,
walsenders would exit without waiting for WAL replication, and the shutdown
sequence could proceed normally. With this approach, WalSndDoneImmediate()
would no longer be needed. I'm still unsure which approach is better....

One issue with the SIGTERM-based approach is that walsenders call
ereport(FATAL) on exit and may try to send the error message to the standby.
If the send buffer is full, the walsender could block in ereport(). This would
need to be addressed if we go that route.

BTW, this issue can also occur when terminating a walsender via
pg_terminate_backend() (for maintenance, etc.). If that is considered
problematic on its own, it might be better handled as a separate patch first.


Regarding the GUC name, wal_sender_shutdown seems simple and sufficient to me.
This isn't a blocker, so I'm fine with the current name for now and
revisiting it later if needed.


On the documentation, there's at least one reference to shutdown behavior
in high-availability.sgml. Since this patch changes walsender shutdown behavior,
that section should be updated as well.


+        In <literal>immediate</literal> mode, the walsender will exit
without waiting
+        for data replication to the receiver. This may break data
consistency between
+        sender and receiver after shutdown, which can be especially
important in
+        case of physical replication and switch-over.

It seems clearer to clarify the benefit of wait_flush in relation to
switchovers, rather than focusing only risks. For example:

----------------------
Specifies how a walsender process terminates after receiving a shutdown
request. Valid values are wait_flush and immediate. The default is wait_flush.
This setting can be configured per walsender.

In wait_flush mode, the walsender waits until all WAL data has been flushed on
the receiver before exiting. This helps keep the sender and receiver in sync
after shutdown, which is especially important for physical replication
switchovers. However, it can delay server shutdown.

In immediate mode, the walsender exits without waiting for WAL data to be
replicated to the receiver. This can reduce shutdown time when flushing
WAL data to the receiver would take a long time, for example on high-latency
networks or when the subscriber's apply worker is blocked waiting for locks
in logical replication.
----------------------


There's no strict rule for the ordering of parameter descriptions, but it seems
more intuitive to place wal_sender_shutdown_mode immediately after
wal_sender_timeout, rather than after track_commit_timestamp,
since the former two are more closely related. If we do that,
postgresql.conf.sample should be updated accordingly.


+#wal_sender_shutdown_mode = wait_flush  # walsender termination mode after
+                                # receival of shutdown request

In postgresql.conf.sample, enum GUCs typically list their valid values in
the comment. It would be good to do the same for wal_sender_shutdown_mode.


At the top of walsender.c, there are comments describing walsender behavior,
including shutdown handling. Since this patch changes shutdown behavior,
those comments should be updated.


+typedef enum
+{
+ WALSND_SHUTDOWN_MODE_WAIT_FLUSH = 0,
+ WALSND_SHUTDOWN_MODE_IMMEDIATE
+} WalSndShutdownMode;

WalSndShutdownMode should be added into src/tools/pgindent/typedefs.list
for pgindent.


+ * NB: This should only be called when immediate shutdown of walsender
+ * was requested and shutdown signal has been received from postmaster.

The comment on WalSndDoneImmediate() says it is only called after a shutdown
signal from the postmaster, but it can also be called when the checkpointer
signals walsenders to move to the stopping state (got_STOPPING = true),
which may happen earlier. So this comment seems to need to be updated.
Or, if this comment is true, WalSndDoneImmediate() should not be called under
got_STOPPING=true.


+ proc_exit(0);
+ abort();
+}

abort() call at the end of WalSndDoneImmediate() does not seem necessary.
Please see the discussion [1].

Regards,

[1] https://postgr.es/m/CAHGQGwHPX1yoixq+YB5rF4zL90TMmSEa3FpHURtqW3Jc5+=oSA@mail.gmail.com

Regards,

-- 
Fujii Masao

  80. Re: Exit walsender before confirming remote flush in logical replication

    Ronan Dunklau <ronan@dunklau.fr> — 2026-01-28T08:27:28Z

    On Monday, January 26th, 2026 at 15:08, Fujii Masao <masao.fujii@gmail.com> wrote:
 
> Regarding the GUC name, wal_sender_shutdown seems simple and sufficient to me.
> This isn't a blocker, so I'm fine with the current name for now and
> revisiting it later if needed.

Are we considering other approaches to this ? Having the behavior be either "wait indefinitely" or "terminate immediately" is a bit coarse I think: a timeout for the wait (maybe named wal_sender_stop_timeout ?) would allow for the same usage as this patch provides (set it to -1 for indefinite wait, 0 for immediate shutdown, or any positive value to give a chance to the walsender to catch up before we terminate it forcibly).

The problem we have as of now is when the walreceiver is indeed connected and not reaching wal_sender_timeout as it's still processing: a distinct timeout would alleviate that.

Regards,

--
Ronan Dunklau

  81. Re: Exit walsender before confirming remote flush in logical replication

    Andrey Silitskiy <a.silitskiy@postgrespro.ru> — 2026-01-28T11:27:14Z

    Thanks for the review!

Updatedthe patchbasedonyourcomments. Regards, Andrey Silitskiy

  82. Re: Exit walsender before confirming remote flush in logical replication

    Andrey Silitskiy <a.silitskiy@postgrespro.ru> — 2026-01-28T11:33:31Z

    Dear Ronan,
Thanks for participating in the discussion.

On Jan 28, 2026 Ronan Dunklau <ronan(at)dunklau(dot)fr> wrote:
 > ... a timeout for the wait (maybe named wal_sender_stop_timeout ?) would
 > allow for the same usage ...

Sounds like an option.
This is also possible in the current implementation, but your option offers
a simpler interface if we do not plan to add new wallsender completion 
modes.
The naming of the parameter is also a question, because wal_sender_timeout
already exists (which also fits the name wal_sender_stop_timeout quite 
well).
The difference between these parameters may not be obvious to users.
Thoughts?

Regards,
Andrey Silitskiy

  83. Re: Exit walsender before confirming remote flush in logical replication

    Andrey Silitskiy <a.silitskiy@postgrespro.ru> — 2026-02-01T19:52:36Z

     > Thanks for the review!
 > Updated the patch based on your comments.Fixed cfbot documentation 
error. Regards, Andrey Silitskiy

  84. Re: Exit walsender before confirming remote flush in logical replication

    Michael Paquier <michael@paquier.xyz> — 2026-02-02T07:55:59Z

    On Mon, Feb 02, 2026 at 02:52:36AM +0700, Andrey Silitskiy wrote:
> +/*
> + * Shutdown walsender in immediate mode.
> + */
> +static void
> +WalSndDoneImmediate()
> +{
> +	QueryCompletion qc;
> +
> +	/* Try to inform receiver that XLOG streaming is done */
> +	SetQueryCompletion(&qc, CMDTAG_COPY, 0);
> +	EndCommand(&qc, DestRemote, false);

Couldn't that be potentially dangerous, particularly if
wal_sender_shutdown_mode is set to immediate, meaning that it applies
to all the WAL senders?  The WAL receiver side could be on a backend
with an older backend version than the WAL sender where this new GUC
exists.  Hence, a completion could be understood incorrectly by a WAL
receiver depending on how the receiving side is coded?  Assuming this
is merged into v19 in this shape, a receiver connecting to a newer
server would get a new bevahior compared to older versions, without
the receiver being aware of that.

I suspect that this option, as designed, is potentially a footgun that 
could surprise many users, especially as it is super aggressive in
shutting down everything on sight, unconditionally.
--
Michael

  85. Re: Exit walsender before confirming remote flush in logical replication

    Fujii Masao <masao.fujii@gmail.com> — 2026-02-02T11:58:00Z

    On Mon, Feb 2, 2026 at 4:56 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Feb 02, 2026 at 02:52:36AM +0700, Andrey Silitskiy wrote:
> > +/*
> > + * Shutdown walsender in immediate mode.
> > + */
> > +static void
> > +WalSndDoneImmediate()
> > +{
> > +     QueryCompletion qc;
> > +
> > +     /* Try to inform receiver that XLOG streaming is done */
> > +     SetQueryCompletion(&qc, CMDTAG_COPY, 0);
> > +     EndCommand(&qc, DestRemote, false);
>
> Couldn't that be potentially dangerous, particularly if
> wal_sender_shutdown_mode is set to immediate, meaning that it applies
> to all the WAL senders?  The WAL receiver side could be on a backend
> with an older backend version than the WAL sender where this new GUC
> exists.  Hence, a completion could be understood incorrectly by a WAL
> receiver depending on how the receiving side is coded?  Assuming this
> is merged into v19 in this shape, a receiver connecting to a newer
> server would get a new bevahior compared to older versions, without
> the receiver being aware of that.
>
> I suspect that this option, as designed, is potentially a footgun that
> could surprise many users, especially as it is super aggressive in
> shutting down everything on sight, unconditionally.

I’m not sure this is actually problematic when terminating a walsender in
immediate mode while the subscriber is running an older PostgreSQL version.
I haven't been able to come up with any failure scenario yet. However,
if this does turn out to be an issue, it might be better to use the alternative
approach I mentioned in [1].

With that approach, even in immediate mode the walsender simply receives
SIGTERM and exits. This is not new behavior for walsenders, so even when
the subscriber is on an older version, seems it shouldn't risk breaking
the shutdown behavior that such a subscriber is expecting.

Regards,

[1] https://postgr.es/m/CAHGQGwGotoS0VeMDdK6ezkhvdQpWZ5oJvO3QKJKEV6Pc+rZ_9A@mail.gmail.com

-- 
Fujii Masao

  86. Re: Exit walsender before confirming remote flush in logical replication

    Andrey Silitskiy <a.silitskiy@postgrespro.ru> — 2026-02-04T14:03:58Z

    On Mon, Feb 2, 2026 at 4:56 PM Michael Paquier
<michael(at)paquier(dot)xyz> wrote:
 > Couldn't that be potentially dangerous, particularly if
 > wal_sender_shutdown_mode is set to immediate, meaning that it applies
 > to all the WAL senders?  The WAL receiver side could be on a backend
 > with an older backend version than the WAL sender where this new GUC
 > exists.

So far, I can't provide a scenario where the old-version receiving side is
having problems due to the behavior of immediate wal_sender_shutdown_mode.
Maybe someone else can.

Even if walsender immediate shutdown mode is set on the sender, it flushes
a done message to the receiver, as it is being done now. In the case 
when this
is not possible, it terminates in a similar way as it is currently doing
in WalSndShutdown(), the old receiver also seems to be able to handle this
case.

Regards,
Andrey Silitskiy

  87. Re: Exit walsender before confirming remote flush in logical replication

    Ronan Dunklau <ronan@dunklau.fr> — 2026-02-10T09:15:51Z

    Le mercredi 28 janvier 2026 à 12:33, Andrey Silitskiy <a.silitskiy@postgrespro.ru> a écrit :
> This is also possible in the current implementation, but your option offers
> a simpler interface if we do not plan to add new wallsender completion
> modes.
> The naming of the parameter is also a question, because wal_sender_timeout
> already exists (which also fits the name wal_sender_stop_timeout quite
> well).
> The difference between these parameters may not be obvious to users.
> Thoughts?

The naming can probably be revisited but the use case I have in mind
is the following:

- we want to bound the time it takes to perform a restart, or a stop, with walsenders setup.
- the immediate shutdown solves this problem
- however, if one were to stop the service before doing a pg_upgrade, they would
have no other choice than either disable the behaviour entirely (by switching to
wait_flush) or fail the upgrade if a logical replication slot is pending
changes. Switching to wait_flush may be acceptable in that case, but then if the walreceiver
does not catch up in time we're back to agressively stopping postgres to abort
the waiting stop process, and the upgrade entirely.

So in my mind having a timeout makes a lot more sense.

What do you think ?

  88. Re: Exit walsender before confirming remote flush in logical replication

    Andrey Silitskiy <a.silitskiy@postgrespro.ru> — 2026-03-03T07:08:52Z

    On Feb 10, 2026 Ronan Dunklau <ronan(at)dunklau(dot)fr> wrote:
 > - however, if one were to stop the service before doing a pg_upgrade,
 > they would have no other choice than either disable the behaviour
 > entirely (by switching to wait_flush) or fail the upgrade if a logical
 > replication slot is pending changes.

I agree with you, this idea is more flexible. So I changed the interface
from the one discussed in this thread from the very beginning to a timeout.
Propose an updated patch.

Best Regards,
Andrey

  89. Re: Exit walsender before confirming remote flush in logical replication

    Japin Li <japinli@hotmail.com> — 2026-03-04T01:59:20Z

    On Tue, 03 Mar 2026 at 14:08, Andrey Silitskiy <a.silitskiy@postgrespro.ru> wrote:
> On Feb 10, 2026 Ronan Dunklau <ronan(at)dunklau(dot)fr> wrote:
>> - however, if one were to stop the service before doing a pg_upgrade,
>> they would have no other choice than either disable the behaviour
>> entirely (by switching to wait_flush) or fail the upgrade if a logical
>> replication slot is pending changes.
>
> I agree with you, this idea is more flexible. So I changed the interface
> from the one discussed in this thread from the very beginning to a timeout.
> Propose an updated patch.
>
> Best Regards,
> Andrey
>

At first glance, wal_sender_shutdown_timeout seems to have the wrong type.
It should be an integer, not an enum.

+      <term><varname>wal_sender_shutdown_timeout</varname> (<type>enum</type>)

> [2. text/x-patch; v1-0001-Introduce-a-new-GUC-wal_sender_shutdown_timeout.patch]...

-- 
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.

  90. Re: Exit walsender before confirming remote flush in logical replication

    Andrey Silitskiy <a.silitskiy@postgrespro.ru> — 2026-03-04T08:47:47Z

    On Wed, 03 Mar 2026 Japin Li <japinli(at)hotmail(dot)com> wrote:
 > At first glance, wal_sender_shutdown_timeout seems to have the wrong
 > type.

Fixed.

Regards,
Andrey Silitskiy

  91. Re: Exit walsender before confirming remote flush in logical replication

    Alexander Korotkov <aekorotkov@gmail.com> — 2026-03-11T14:24:05Z

    Hi!

On Wed, Mar 4, 2026 at 10:47 AM Andrey Silitskiy
<a.silitskiy@postgrespro.ru> wrote:
> On Wed, 03 Mar 2026 Japin Li <japinli(at)hotmail(dot)com> wrote:
>  > At first glance, wal_sender_shutdown_timeout seems to have the wrong
>  > type.
>
> Fixed.

I've revised this patch fixing grammar in commit message, comments and
documentation.

I think the current patch addresses all the main concerns raised in
the thread.  The patch doesn't unconditionally change the behavior: it
introduces a new GUC, which could be set on per-connection basis, and
also affects physical WAL senders.  The GUC specifies timeout, which
gives user a flexibility.  The default value of the GUC is -1
(disabled).  So, no behavior change by default.  Also, it doesn't
require replication protocol change.  New WalSndDoneImmediate() sends
done message to the receiver just like WalSndDone().  So, existing
clients should be OK.

I'm going to push this if no objections.

------
Regards,
Alexander Korotkov
Supabase

  92. Re: Exit walsender before confirming remote flush in logical replication

    Greg Sabino Mullane <htamfids@gmail.com> — 2026-03-11T21:12:23Z

    Well, since I am listed as a reviewer, I might as well do a real review. :)

>  It is disabled by default. This parameter can be set for each walsender.

Not sure what this means, it seems to imply you can tweak already-created
walsenders. Remove or reword.


> WalSndCheckTimeOut

Should be "Timeout" everywhere (timeout is one word, not two)


> if (!(got_STOPPING || got_SIGUSR2))

I'm not really clear on this - wouldn't we want this only for got_SIGUSR2?


> /* Try to inform receiver that XLOG streaming is done */
> SetQueryCompletion(&qc, CMDTAG_COPY, 0);
> EndCommand(&qc, DestRemote, false);

Is it possible to get here without being in a copy state? I'm worried about
unconditionally sending this.


> (errmsg("walsender shutting down due to wal_sender_shutdown_timeout
expiration"),

Any way to give more context here? Like tell if things were buffered, or
where we were in the stream?

Also more standardly written as "terminating walsender process due to
wal_sender_shutdown_timeout"


> errhint("Replication may be incomplete.")));

Is that a hint? Seems it should be part of the warning.


> +  long_desc => '-1 disables timeout',

Worth a "0 means ..." like some other GUCs do?


> # 0 sets immediate termination of walsender

Better as "0 means immediate termination of walsender"


> WalSndDoneImmediate()

Add (void) to match the declaration

Tests:

* Should test something other than -1 and 0 (e.g. a positive value)
* No checking that the WARNING/HINT is emitted
* s/with wal_sender_shutdown_timeout is set/with
wal_sender_shutdown_timeout set/
* Import PostgreSQL::Test::Utils at the top
* pass('successfull fast shutdown of server with empty output buffers'); <<
are they really empty?


Three places have 'successfull' which should be 'successful'

Two places have 'receival' which should be 'receipt'

> * This waiting time can be limited by wal_sender_shutdown_timeout
parameter.

s/limited by /limited by the /

Cheers,
Greg

  93. Re: Exit walsender before confirming remote flush in logical replication

    Japin Li <japinli@hotmail.com> — 2026-03-12T04:19:07Z

    Hi, Alexander

On Wed, 11 Mar 2026 at 16:24, Alexander Korotkov <aekorotkov@gmail.com> wrote:
> Hi!
>
> On Wed, Mar 4, 2026 at 10:47 AM Andrey Silitskiy
> <a.silitskiy@postgrespro.ru> wrote:
>> On Wed, 03 Mar 2026 Japin Li <japinli(at)hotmail(dot)com> wrote:
>>  > At first glance, wal_sender_shutdown_timeout seems to have the wrong
>>  > type.
>>
>> Fixed.
>
> I've revised this patch fixing grammar in commit message, comments and
> documentation.
>
> I think the current patch addresses all the main concerns raised in
> the thread.  The patch doesn't unconditionally change the behavior: it
> introduces a new GUC, which could be set on per-connection basis, and
> also affects physical WAL senders.  The GUC specifies timeout, which
> gives user a flexibility.  The default value of the GUC is -1
> (disabled).  So, no behavior change by default.  Also, it doesn't
> require replication protocol change.  New WalSndDoneImmediate() sends
> done message to the receiver just like WalSndDone().  So, existing
> clients should be OK.

Thanks for updating the patch.

1.
The shutdown_request_timestamp is used only in WalSndCheckShutdownTimeOut().
Would it make sense to declare it inside this function instead?

2.
+static void
+WalSndDoneImmediate()
+{

We should add `void` to the parameter list here to match the declaration:

>
> I'm going to push this if no objections.
>
> ------
> Regards,
> Alexander Korotkov
> Supabase
>
> [2. application/x-patch; v3-0001-Introduce-a-new-wal_sender_shutdown_timeout-GUC.patch]...

-- 
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.

  94. Re: Exit walsender before confirming remote flush in logical replication

    Fujii Masao <masao.fujii@gmail.com> — 2026-03-13T04:40:19Z

    On Thu, Mar 12, 2026 at 1:21 PM Japin Li <japinli@hotmail.com> wrote:
>
>
> Hi, Alexander
>
> On Wed, 11 Mar 2026 at 16:24, Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > Hi!
> >
> > On Wed, Mar 4, 2026 at 10:47 AM Andrey Silitskiy
> > <a.silitskiy@postgrespro.ru> wrote:
> >> On Wed, 03 Mar 2026 Japin Li <japinli(at)hotmail(dot)com> wrote:
> >>  > At first glance, wal_sender_shutdown_timeout seems to have the wrong
> >>  > type.
> >>
> >> Fixed.
> >
> > I've revised this patch fixing grammar in commit message, comments and
> > documentation.
> >
> > I think the current patch addresses all the main concerns raised in
> > the thread.  The patch doesn't unconditionally change the behavior: it
> > introduces a new GUC, which could be set on per-connection basis, and
> > also affects physical WAL senders.  The GUC specifies timeout, which
> > gives user a flexibility.  The default value of the GUC is -1
> > (disabled).  So, no behavior change by default.  Also, it doesn't
> > require replication protocol change.  New WalSndDoneImmediate() sends
> > done message to the receiver just like WalSndDone().  So, existing
> > clients should be OK.
>
> Thanks for updating the patch.
>
> 1.
> The shutdown_request_timestamp is used only in WalSndCheckShutdownTimeOut().
> Would it make sense to declare it inside this function instead?
>
> 2.
> +static void
> +WalSndDoneImmediate()
> +{
>
> We should add `void` to the parameter list here to match the declaration:
>
> >
> > I'm going to push this if no objections.

Thanks for working on this patch!

I'm not sure introducing a timeout for walsender shutdown is a good idea,
or whether users can easily determine an appropriate timeout value.

With the patch, when I set wal_sender_shutdown_timeout to 1ms and
wal_sender_timeout to 1min, it still took about 26s for walsender to shut down
due to wal_sender_shutdown_timeout expiration in my test. This suggests
the timeout may not be handled correctly.

My test steps were:
1. Set up a logical replication environment.
2. Lock the table in ACCESS EXCLUSIVE mode on the subscriber.
3. Load a large amount of data into the table on the publisher.
   As a result, the logical apply worker cannot receive new data due
to the lock wait, so the data is not replicated and the send buffer
becomes full.
4. Shut down the publisher.

2026-03-13 13:24:24 JST [postmaster] LOG:  received fast shutdown request
2026-03-13 13:24:24 JST [postmaster] LOG:  aborting any active transactions
2026-03-13 13:24:24 JST [postmaster] LOG:  background worker "logical
replication launcher" (PID 34410) exited with exit code 1
2026-03-13 13:24:24 JST [checkpointer] LOG:  shutting down
2026-03-13 13:24:50 JST [walsender] WARNING:  walsender shutting down
due to wal_sender_shutdown_timeout expiration
2026-03-13 13:24:50 JST [walsender] HINT:  Replication may be incomplete.

Regards,

-- 
Fujii Masao

  95. Re: Exit walsender before confirming remote flush in logical replication

    Andrey Silitskiy <a.silitskiy@postgrespro.ru> — 2026-03-13T12:00:40Z

    Thanks for the review! Fixed patch is attached.

On Wed, 11 Mar 2026 Greg Sabino Mullane <htamfids(at)gmail(dot)com> wrote:
 > I'm not really clear on this - wouldn't we want this only for 
got_SIGUSR2?

Сhecking got_SIGUSR2 alone is not enough. If we received got_STOPPING,
got_SIGUSR2 will be set only if WalSndCaughtUp is true. This may fail,
and then the test hangs with the inability to terminate walsender.

 > Tests:
 > * Should test something other than -1 and 0 (e.g. a positive value)

I'm not sure if it's worth adding a test containing a long timeout wait.
I can add a test-case with a small positive parameter value (for example,
200ms). However, I don't think it will be possible to verify the time period
for which the walsender was actually terminated. Still, it will be 
limited to
checking the fact of termination. What do you think?

Regards,
Andrey Silitskiy

  96. Re: Exit walsender before confirming remote flush in logical replication

    Andrey Silitskiy <a.silitskiy@postgrespro.ru> — 2026-03-13T12:14:07Z

    On Fri, 13 Mar 2026 Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
 > With the patch, when I set wal_sender_shutdown_timeout to 1ms and
 > wal_sender_timeout to 1min, it still took about 26s for walsender to shut
 > down due to wal_sender_shutdown_timeout expiration.

Thanks!.

I also reproduced this delay in case of full output buffers. It has already
been fixed in the latest patch.
I think in your case, the 26-second wait was due to walsender falling asleep
at wal_sender_timeout / 2 seconds after receiving the shutdown request.
The issue should have been fixed in the patch (see WalSndComputeSleeptime).

Regards,
Andrey Silitskiy

  97. Re: Exit walsender before confirming remote flush in logical replication

    Greg Sabino Mullane <htamfids@gmail.com> — 2026-03-13T12:24:04Z

    On Fri, Mar 13, 2026 at 8:00 AM Andrey Silitskiy <a.silitskiy@postgrespro.ru>
wrote:

> Сhecking got_SIGUSR2 alone is not enough. If we received got_STOPPING,
> got_SIGUSR2 will be set only if WalSndCaughtUp is true. This may fail, and
> then the test hangs with the inability to terminate walsender.
>

Got it, thanks


>  > Tests:
>  > * Should test something other than -1 and 0 (e.g. a positive value)
>
> I'm not sure if it's worth adding a test containing a long timeout wait. I
> can add a test-case with a small positive parameter value (for example,
> 200ms). However, I don't think it will be possible to verify the time
> period for which the walsender was actually terminated. Still, it will be
> limited to checking the fact of termination. What do you think?
>

+1. I don't think we need to measure any times, but we do need to exercise
that whole part of the code, so even a setting of 5ms would be an
improvement. Without it, the tests don't even touch the second half of
WalSndCheckShutdownTimeOut(), which is really the interesting bit.

-- 
Cheers,
Greg

  98. Re: Exit walsender before confirming remote flush in logical replication

    Andrey Silitskiy <a.silitskiy@postgrespro.ru> — 2026-03-15T20:52:41Z

    On Fri, 13 Mar 2026 Greg Sabino Mullane <htamfids(at)gmail(dot)com> wrote:
 > +1. I don't think we need to measure any times, but we do need to
 > exercise that whole part of the code, ...

Added a case with a positive but small wal_sender_shutdown_timeout
to the test.

Regards,
Andrey Silitskiy

  99. Re: Exit walsender before confirming remote flush in logical replication

    Greg Sabino Mullane <htamfids@gmail.com> — 2026-03-16T13:31:12Z

    On Sun, Mar 15, 2026 at 4:52 PM Andrey Silitskiy <a.silitskiy@postgrespro.ru>
wrote:

> Added a case with a positive but small wal_sender_shutdown_timeout to the
> test.
>

Thanks, looks good to me

  100. Re: Exit walsender before confirming remote flush in logical replication

    Fujii Masao <masao.fujii@gmail.com> — 2026-03-25T12:39:00Z

    On Mon, Mar 16, 2026 at 5:52 AM Andrey Silitskiy
<a.silitskiy@postgrespro.ru> wrote:
>
> On Fri, 13 Mar 2026 Greg Sabino Mullane <htamfids(at)gmail(dot)com> wrote:
>  > +1. I don't think we need to measure any times, but we do need to
>  > exercise that whole part of the code, ...
>
> Added a case with a positive but small wal_sender_shutdown_timeout
> to the test.

Thanks for updating the patch!

I tested wal_sender_shutdown_timeout under several configurations and
encountered a case where the primary shutdown got stuck, even with the patch
and wal_sender_shutdown_timeout = 1. I'm not sure yet whether this is a bug in
the patch or an issue with my test setup, but anyway I'd like to share
the reproduction steps for reference.

--------------------------------------------------------------------------------
#1. Set up primary, standby (with slot sync), and subscriber

initdb -D data --encoding=UTF8 --locale=C
cat <<EOF >> data/postgresql.conf
wal_level = logical
synchronized_standby_slots = 'physical_slot'
wal_sender_timeout = 1h
wal_sender_shutdown_timeout = 1
log_line_prefix = '%t %p [%b] data '
EOF
pg_ctl -D data start
pg_receivewal --create-slot -S physical_slot

pg_basebackup -D sby1 -c fast -R -S physical_slot -d "dbname=postgres"
cat <<EOF >> sby1/postgresql.conf
port = 5433
sync_replication_slots = on
hot_standby_feedback = on
log_line_prefix = '%t %p [%b] sby1 '
EOF
pg_ctl -D sby1 start

initdb -D sub1 --encoding=UTF8 --locale=C
cat <<EOF >> sub1/postgresql.conf
port = 5434
wal_level = logical
log_line_prefix = '%t %p [%b] sub1 '
EOF
pg_ctl -D sub1 start


#2. Create table, publication, and subscription

psql -p 5432 <<EOF
create table t (i int primary key, j int);
create publication mypub for table t;
EOF

psql -p 5434 <<EOF
create table t (i int primary key, j int);
create subscription mysub connection 'port=5432' publication mypub
with (failover = 'on');
EOF


#3. Lock the table on the subscriber

psql -p 5434 <<EOF
begin;
lock table t in access exclusive mode;
select pg_sleep(1000);
EOF


#4. Confirm the apply worker is waiting

psql -p 5432 -c "insert into t values(1, 0)"

psql -p 5434 -c "select * from pg_stat_activity where backend_type
like '%apply worker%'"


#5. Block walreceiver on the standby by using SIGSTOP signal

kill -SIGSTOP $(psql -p 5433 -X -Atc "SELECT pid FROM pg_stat_wal_receiver")


#6. Insert another row and shut down the primary

psql -p 5432 -c "insert into t values(2, 0)"

pg_ctl -D data stop
--------------------------------------------------------------------------------

In my tests, the shutdown in step #6 got stuck.

Any thoughts on whether this indicates a problem in the patch or
something off in my setup?

Regards,

-- 
Fujii Masao

  101. Re: Exit walsender before confirming remote flush in logical replication

    Vitaly Davydov <v.davydov@postgrespro.ru> — 2026-03-25T15:16:13Z

    Hi Fujii-san,

Thank you for the testing.

On 3/25/26 15:39, Fujii Masao wrote:
 > I tested wal_sender_shutdown_timeout under several configurations and
 > encountered a case where the primary shutdown got stuck, even with the patch
 > and wal_sender_shutdown_timeout = 1. I'm not sure yet whether this is a bug in
 > the patch or an issue with my test setup, but anyway I'd like to share
 > the reproduction steps for reference.

It seems that the problem lies in the logic of calculating sleep time in
WalSndComputeSleeptime function. If the parameter wal_sender_timeout is set to
one hour and the function WalSndWait executes with an argument sleeptime = 1h,
then the variable shutdown_request_timestamp will only be updated after one
hour at next call of WalSndCheckShutdownTimeout immediately following the
waiting period completion.

May be to use the minimal timeout in WalSndComputeSleeptimes or to use
the timeouts mechanism (timeout.c), but WalSndWait should wake up on latch then.

With best regards,
Vitaly

  102. Re: Exit walsender before confirming remote flush in logical replication

    Alexander Korotkov <aekorotkov@gmail.com> — 2026-03-29T12:34:06Z

    Hi, Fujii!

On Wed, Mar 25, 2026 at 2:39 PM Fujii Masao <masao.fujii@gmail.com> wrote:
> In my tests, the shutdown in step #6 got stuck.
>
> Any thoughts on whether this indicates a problem in the patch or
> something off in my setup?

I tried your test case, but I didn't stuck for me while using v5 patch
from Andrey [1].

One possible idea why hand may happen for is is that
WalSndWaitForWal() has missing WalSndCheckShutdownTimeout() call.  See
the patch attached.  Could I ask you, to retry with this patch applied
on top of v5 patch by Andrey [1].  And also, if possible, turn your
test into a perl tap test inside 038_walsnd_shutdown_timeout.pl.

Thank you.

Links
1. https://www.postgresql.org/message-id/aed6b990-e7d8-40d1-96a4-64ccbfbda876%40postgrespro.ru

------
Regards,
Alexander Korotkov
Supabase

  103. Re: Exit walsender before confirming remote flush in logical replication

    Andrey Silitskiy <a.silitskiy@postgrespro.ru> — 2026-03-30T03:14:15Z

    On Mar 29, 2026 Alexander Korotkov <aekorotkov(at)gmail(dot)com> wrote:
 > One possible idea why hand may happen for is is that
 > WalSndWaitForWal() has missing WalSndCheckShutdownTimeout() call.

On Mar 25, 2026 Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
 > I tested wal_sender_shutdown_timeout under several configurations and
 > encountered a case where the primary shutdown got stuck, ...

Thanks for your help in finding the issue!

I reproduced the problem, in this configuration it turned out that the
walsender was not terminated by wal_sender_shutdown_timeout in
WalSndWaitForWal(), but only when the physical slot was checked for 
inactive flag,
which caused shutdown to hang.

Fix added to the latest patch. Added a perl test-case for this 
configuration.
It is worth noting that in this configuration, the second walsender may
terminate due to inactive slot before the wal_sender_shutdown_timeout
terminates the process, so the test checks timeout termination of only
one walsender.

Regards,
Andrey Silitskiy

  104. Re: Exit walsender before confirming remote flush in logical replication

    Andrey Silitskiy <a.silitskiy@postgrespro.ru> — 2026-03-30T17:31:04Z

    On Mar 30, 2026 Andrey Silitskiy
<a(dot)silitskiy(at)postgrespro(dot)ru>wrote:
 > It is worth noting that in this configuration, the second walsender may
 > terminate due to inactive slot before the wal_sender_shutdown_timeout
 > terminates the process, ...

Updated. In this configuration, in some situations walsender processes
may already have been terminated by the time of fast shutdown.
And in this case, the fast shutdown will be successful, but it will not
be the reason for walsender termination and the logs will be different.
Decided to leave the new shutdown test case, but not check the logs in it.

  105. Re: Exit walsender before confirming remote flush in logical replication

    Fujii Masao <masao.fujii@gmail.com> — 2026-03-31T17:33:20Z

    On Mon, Mar 30, 2026 at 12:14 PM Andrey Silitskiy
<a.silitskiy@postgrespro.ru> wrote:
>
> On Mar 29, 2026 Alexander Korotkov <aekorotkov(at)gmail(dot)com> wrote:
>  > One possible idea why hand may happen for is is that
>  > WalSndWaitForWal() has missing WalSndCheckShutdownTimeout() call.
>
> On Mar 25, 2026 Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>  > I tested wal_sender_shutdown_timeout under several configurations and
>  > encountered a case where the primary shutdown got stuck, ...
>
> Thanks for your help in finding the issue!
>
> I reproduced the problem, in this configuration it turned out that the
> walsender was not terminated by wal_sender_shutdown_timeout in
> WalSndWaitForWal(), but only when the physical slot was checked for
> inactive flag,
> which caused shutdown to hang.

Regarding the issue I reported, Vitaly's analysis upthread seems correct to me.
If WalSndComputeSleeptime() is called before WalSndCheckShutdownTimeout(), then
shutdown_request_timestamp is still 0, so wal_sender_shutdown_timeout is not
taken into account even though shutdown has already been requested
(i.e., got_STOPPING || got_SIGUSR2 is true).

In that case, if wal_sender_timeout is large, the computed sleep time can also
be large, and the walsender may wait in WalSndWait() longer than intended.

To fix this, walsender should call WalSndCheckShutdownTimeout() first so that
shutdown_request_timestamp is set before computing the sleep time. The v7 patch
already does this, which looks good to me. The comments for that
WalSndCheckShutdownTimeout() might need an update, though.

Regards,

-- 
Fujii Masao

  106. Re: Exit walsender before confirming remote flush in logical replication

    Fujii Masao <masao.fujii@gmail.com> — 2026-03-31T17:34:59Z

    On Tue, Mar 31, 2026 at 2:31 AM Andrey Silitskiy
<a.silitskiy@postgrespro.ru> wrote:
>
> On Mar 30, 2026 Andrey Silitskiy
> <a(dot)silitskiy(at)postgrespro(dot)ru>wrote:
>  > It is worth noting that in this configuration, the second walsender may
>  > terminate due to inactive slot before the wal_sender_shutdown_timeout
>  > terminates the process, ...
>
> Updated. In this configuration, in some situations walsender processes
> may already have been terminated by the time of fast shutdown.
> And in this case, the fast shutdown will be successful, but it will not
> be the reason for walsender termination and the logs will be different.
> Decided to leave the new shutdown test case, but not check the logs in it.

Thanks for updating the patch!


+     <varlistentry id="guc-wal_sender_shutdown_timeout"
xreflabel="wal_sender_shutdown_timeout">

Placing wal_sender_shutdown_timeout next to wal_sender_timeout would improve
readability, since they are closely related. Also, the "id" should probably use
hyphens (i.e., guc-wal-sender-shutdown-timeout) rather than underscores.


 This parameter can be set individually
+        for each walsender.

This sentence doesn't seem necessary, as similar GUCs don't mention this.


+        If disabled, the walsender will wait for all WAL data to be
+        successfully flushed on the receiver side before exiting the process.

This description is a bit misleading, since walsender basically waits for WAL
replication regardless. It would be clearer to describe the behavior in terms
of waiting for replication to complete, and how the timeout affects that.
For example,

------------------------------------
Specifies the maximum time the server waits during shutdown for all
WAL data to be replicated to the receiver. If this value is specified
without units, it is taken as milliseconds. A value of -1 (the
default) disables the timeout mechanism.

When replication is in use, the sending server normally waits until
all WAL data has been transferred to the receiver before completing
shutdown. This helps keep sender and receiver in sync after shutdown,
which is especially important for physical replication switchovers,
but it can delay shutdown.

If this parameter is set, the server stops waiting and completes
shutdown when the timeout expires. This can shorten shutdown time, for
example, when replication is slow on high-latency networks or when a
logical replication apply worker is blocked waiting for locks.
However, in this case the sender and receiver may be out of sync after
shutdown.
------------------------------------


+    Users will stop waiting if a fast shutdown is requested. However, if
+    <varname>wal_sender_shutdown_timeout</varname> is not set, the server will
+    not fully shutdown until all outstanding WAL records are transferred to
+    the currently connected standby servers. This waiting applies to both
+    asynchronous and synchronous replication.

Similarly, the paragraph about fast shutdown is slightly misleading for the
same reason above. How about updating this to the following, for example?

------------------------------------
Users will stop waiting if a fast shutdown is requested. However, when
using replication, the server will not fully shutdown until all
outstanding WAL records are transferred to the currently connected
standby servers, or wal_sender_shutdown_timeout (if set) expires,
regardless of whether replication is synchronous or asynchronous.
------------------------------------


+    <varname>wal_sender_shutdown_timeout</varname> is not set, the server will

Instead of referring to the parameter name directly, it would be better to use
<xref linkend="guc-wal-sender-shutdown-timeout"/> so readers can easily jump
to its description.


+ if (shutdown_request_timestamp == 0)
+ {
+ shutdown_request_timestamp = now;

It might be safer to set shutdown_request_timestamp even when
wal_sender_shutdown_timeout = -1, in case the value is changed during shutdown.
Currently, wal_sender_shutdown_timeout cannot normally be changed during
shutdown, since the postmaster does almost nothing on SIGHUP. However,
sending SIGHUP directly to the walsender can still change this parameter even
during shutdown.


+ ereport(WARNING,
+ (errmsg("terminating walsender due to wal_sender_shutdown_timeout
expiration, replication may be incomplete")));

"expiration" seems unnecessary for the sake of other similar log messages like
"terminating walsender process due to replication timeout". So omething like
"terminating walsender process due to wal_sender_shutdown_timeout" should be
enough.

It would also be better to move "replication may be incomplete" to errdetail(),
and clarify it, for example, "Walsender is terminated before all WAL data was
replicated to the receiver". Thought?

Regards,

-- 
Fujii Masao

  107. Re: Exit walsender before confirming remote flush in logical replication

    Fujii Masao <masao.fujii@gmail.com> — 2026-04-03T06:13:39Z

    On Wed, Apr 1, 2026 at 2:34 AM Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Tue, Mar 31, 2026 at 2:31 AM Andrey Silitskiy
> <a.silitskiy@postgrespro.ru> wrote:
> >
> > On Mar 30, 2026 Andrey Silitskiy
> > <a(dot)silitskiy(at)postgrespro(dot)ru>wrote:
> >  > It is worth noting that in this configuration, the second walsender may
> >  > terminate due to inactive slot before the wal_sender_shutdown_timeout
> >  > terminates the process, ...
> >
> > Updated. In this configuration, in some situations walsender processes
> > may already have been terminated by the time of fast shutdown.
> > And in this case, the fast shutdown will be successful, but it will not
> > be the reason for walsender termination and the logs will be different.
> > Decided to leave the new shutdown test case, but not check the logs in it.
>
> Thanks for updating the patch!
>
>
> +     <varlistentry id="guc-wal_sender_shutdown_timeout"
> xreflabel="wal_sender_shutdown_timeout">
>
> Placing wal_sender_shutdown_timeout next to wal_sender_timeout would improve
> readability, since they are closely related. Also, the "id" should probably use
> hyphens (i.e., guc-wal-sender-shutdown-timeout) rather than underscores.
>
>
>  This parameter can be set individually
> +        for each walsender.
>
> This sentence doesn't seem necessary, as similar GUCs don't mention this.
>
>
> +        If disabled, the walsender will wait for all WAL data to be
> +        successfully flushed on the receiver side before exiting the process.
>
> This description is a bit misleading, since walsender basically waits for WAL
> replication regardless. It would be clearer to describe the behavior in terms
> of waiting for replication to complete, and how the timeout affects that.
> For example,
>
> ------------------------------------
> Specifies the maximum time the server waits during shutdown for all
> WAL data to be replicated to the receiver. If this value is specified
> without units, it is taken as milliseconds. A value of -1 (the
> default) disables the timeout mechanism.
>
> When replication is in use, the sending server normally waits until
> all WAL data has been transferred to the receiver before completing
> shutdown. This helps keep sender and receiver in sync after shutdown,
> which is especially important for physical replication switchovers,
> but it can delay shutdown.
>
> If this parameter is set, the server stops waiting and completes
> shutdown when the timeout expires. This can shorten shutdown time, for
> example, when replication is slow on high-latency networks or when a
> logical replication apply worker is blocked waiting for locks.
> However, in this case the sender and receiver may be out of sync after
> shutdown.
> ------------------------------------
>
>
> +    Users will stop waiting if a fast shutdown is requested. However, if
> +    <varname>wal_sender_shutdown_timeout</varname> is not set, the server will
> +    not fully shutdown until all outstanding WAL records are transferred to
> +    the currently connected standby servers. This waiting applies to both
> +    asynchronous and synchronous replication.
>
> Similarly, the paragraph about fast shutdown is slightly misleading for the
> same reason above. How about updating this to the following, for example?
>
> ------------------------------------
> Users will stop waiting if a fast shutdown is requested. However, when
> using replication, the server will not fully shutdown until all
> outstanding WAL records are transferred to the currently connected
> standby servers, or wal_sender_shutdown_timeout (if set) expires,
> regardless of whether replication is synchronous or asynchronous.
> ------------------------------------
>
>
> +    <varname>wal_sender_shutdown_timeout</varname> is not set, the server will
>
> Instead of referring to the parameter name directly, it would be better to use
> <xref linkend="guc-wal-sender-shutdown-timeout"/> so readers can easily jump
> to its description.
>
>
> + if (shutdown_request_timestamp == 0)
> + {
> + shutdown_request_timestamp = now;
>
> It might be safer to set shutdown_request_timestamp even when
> wal_sender_shutdown_timeout = -1, in case the value is changed during shutdown.
> Currently, wal_sender_shutdown_timeout cannot normally be changed during
> shutdown, since the postmaster does almost nothing on SIGHUP. However,
> sending SIGHUP directly to the walsender can still change this parameter even
> during shutdown.
>
>
> + ereport(WARNING,
> + (errmsg("terminating walsender due to wal_sender_shutdown_timeout
> expiration, replication may be incomplete")));
>
> "expiration" seems unnecessary for the sake of other similar log messages like
> "terminating walsender process due to replication timeout". So omething like
> "terminating walsender process due to wal_sender_shutdown_timeout" should be
> enough.
>
> It would also be better to move "replication may be incomplete" to errdetail(),
> and clarify it, for example, "Walsender is terminated before all WAL data was
> replicated to the receiver". Thought?

I've updated the patch based on my earlier comments and made some cosmetic
improvements. The updated version is attached.

I haven't fully reviewed the new tests yet. It looks like five test cases are
added, and I'm wondering if all of them are necessary, or if we should keep
only the valuable ones to avoid unnecessarily increasing test time.

Regards,

-- 
Fujii Masao

  108. Re: Exit walsender before confirming remote flush in logical replication

    Andrey Silitskiy <a.silitskiy@postgrespro.ru> — 2026-04-03T09:37:36Z

    On Apr 3, 2026 Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
 > I've updated the patch based on my earlier comments and made some
 > cosmetic improvements.

Thanks for comments! I also just made the same changes after your review.
Attaching an updated patch with fixes after your review, some of last
cosmetic changes and two excluded test cases.

There are currently 5 test cases: two for 0ms(empty and full buffers), two
for 10ms  (empty and full buffers), and one for the standby case. I think
it is ok to exclude two cases with 0ms to speed up the launch of tests.

 > This sentence doesn't seem necessary, as similar GUCs don't mention this

I decided to leave in the documentation a mention of the possibility of
setting a parameter per replication connection. I think some users are not
aware of this possibility and this short sentence might give them the idea.

 > It would also be better to move "replication may be incomplete" to 
errdetail(),
 > and clarify it, for example, "Walsender is terminated before all WAL 
data was
 > replicated to the receiver".

In some cases, replication may be fully completed even when exiting with
WalSndDoneImmediate, so I edited the message.

Regards,
Andrey Silitskiy

  109. Re: Exit walsender before confirming remote flush in logical replication

    Fujii Masao <masao.fujii@gmail.com> — 2026-04-05T02:24:08Z

    On Fri, Apr 3, 2026 at 6:37 PM Andrey Silitskiy
<a.silitskiy@postgrespro.ru> wrote:
>
> On Apr 3, 2026 Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>  > I've updated the patch based on my earlier comments and made some
>  > cosmetic improvements.
>
> Thanks for comments! I also just made the same changes after your review.
> Attaching an updated patch with fixes after your review, some of last
> cosmetic changes and two excluded test cases.
>
> There are currently 5 test cases: two for 0ms(empty and full buffers), two
> for 10ms  (empty and full buffers), and one for the standby case. I think
> it is ok to exclude two cases with 0ms to speed up the launch of tests.

Agreed.
I removed those two tests and restructured the walsender shutdown tests.

Previously, each test was defined as a function and executed with different
settings (0ms and 10ms). Since the 0ms variants were dropped, I simplified
the structure and inlined the tests for better readability.

For the test case where both physical and logical replication run with
slotsync enabled, the test used SIGSTOP to stop the walreceiver. However,
sending SIGSTOP via kill is not available on Windows, so this test is now
skipped on Windows.

An updated patch is attached. I'm thinking to commit this before the feature
freeze. If there are any remaining minor issues, we can continue to refine
them afterward.


>  > This sentence doesn't seem necessary, as similar GUCs don't mention this
>
> I decided to leave in the documentation a mention of the possibility of
> setting a parameter per replication connection. I think some users are not
> aware of this possibility and this short sentence might give them the idea.

OK. So, how about expanding the description to make it clearer for users?
I updated the description as follows in the updated patch.

+        This parameter can be set in <varname>primary_conninfo</varname> and
+        in the <literal>CONNECTION</literal> clause of
+        <command>CREATE SUBSCRIPTION</command> (for example, include
+        <literal>options=-cwal_sender_shutdown_timeout=10s</literal> in the
+        connection string), allowing different timeouts per replication
+        connection.  For example, when both physical and logical replication
+        are used, it can be disabled for physical replication (e.g., for
+        switchovers) while enabled for logical replication to limit shutdown
+        time.


>  > It would also be better to move "replication may be incomplete" to
> errdetail(),
>  > and clarify it, for example, "Walsender is terminated before all WAL
> data was
>  > replicated to the receiver".
>
> In some cases, replication may be fully completed even when exiting with
> WalSndDoneImmediate, so I edited the message.

Understood. Per the Error Message Style Guide [1], I changed "may" to "might"
in the detail message:

+ errdetail("Walsender process might have been terminated before all
WAL data was replicated to the receiver.")));

Regards,

[1] https://www.postgresql.org/docs/devel/error-style-guide.html#ERROR-STYLE-GUIDE-TRICKY-WORDS

-- 
Fujii Masao

  110. Re: Exit walsender before confirming remote flush in logical replication

    Fujii Masao <masao.fujii@gmail.com> — 2026-04-06T02:40:24Z

    On Sun, Apr 5, 2026 at 11:24 AM Fujii Masao <masao.fujii@gmail.com> wrote:
> An updated patch is attached. I'm thinking to commit this before the feature
> freeze.

I've pushed the patch. Thanks!

If there are any remaining issues with this feature, let's continue discussing
and improving it!


Regards,

-- 
Fujii Masao

  111. Re: Exit walsender before confirming remote flush in logical replication

    Andres Freund <andres@anarazel.de> — 2026-04-06T15:32:55Z

    Hi,

On 2026-04-06 11:40:24 +0900, Fujii Masao wrote:
> On Sun, Apr 5, 2026 at 11:24 AM Fujii Masao <masao.fujii@gmail.com> wrote:
> > An updated patch is attached. I'm thinking to commit this before the feature
> > freeze.
>
> I've pushed the patch. Thanks!

Failed on CI just now:

https://cirrus-ci.com/task/6745359004729344?logs=test_world#L410
https://api.cirrus-ci.com/v1/artifact/task/6745359004729344/testrun/build/testrun/subscription/038_walsnd_shutdown_timeout/log/regress_log_038_walsnd_shutdown_timeout

[14:58:26.146](0.066s) ok 3 - have walreceiver pid 13796
### Stopping node "publisher" using mode fast
# Running: pg_ctl --pgdata /home/postgres/postgres/build/testrun/subscription/038_walsnd_shutdown_timeout/data/t_038_walsnd_shutdown_timeout_publisher_data/pgdata --mode fast stop
waiting for server to shut down........................................................................................................................... failed
pg_ctl: server does not shut down
# pg_ctl stop failed: 256
# Postmaster PID for node "publisher" is 3679
[15:00:38.178](132.032s) Bail out!  pg_ctl stop failed

Greetings,

Andres

  112. Re: Exit walsender before confirming remote flush in logical replication

    Fujii Masao <masao.fujii@gmail.com> — 2026-04-07T05:39:07Z

    On Tue, Apr 7, 2026 at 12:32 AM Andres Freund <andres@anarazel.de> wrote:
> Failed on CI just now:
>
> https://cirrus-ci.com/task/6745359004729344?logs=test_world#L410
> https://api.cirrus-ci.com/v1/artifact/task/6745359004729344/testrun/build/testrun/subscription/038_walsnd_shutdown_timeout/log/regress_log_038_walsnd_shutdown_timeout
>
> [14:58:26.146](0.066s) ok 3 - have walreceiver pid 13796
> ### Stopping node "publisher" using mode fast
> # Running: pg_ctl --pgdata /home/postgres/postgres/build/testrun/subscription/038_walsnd_shutdown_timeout/data/t_038_walsnd_shutdown_timeout_publisher_data/pgdata --mode fast stop
> waiting for server to shut down........................................................................................................................... failed
> pg_ctl: server does not shut down
> # pg_ctl stop failed: 256
> # Postmaster PID for node "publisher" is 3679
> [15:00:38.178](132.032s) Bail out!  pg_ctl stop failed

Thanks for reporting this!

From the CI results [1], the failure in 038_walsnd_shutdown_timeout.pl appears
to occur intermittently on FreeBSD. The failing case tests that, when both
physical and logical replication are in use with slotsync enabled and both are
stalled (walreceiver on the standby and the logical apply worker on
the subscriber are blocked), shutting down the primary completes due to
wal_sender_shutdown_timeout.

On FreeBSD, however, it seems that after the shutdown request, the physical
walsender can occasionally keep running, preventing shutdown from completing.
As a result, pg_ctl stop times out and the test fails.

I’ll investigate the cause. If it takes time to identify, I may temporarily
disable just this test case so it doesn’t block other development and testing,
then re-enable it once the issue is fixed.

Regards,

[1]
https://cirrus-ci.com/build/5134823678803968
https://cirrus-ci.com/build/5735329598013440
https://cirrus-ci.com/build/5917696627310592
https://cirrus-ci.com/build/5742460250357760

-- 
Fujii Masao

  113. Re: Exit walsender before confirming remote flush in logical replication

    Chao Li <li.evan.chao@gmail.com> — 2026-04-08T07:04:34Z

    
> On Apr 7, 2026, at 13:39, Fujii Masao <masao.fujii@gmail.com> wrote:
> 
> On Tue, Apr 7, 2026 at 12:32 AM Andres Freund <andres@anarazel.de> wrote:
>> Failed on CI just now:
>> 
>> https://cirrus-ci.com/task/6745359004729344?logs=test_world#L410
>> https://api.cirrus-ci.com/v1/artifact/task/6745359004729344/testrun/build/testrun/subscription/038_walsnd_shutdown_timeout/log/regress_log_038_walsnd_shutdown_timeout
>> 
>> [14:58:26.146](0.066s) ok 3 - have walreceiver pid 13796
>> ### Stopping node "publisher" using mode fast
>> # Running: pg_ctl --pgdata /home/postgres/postgres/build/testrun/subscription/038_walsnd_shutdown_timeout/data/t_038_walsnd_shutdown_timeout_publisher_data/pgdata --mode fast stop
>> waiting for server to shut down........................................................................................................................... failed
>> pg_ctl: server does not shut down
>> # pg_ctl stop failed: 256
>> # Postmaster PID for node "publisher" is 3679
>> [15:00:38.178](132.032s) Bail out!  pg_ctl stop failed
> 
> Thanks for reporting this!
> 
> From the CI results [1], the failure in 038_walsnd_shutdown_timeout.pl appears
> to occur intermittently on FreeBSD. The failing case tests that, when both
> physical and logical replication are in use with slotsync enabled and both are
> stalled (walreceiver on the standby and the logical apply worker on
> the subscriber are blocked), shutting down the primary completes due to
> wal_sender_shutdown_timeout.
> 
> On FreeBSD, however, it seems that after the shutdown request, the physical
> walsender can occasionally keep running, preventing shutdown from completing.
> As a result, pg_ctl stop times out and the test fails.
> 
> I’ll investigate the cause. If it takes time to identify, I may temporarily
> disable just this test case so it doesn’t block other development and testing,
> then re-enable it once the issue is fixed.
> 
> Regards,
> 
> [1]
> https://cirrus-ci.com/build/5134823678803968
> https://cirrus-ci.com/build/5735329598013440
> https://cirrus-ci.com/build/5917696627310592
> https://cirrus-ci.com/build/5742460250357760
> 
> -- 
> Fujii Masao
> 
> 

I have some CF entries failed on this test case as well, so I tried to look into the problem. I have a finding for your reference.

With a8f45dee917, wal_sender_shutdown_timeout is only enforced while the walsender keeps returning to WalSndCheckShutdownTimeout() in the main loops, but there is a path to enter WalSndDone:
```
			/*
			 * When SIGUSR2 arrives, we send any outstanding logs up to the
			 * shutdown checkpoint record (i.e., the latest record), wait for
			 * them to be replicated to the standby, and exit. This may be a
			 * normal termination at shutdown, or a promotion, the walsender
			 * is not sure which.
			 */
			if (got_SIGUSR2)
				WalSndDone(send_data);
```

Once entering WalSndDone(), it might call pg_flush() and get stuck:
```
	if (WalSndCaughtUp && sentPtr == replicatedPtr &&
		!pq_is_send_pending())
	{
		QueryCompletion qc;

		/* Inform the standby that XLOG streaming is done */
		SetQueryCompletion(&qc, CMDTAG_COPY, 0);
		EndCommand(&qc, DestRemote, false);
		pq_flush();

		proc_exit(0);
```

And once stuck, it will never get back to WalSndCheckShutdownTimeout(), so the new GUC timeout cannot rescue it.

In WalSndDoneImmediate(), pq_flush_if_writable() is used, and the comment talks about the possible stuck:
```
		/*
		 * Note that the output buffer may be full during the forced shutdown
		 * of walsender. If pq_flush() is called at that time, the walsender
		 * process will be stuck. Therefore, call pq_flush_if_writable()
		 * instead. Successful reception of the done message with the
		 * walsender forced into a shutdown is not guaranteed.
		 */
		pq_flush_if_writable();
```

So, maybe switch to use pq_flush_if_writable() in WalSndDone()?

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  114. Re: Exit walsender before confirming remote flush in logical replication

    Fujii Masao <masao.fujii@gmail.com> — 2026-04-08T08:11:50Z

    On Wed, Apr 8, 2026 at 4:05 PM Chao Li <li.evan.chao@gmail.com> wrote:
> I have some CF entries failed on this test case as well, so I tried to look into the problem.

Thanks for working on this, much appreciated!


> Once entering WalSndDone(), it might call pg_flush() and get stuck:
> ```
>         if (WalSndCaughtUp && sentPtr == replicatedPtr &&
>                 !pq_is_send_pending())
>         {
>                 QueryCompletion qc;
>
>                 /* Inform the standby that XLOG streaming is done */
>                 SetQueryCompletion(&qc, CMDTAG_COPY, 0);
>                 EndCommand(&qc, DestRemote, false);
>                 pq_flush();
>
>                 proc_exit(0);
> ```
>
> And once stuck, it will never get back to WalSndCheckShutdownTimeout(), so the new GUC timeout cannot rescue it.

pq_flush() is called when WalSndCaughtUp && sentPtr == replicatedPtr
&& !pq_is_send_pending().
Under these conditions, I was thinking that we can assume the kernel send
buffer isn't full, so pq_flush() (i.e., the send() call) can copy the data
without blocking and return immediately.

I'm not very familiar with FreeBSD, but based on [1], I wonder if this
assumption may not hold there, and pq_flush() could still block....

Regards,

[1] https://man.freebsd.org/cgi/man.cgi?unix(4)#BUFFERING

> Due  to the local nature of the Unix-domain sockets, they do not imple-
> ment send buffers.  The send(2) and write(2) families of system calls
> attempt to write data to the receive buffer of the destination socket.

-- 
Fujii Masao

  115. Re: Exit walsender before confirming remote flush in logical replication

    Chao Li <li.evan.chao@gmail.com> — 2026-04-08T08:38:32Z

    
> On Apr 8, 2026, at 16:11, Fujii Masao <masao.fujii@gmail.com> wrote:
> 
> On Wed, Apr 8, 2026 at 4:05 PM Chao Li <li.evan.chao@gmail.com> wrote:
>> I have some CF entries failed on this test case as well, so I tried to look into the problem.
> 
> Thanks for working on this, much appreciated!
> 
> 
>> Once entering WalSndDone(), it might call pg_flush() and get stuck:
>> ```
>>        if (WalSndCaughtUp && sentPtr == replicatedPtr &&
>>                !pq_is_send_pending())
>>        {
>>                QueryCompletion qc;
>> 
>>                /* Inform the standby that XLOG streaming is done */
>>                SetQueryCompletion(&qc, CMDTAG_COPY, 0);
>>                EndCommand(&qc, DestRemote, false);
>>                pq_flush();
>> 
>>                proc_exit(0);
>> ```
>> 
>> And once stuck, it will never get back to WalSndCheckShutdownTimeout(), so the new GUC timeout cannot rescue it.
> 
> pq_flush() is called when WalSndCaughtUp && sentPtr == replicatedPtr
> && !pq_is_send_pending().
> Under these conditions, I was thinking that we can assume the kernel send
> buffer isn't full, so pq_flush() (i.e., the send() call) can copy the data
> without blocking and return immediately.
> 
> I'm not very familiar with FreeBSD, but based on [1], I wonder if this
> assumption may not hold there, and pq_flush() could still block....
> 
> Regards,
> 
> [1] https://man.freebsd.org/cgi/man.cgi?unix(4)#BUFFERING
> 
>> Due  to the local nature of the Unix-domain sockets, they do not imple-
>> ment send buffers.  The send(2) and write(2) families of system calls
>> attempt to write data to the receive buffer of the destination socket.
> 
> -- 
> Fujii Masao

I don’t have a FreeBSD box to verify that directly. But the document you pointed out seems to state explicitly that, on Unix-domain sockets, writes go directly to the peer’s receive buffer. If so, the assumption that “the kernel send buffer isn’t full” no longer really holds on FreeBSD. From this perspective, changing to non-blocking pq_flush_if_writable() makes sense to me.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  116. Re: Exit walsender before confirming remote flush in logical replication

    Evgeny Voropaev <evgeny.voropaev@tantorlabs.com> — 2026-04-21T10:41:00Z

    > Failed on CI just now:
>
> https://cirrus-ci.com/task/6745359004729344?logs=test_world#L410
> https://api.cirrus-ci.com/v1/artifact/task/6745359004729344/testrun/build/testrun/subscription/038_walsnd_shutdown_timeout/log/regress_log_038_walsnd_shutdown_timeout
>
> [14:58:26.146](0.066s) ok 3 - have walreceiver pid 13796
> ### Stopping node "publisher" using mode fast
> # Running: pg_ctl --pgdata /home/postgres/postgres/build/testrun/subscription/038_walsnd_shutdown_timeout/data/t_038_walsnd_shutdown_timeout_publisher_data/pgdata --mode fast stop
> waiting for server to shut down........................................................................................................................... failed
> pg_ctl: server does not shut down
> # pg_ctl stop failed: 256
> # Postmaster PID for node "publisher" is 3679
> [15:00:38.178](132.032s) Bail out!  pg_ctl stop failed

+1
It still fails sporadically.
For example, It had failed here - https://cirrus-ci.com/task/5208188162670592?logs=test_world#L417

     ok 3 - have walreceiver pid 69871
     Bail out!  pg_ctl stop failed
     # test failed

and then passed here:
https://cirrus-ci.com/task/5031400161673216?logs=test_world#L340

Best regards,
Evgeny Voropaev,Tantor Labs, LLC

  117. Re: Exit walsender before confirming remote flush in logical replication

    Fujii Masao <masao.fujii@gmail.com> — 2026-04-21T18:32:14Z

    On Wed, Apr 8, 2026 at 5:39 PM Chao Li <li.evan.chao@gmail.com> wrote:
> I don’t have a FreeBSD box to verify that directly. But the document you pointed out seems to state explicitly that, on Unix-domain sockets, writes go directly to the peer’s receive buffer. If so, the assumption that “the kernel send buffer isn’t full” no longer really holds on FreeBSD. From this perspective, changing to non-blocking pq_flush_if_writable() makes sense to me.

I was thinking about the effect of replacing that pq_flush() with
pq_flush_if_writable().

Under normal conditions, there should be no behavioral difference. In either
case, the end-of-streaming message is sent to the standby or subscriber.

The difference only appears if walsender cannot complete the send() call for
that message immediately (that is, it cannot append the message to the kernel
send buffer). This should be rare, because that pq_flush() call happens under
the condition "WalSndCaughtUp && sentPtr == replicatedPtr &&
!pq_is_send_pending()",
where the send buffer would normally not be full. However, as observed,
this can apparently happen on FreeBSD when using Unix-domain sockets.

Even if pq_flush_if_writable() fails to send the end-of-streaming message,
walreceiver and the logical apply worker seem to behave almost the same
in practice whether they receive it or not. The main differences are how they
detect closure of the replication connection and the resulting log messages.
If that analysis is correct, replacing pq_flush() with pq_flush_if_writable()
seems acceptable to me.

If the end-of-streaming message is received, walreceiver and the apply worker
log messages like the following, then try to send a reply, which fails because
the connection has already been closed:

    [walreceiver] LOG: replication terminated by primary server
    [walreceiver] DETAIL: End of WAL reached ...
    [walreceiver] FATAL: could not send end-of-streaming message to
primary: server closed the connection unexpectedly

    [apply worker] LOG: data stream from publisher has ended
    [apply worker] ERROR: could not send end-of-streaming message to
primary: server closed the connection unexpectedly

If the message is not received, they simply detect the closed connection
while waiting for the next message:

    [walreceiver] FATAL: could not receive data from WAL stream:
server closed the connection unexpectedly

    [apply worker] ERROR: could not receive data from WAL stream:
server closed the connection unexpectedly

Therefore, since replacing pq_flush() with pq_flush_if_writable() seems to
change behavior only in a limited and acceptable way, I'm thinking to create
the patch doing that replacement.

BTW, though this is not directly related to this topic, I'm also wondering
whether walreceiver can ever successfully send an end-of-streaming message
back to walsender. It appears to attempt that only after the replication
connection has already been closed, which would seem to make it fail every time.

Regards,

-- 
Fujii Masao

  118. Re: Exit walsender before confirming remote flush in logical replication

    Fujii Masao <masao.fujii@gmail.com> — 2026-04-23T04:51:53Z

    On Wed, Apr 22, 2026 at 3:32 AM Fujii Masao <masao.fujii@gmail.com> wrote:
> Therefore, since replacing pq_flush() with pq_flush_if_writable() seems to
> change behavior only in a limited and acceptable way, I'm thinking to create
> the patch doing that replacement.

On second thought, replacing pq_flush() with pq_flush_if_writable() is not
sufficient. EndCommand(), which WalSndDone() calls before pq_flush(), can also
block when the send buffer is full. That happens because EndCommand() uses
pq_putmessage() rather than pq_putmessage_noblock().

Also, replacing pq_flush() with pq_flush_if_writable() would cause walsender to
give up sending pending messages, including CommandComplete, even before
wal_sender_shutdown_timeout expires. That seems a bit odd. I think it is better
for walsender to continue honoring wal_sender_shutdown_timeout while attempting
to send the final CommandComplete message.

I've attached a patch that addresses both issues. For the first, it introduces
EndCommandExtended(), which allows CommandComplete to be queued with
pq_putmessage_noblock(). For the second, it updates WalSndDone() to use
ProcessPendingWrites() instead of pq_flush(), so the walsender write loop can
continue processing replies and checking replication and shutdown timeouts
while pending output is being flushed.

Thoughts?

Regards,

-- 
Fujii Masao

  119. Re: Exit walsender before confirming remote flush in logical replication

    Chao Li <li.evan.chao@gmail.com> — 2026-04-24T02:04:26Z

    
> On Apr 23, 2026, at 12:51, Fujii Masao <masao.fujii@gmail.com> wrote:
> 
> On Wed, Apr 22, 2026 at 3:32 AM Fujii Masao <masao.fujii@gmail.com> wrote:
>> Therefore, since replacing pq_flush() with pq_flush_if_writable() seems to
>> change behavior only in a limited and acceptable way, I'm thinking to create
>> the patch doing that replacement.
> 
> On second thought, replacing pq_flush() with pq_flush_if_writable() is not
> sufficient. EndCommand(), which WalSndDone() calls before pq_flush(), can also
> block when the send buffer is full. That happens because EndCommand() uses
> pq_putmessage() rather than pq_putmessage_noblock().
> 
> Also, replacing pq_flush() with pq_flush_if_writable() would cause walsender to
> give up sending pending messages, including CommandComplete, even before
> wal_sender_shutdown_timeout expires. That seems a bit odd. I think it is better
> for walsender to continue honoring wal_sender_shutdown_timeout while attempting
> to send the final CommandComplete message.
> 
> I've attached a patch that addresses both issues. For the first, it introduces
> EndCommandExtended(), which allows CommandComplete to be queued with
> pq_putmessage_noblock(). For the second, it updates WalSndDone() to use
> ProcessPendingWrites() instead of pq_flush(), so the walsender write loop can
> continue processing replies and checking replication and shutdown timeouts
> while pending output is being flushed.
> 
> Thoughts?
> 
> Regards,
> 
> -- 
> Fujii Masao
> <v1-0001-Avoid-blocking-indefinitely-while-finishing-walse.patch>

```
-		EndCommand(&qc, DestRemote, false);
-		pq_flush();
+		EndCommandExtended(&qc, DestRemote, false, true);
+		shutdown_stream_done_queued = true;
+
+		/*
+		 * Don't call pq_flush() here. It can block indefinitely waiting for
+		 * the socket to become writeable, which would prevent
+		 * wal_sender_shutdown_timeout from being enforced. Use the regular
+		 * walsender non-blocking flush path so shutdown and replication
+		 * timeouts continue to be checked while waiting for the send buffer
+		 * to drain.
+		 */
+		ProcessPendingWrites();
```

I think adding EndCommandExtended() with a “nonblock” parameter is good. However, I have a suspicion replacing pg_flush with ProcessPendingWrites().

ProcessPendingWrites() calls ProcessRepliesIfAny() in the first place, so if it is possible that, a new COPY message is appended after the already-queued CommandComplete? Which seems to violate the protocol, but I am not sure if that would lead to any trouble.

So, maybe we need a new helper, say ProcessPendingWritesForShutdown(), that loops while pq_is_send_pending(), call WalSndCheckShutdownTimeout() and only wait for WL_SOCKET_WRITEABLE, then pq_flush_if_writable(), on flush failure, maybe WalSndShutdown().

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  120. Re: Exit walsender before confirming remote flush in logical replication

    Fujii Masao <masao.fujii@gmail.com> — 2026-04-24T10:29:08Z

    On Fri, Apr 24, 2026 at 11:05 AM Chao Li <li.evan.chao@gmail.com> wrote:
> ProcessPendingWrites() calls ProcessRepliesIfAny() in the first place, so if it is possible that, a new COPY message is appended after the already-queued CommandComplete? Which seems to violate the protocol, but I am not sure if that would lead to any trouble.
>
> So, maybe we need a new helper, say ProcessPendingWritesForShutdown(), that loops while pq_is_send_pending(), call WalSndCheckShutdownTimeout() and only wait for WL_SOCKET_WRITEABLE, then pq_flush_if_writable(), on flush failure, maybe WalSndShutdown().

Thanks for the review! You're right.

I added such loop directly in WalSndDone() instead of introducing a new helper
function, since only WalSndDone() needs it and the extra loop does not make
WalSndDone() significantly more complicated.

I've attached an updated patch.

Regards,

-- 
Fujii Masao

  121. Re: Exit walsender before confirming remote flush in logical replication

    Chao Li <li.evan.chao@gmail.com> — 2026-04-30T07:10:03Z

    
> On Apr 24, 2026, at 18:29, Fujii Masao <masao.fujii@gmail.com> wrote:
> 
> On Fri, Apr 24, 2026 at 11:05 AM Chao Li <li.evan.chao@gmail.com> wrote:
>> ProcessPendingWrites() calls ProcessRepliesIfAny() in the first place, so if it is possible that, a new COPY message is appended after the already-queued CommandComplete? Which seems to violate the protocol, but I am not sure if that would lead to any trouble.
>> 
>> So, maybe we need a new helper, say ProcessPendingWritesForShutdown(), that loops while pq_is_send_pending(), call WalSndCheckShutdownTimeout() and only wait for WL_SOCKET_WRITEABLE, then pq_flush_if_writable(), on flush failure, maybe WalSndShutdown().
> 
> Thanks for the review! You're right.
> 
> I added such loop directly in WalSndDone() instead of introducing a new helper
> function, since only WalSndDone() needs it and the extra loop does not make
> WalSndDone() significantly more complicated.
> 
> I've attached an updated patch.
> 
> Regards,
> 
> -- 
> Fujii Masao
> <v2-0001-Avoid-blocking-indefinitely-while-finishing-walse.patch>

Thanks for updating the patch. V2 looks good to me. “Make check-world” also passed from my side on MacOS.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  122. Re: Exit walsender before confirming remote flush in logical replication

    Fujii Masao <masao.fujii@gmail.com> — 2026-05-01T03:15:10Z

    On Thu, Apr 30, 2026 at 4:10 PM Chao Li <li.evan.chao@gmail.com> wrote:
> Thanks for updating the patch. V2 looks good to me. “Make check-world” also passed from my side on MacOS.

Thanks for the review! I've pushed the patch.

Regards,

-- 
Fujii Masao

  123. Re: Exit walsender before confirming remote flush in logical replication

    Alexander Lakhin <exclusion@gmail.com> — 2026-05-31T11:00:01Z

    Hello Fujii-san,

06.04.2026 18:32, Andres Freund wrote:
> Hi,
>
> On 2026-04-06 11:40:24 +0900, Fujii Masao wrote:
>> I've pushed the patch. Thanks!
> Failed on CI just now:
>
> https://cirrus-ci.com/task/6745359004729344?logs=test_world#L410
> https://api.cirrus-ci.com/v1/artifact/task/6745359004729344/testrun/build/testrun/subscription/038_walsnd_shutdown_timeout/log/regress_log_038_walsnd_shutdown_timeout

A recent failure occurred on the buildfarm [1] indicates another stability
issue, this time with the test itself, I guess:
[21:12:03.301](0.068s) ok 3 - have walreceiver pid 2275585
### Stopping node "publisher" using mode fast
# Running: pg_ctl --pgdata 
/home/bf/bf-build/tamandua/HEAD/pgsql.build/testrun/subscription/038_walsnd_shutdown_timeout/data/t_038_walsnd_shutdown_timeout_publisher_data/pgdata 
--mode fast stop
waiting for server to shut down.... done
server stopped
# No postmaster PID for node "publisher"
[21:12:03.752](0.452s) not ok 4 - walsender exits due to wal_sender_shutdown_timeout even when both physical and logical 
replication are stalled
[21:12:03.753](0.001s) #   Failed test 'walsender exits due to wal_sender_shutdown_timeout even when both physical and 
logical replication are stalled'
#   at /home/bf/bf-build/tamandua/HEAD/pgsql/src/test/subscription/t/038_walsnd_shutdown_timeout.pl line 189.
### Stopping node "subscriber" using mode fast

038_walsnd_shutdown_timeout_subscriber.log doesn't really contain the
expected warning:
2026-05-29 21:11:58.777 CEST [2273817][logical replication apply worker][124/2:0] LOG:  logical replication apply worker 
for subscription "test_sub" has started
2026-05-29 21:12:03.232 CEST [2271870][client backend][4/6:0] LOG: statement: BEGIN;
2026-05-29 21:12:03.232 CEST [2271870][client backend][4/6:0] LOG: statement: LOCK TABLE test_tab IN EXCLUSIVE MODE;
2026-05-29 21:12:03.232 CEST [2271870][client backend][4/6:0] LOG: statement: ;
2026-05-29 21:12:03.268 CEST [2273817][logical replication apply worker][124/0:0] LOG:  received message via 
replication: WARNING: replication slot "test_slot" specified in parameter "synchronized_standby_slots" does not have 
active_pid
     DETAIL:  Logical replication is waiting on the standby associated with replication slot "test_slot".
     HINT:  Start the standby associated with the replication slot "test_slot", or amend parameter 
"synchronized_standby_slots".
2026-05-29 21:12:03.433 CEST [2273817][logical replication apply worker][124/0:0] ERROR:  could not receive data from 
WAL stream: ERROR:  replication slot "test_slot" specified in parameter "synchronized_standby_slots" does not have 
active_pid
     DETAIL:  Logical replication is waiting on the standby associated with replication slot "test_slot".
     HINT:  Start the standby associated with the replication slot "test_slot", or amend parameter 
"synchronized_standby_slots".
2026-05-29 21:12:03.437 CEST [2271017][postmaster][:0] LOG: background worker "logical replication apply worker" (PID 
2273817) exited with exit code 1
2026-05-29 21:12:03.439 CEST [2275785][logical replication apply worker][125/2:0] LOG:  logical replication apply worker 
for subscription "test_sub" has started
2026-05-29 21:12:03.441 CEST [2275785][logical replication apply worker][125/0:0] ERROR:  apply worker for subscription 
"test_sub" could not connect to the publisher: connection to server on socket "/tmp/3gDgOpAsXL/.s.PGSQL.27739" failed: 
FATAL:  the database system is shutting down
2026-05-29 21:12:03.445 CEST [2271017][postmaster][:0] LOG: background worker "logical replication apply worker" (PID 
2275785) exited with exit code 1
2026-05-29 21:12:03.763 CEST [2271870][client backend][:0] LOG: disconnection: session time: 0:00:08.180 user=bf 
database=postgres host=[local]
2026-05-29 21:12:03.767 CEST [2271017][postmaster][:0] LOG: received fast shutdown request

I think this can be explained by the fact that walrcv->ready_to_display
is set before WalReceiverMain's loop reached. I've reproduced this test
failure with:
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -280,6 +280,7 @@ WalReceiverMain(const void *startup_data, size_t startup_data_len)
      walrcv->sender_port = 0;
      walrcv->ready_to_display = true;
      SpinLockRelease(&walrcv->mutex);
+pg_usleep(500 * 1000);

      /* Establish the connection to the primary for XLOG streaming */
      appname = cluster_name[0] ? cluster_name : "walreceiver";


Could you please look if this can be improved?

[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=tamandua&dt=2026-05-29%2019%3A03%3A21

Best regards,
Alexander

  124. RE: Exit walsender before confirming remote flush in logical replication

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2026-06-01T03:57:27Z

    Dear Alexander, Fujii-san,

> 038_walsnd_shutdown_timeout_subscriber.log doesn't really contain the
> expected warning:
> 2026-05-29 21:11:58.777 CEST [2273817][logical replication apply worker][124/2:0] LOG:  logical replication apply worker for subscription "test_sub" has started
> 2026-05-29 21:12:03.232 CEST [2271870][client backend][4/6:0] LOG:  statement: BEGIN;
> 2026-05-29 21:12:03.232 CEST [2271870][client backend][4/6:0] LOG:  statement: LOCK TABLE test_tab IN EXCLUSIVE MODE;
...

To confirm; IIUC the warning should be contained on the publisher log, not the
subscriber side. And below log appeared on the publisher;

```
2026-05-29 21:12:03.426 CEST [2275591][walsender][26/1:0] FATAL:  canceling authentication due to timeout
2026-05-29 21:12:03.432 CEST [2273580][checkpointer][:0] LOG:  shutting down
```

Is there a possibility that walsender was shut down during the authentication,
especially in-between BackendInitialize() and end of PerformAuthentication()?

> I think this can be explained by the fact that walrcv->ready_to_display
> is set before WalReceiverMain's loop reached. I've reproduced this test
> failure with:

Verified it could reproduce the failure, but there were no "canceling authentication
due to timeout" in the publisher log on my env.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

  125. Re: Exit walsender before confirming remote flush in logical replication

    Alexander Lakhin <exclusion@gmail.com> — 2026-06-01T20:00:00Z

    Dear Kuroda-san,

01.06.2026 06:57, Hayato Kuroda (Fujitsu) wrote:
>> 038_walsnd_shutdown_timeout_subscriber.log doesn't really contain the
>> expected warning:
>> 2026-05-29 21:11:58.777 CEST [2273817][logical replication apply worker][124/2:0] LOG:  logical replication apply worker for subscription "test_sub" has started
>> 2026-05-29 21:12:03.232 CEST [2271870][client backend][4/6:0] LOG:  statement: BEGIN;
>> 2026-05-29 21:12:03.232 CEST [2271870][client backend][4/6:0] LOG:  statement: LOCK TABLE test_tab IN EXCLUSIVE MODE;
> ...
>
> To confirm; IIUC the warning should be contained on the publisher log, not the
> subscriber side. And below log appeared on the publisher;
>
> ```
> 2026-05-29 21:12:03.426 CEST [2275591][walsender][26/1:0] FATAL:  canceling authentication due to timeout
> 2026-05-29 21:12:03.432 CEST [2273580][checkpointer][:0] LOG:  shutting down
> ```

I'm sorry. I switched to the wrong log file during my investigation.

>
> Is there a possibility that walsender was shut down during the authentication,
> especially in-between BackendInitialize() and end of PerformAuthentication()?
>
>> I think this can be explained by the fact that walrcv->ready_to_display
>> is set before WalReceiverMain's loop reached. I've reproduced this test
>> failure with:
> Verified it could reproduce the failure, but there were no "canceling authentication
> due to timeout" in the publisher log on my env.

I think the original failure can be reproduced with:
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -645,6 +645,7 @@ ClientAuthentication(Port *port)
  #endif
      }

+if (am_walsender) pg_usleep(500 * 1000);
      if ((log_connections & LOG_CONNECTION_AUTHENTICATION) &&
          status == STATUS_OK &&
          !MyClientConnectionInfo.authn_id)

With this modification, I can see:
#   Failed test 'walsender exits due to wal_sender_shutdown_timeout even when both physical and logical replication are 
stalled'
#   at t/038_walsnd_shutdown_timeout.pl line 189.
# Looks like you failed 1 test of 4.
t/038_walsnd_shutdown_timeout.pl .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/4 subtests

038_walsnd_shutdown_timeout_publisher.log contains:
2026-06-01 22:32:35.166 EEST [1851879][client backend][0/0:0] FATAL:  terminating connection due to administrator command
2026-06-01 22:32:35.166 EEST [1851879][client backend][0/0:0] DETAIL:  Signal sent by PID 1851819, UID 1000.
2026-06-01 22:32:35.166 EEST [1851878][walsender][26/1:0] FATAL: canceling authentication due to timeout

Best regards,
Alexander

  126. Re: Exit walsender before confirming remote flush in logical replication

    Andrey Silitskiy <a.silitskiy@postgrespro.ru> — 2026-06-03T10:32:30Z

    Dear Alexander, Kuroda-san,

IIUC, the problem is that the walreceiver does not reach the main loop
before it receives a signal and this leads to the absence of the required
log on publisher. So what do think of this fix?

Added a check to the test scenario that the walreceiver on standby was
fully initialized and replication started. With this fix, I can not
reproduce failure anymore.

Regards,
Andrey Silitskiy