Horton

Thread

Commits

  1. Revert "libpqwalreceiver: Convert to libpq-be-fe-helpers.h"

  2. libpqwalreceiver: Convert to libpq-be-fe-helpers.h

  1. libpqsrv_connect_params should call ProcessWalRcvInterrupts

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2023-12-31T11:02:18Z

    Hello.

We've noticed that when walreceiver is waiting for a connection to
complete, standby does not immediately respond to promotion
requests. In PG14, upon receiving a promotion request, walreceiver
terminates instantly, but in PG16, it waits for connection
timeout. This behavior is attributed to commit 728f86fec65, where a
part of libpqrcv_connect was simply replaced with a call to
libpqsrc_connect_params. This behavior can be verified by simply
dropping packets from the standby to the primary.

By a simple thought, in walreceiver, libpqsrv_connect_internal could
just call ProcessWalRcvInterrupts() instead of CHECK_FOR_INTERRUPTS(),
but this approach is quite ugly. Since ProcessWalRcvInterrupts()
originally calls CHECK_FOR_INTERRUPTS() and there are no standalone
calls to CHECK_FOR_INTERRUPTS() within walreceiver, I think it might
be better to use ProcDiePending instead of ShutdownRequestPending.  I
added a subset function of die() as the SIGTERM handler in walsender
in a crude patch attached.

What do you think about the issue, and the approach?

If there are no issues or objections with this method, I will continue
to refine this patch. For now, I plan to register it for the upcoming
commitfest.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  2. Network failure may prevent promotion

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2023-12-31T11:07:41Z

    (Apology for resubmitting due to poor subject of the previous mail)
---
Hello.

We've noticed that when walreceiver is waiting for a connection to
complete, standby does not immediately respond to promotion
requests. In PG14, upon receiving a promotion request, walreceiver
terminates instantly, but in PG16, it waits for connection
timeout. This behavior is attributed to commit 728f86fec65, where a
part of libpqrcv_connect was simply replaced with a call to
libpqsrc_connect_params. This behavior can be verified by simply
dropping packets from the standby to the primary.

By a simple thought, in walreceiver, libpqsrv_connect_internal could
just call ProcessWalRcvInterrupts() instead of CHECK_FOR_INTERRUPTS(),
but this approach is quite ugly. Since ProcessWalRcvInterrupts()
originally calls CHECK_FOR_INTERRUPTS() and there are no standalone
calls to CHECK_FOR_INTERRUPTS() within walreceiver, I think it might
be better to use ProcDiePending instead of ShutdownRequestPending.  I
added a subset function of die() as the SIGTERM handler in walsender
in a crude patch attached.

What do you think about the issue, and the approach?

If there are no issues or objections with this method, I will continue
to refine this patch. For now, I plan to register it for the upcoming
commitfest.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  3. Re: Network failure may prevent promotion

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-01-18T08:26:31Z

    At Sun, 31 Dec 2023 20:07:41 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> We've noticed that when walreceiver is waiting for a connection to
> complete, standby does not immediately respond to promotion
> requests. In PG14, upon receiving a promotion request, walreceiver
> terminates instantly, but in PG16, it waits for connection
> timeout. This behavior is attributed to commit 728f86fec65, where a
> part of libpqrcv_connect was simply replaced with a call to
> libpqsrc_connect_params. This behavior can be verified by simply
> dropping packets from the standby to the primary.

Apologize for the inconvenience on my part, but I need to fix this
behavior. To continue this discussion, I'm providing a repro script
here.

With the script, the standby is expected to promote immediately,
emitting the following log lines:

standby.log:
> 2024-01-18 16:25:22.245 JST [31849] LOG:  received promote request
> 2024-01-18 16:25:22.245 JST [31850] FATAL:  terminating walreceiver process due to administrator command
> 2024-01-18 16:25:22.246 JST [31849] LOG:  redo is not required
> 2024-01-18 16:25:22.246 JST [31849] LOG:  selected new timeline ID: 2
> 2024-01-18 16:25:22.274 JST [31849] LOG:  archive recovery complete
> 2024-01-18 16:25:22.275 JST [31847] LOG:  checkpoint starting: force
> 2024-01-18 16:25:22.277 JST [31846] LOG:  database system is ready to accept connections
> 2024-01-18 16:25:22.280 JST [31847] LOG:  checkpoint complete: wrote 3 buffers (0.0%); 0 WAL file(s) added, 0 removed, 0 recycled; write=0.001 s, sync=0.001 s, total=0.005 s; sync files=2, longest=0.001 s, average=0.001 s; distance=0 kB, estimate=0 kB; lsn=0/1548E98, redo lsn=0/1548E40
> 2024-01-18 16:25:22.356 JST [31846] LOG:  received immediate shutdown request
> 2024-01-18 16:25:22.361 JST [31846] LOG:  database system is shut down

After 728f86fec65 was introduced, promotion does not complete with the
same operation, as follows. The patch attached to the previous mail
fixes this behavior to the old behavior above.

> 2024-01-18 16:47:53.314 JST [34515] LOG:  received promote request
> 2024-01-18 16:48:03.947 JST [34512] LOG:  received immediate shutdown request
> 2024-01-18 16:48:03.952 JST [34512] LOG:  database system is shut down

The attached script requires that sudo is executable. And there's
another point to note. The script attempts to establish a replication
connection to $primary_address:$primary_port. To packet-filter can
work, it must be a remote address that is accessible when no
packet-filter setting is set up.  The firewall-cmd setting, need to be
configured to block this connection. If simply an inaccessible IP
address is set, the process will fail immediately with a "No route to
host" error before the first packet is sent out, and it will not be
blocked as intended.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  4. Re: Network failure may prevent promotion

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-01-18T13:42:28Z

    On 18/01/2024 10:26, Kyotaro Horiguchi wrote:
> At Sun, 31 Dec 2023 20:07:41 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
>> We've noticed that when walreceiver is waiting for a connection to
>> complete, standby does not immediately respond to promotion
>> requests. In PG14, upon receiving a promotion request, walreceiver
>> terminates instantly, but in PG16, it waits for connection
>> timeout. This behavior is attributed to commit 728f86fec65, where a
>> part of libpqrcv_connect was simply replaced with a call to
>> libpqsrc_connect_params. This behavior can be verified by simply
>> dropping packets from the standby to the primary.
> 
> Apologize for the inconvenience on my part, but I need to fix this
> behavior. To continue this discussion, I'm providing a repro script
> here.

Thanks for script, I can repro this with it.

Given that commit 728f86fec6 that introduced this issue was not strictly 
required, perhaps we should just revert it for v16.

In your patch, there's one more stray reference to 
ProcessWalRcvInterrupts() in the comment above libpqrcv_PQexec. That 
makes me wonder, why didn't commit 728f86fec6 go all the way and also 
replace libpqrcv_PQexec and libpqrcv_PQgetResult with libpqsrv_exec and 
libpqsrv_get_result?

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  5. Re: Network failure may prevent promotion

    Michael Paquier <michael@paquier.xyz> — 2024-01-19T03:28:05Z

    On Thu, Jan 18, 2024 at 03:42:28PM +0200, Heikki Linnakangas wrote:
> Given that commit 728f86fec6 that introduced this issue was not strictly
> required, perhaps we should just revert it for v16.

Is there a point in keeping 728f86fec6 as well on HEAD?  That does not
strike me as wise to keep that in the tree for now.  If it needs to be
reworked, looking at this problem from scratch would be a safer
approach.
--
Michael

  6. Re: Network failure may prevent promotion

    Peter Smith <smithpb2250@gmail.com> — 2024-01-22T05:19:05Z

    2024-01 Commitfest.

Hi, This patch has a CF status of "Needs Review" [1], but it seems
there were CFbot test failures last time it was run [2]. Please have a
look and post an updated version if necessary.

======
[1] https://commitfest.postgresql.org/46/4748/
[2] https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/46/4748

Kind Regards,
Peter Smith.

  7. Re: Network failure may prevent promotion

    Fujii Masao <masao.fujii@gmail.com> — 2024-01-22T17:52:25Z

    On Thu, Jan 18, 2024 at 10:42 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> Given that commit 728f86fec6 that introduced this issue was not strictly
> required, perhaps we should just revert it for v16.

+1 for the revert.

This issue should be fixed in the upcoming minor release
since it might cause unexpected delays in failover times.

Regards,

-- 
Fujii Masao

  8. Re: Network failure may prevent promotion

    Andres Freund <andres@anarazel.de> — 2024-01-22T21:29:10Z

    Hi,

On 2024-01-19 12:28:05 +0900, Michael Paquier wrote:
> On Thu, Jan 18, 2024 at 03:42:28PM +0200, Heikki Linnakangas wrote:
> > Given that commit 728f86fec6 that introduced this issue was not strictly
> > required, perhaps we should just revert it for v16.
> 
> Is there a point in keeping 728f86fec6 as well on HEAD?  That does not
> strike me as wise to keep that in the tree for now.  If it needs to be
> reworked, looking at this problem from scratch would be a safer
> approach.

IDK, I think we'll introduce this type of bug over and over if we don't fix it
properly.

Greetings,

Andres Freund

  9. Re: Network failure may prevent promotion

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-01-23T04:23:20Z

    At Mon, 22 Jan 2024 13:29:10 -0800, Andres Freund <andres@anarazel.de> wrote in 
> Hi,
> 
> On 2024-01-19 12:28:05 +0900, Michael Paquier wrote:
> > On Thu, Jan 18, 2024 at 03:42:28PM +0200, Heikki Linnakangas wrote:
> > > Given that commit 728f86fec6 that introduced this issue was not strictly
> > > required, perhaps we should just revert it for v16.
> > 
> > Is there a point in keeping 728f86fec6 as well on HEAD?  That does not
> > strike me as wise to keep that in the tree for now.  If it needs to be
> > reworked, looking at this problem from scratch would be a safer
> > approach.
> 
> IDK, I think we'll introduce this type of bug over and over if we don't fix it
> properly.

Just to clarify my position, I thought that 728f86fec6 was heading the
right direction. Considering the current approach to signal handling
in walreceiver, I believed that it would be better to further
generalize in this direction rather than reverting. That's why I
proposed that patch.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  10. Re: Network failure may prevent promotion

    Fujii Masao <masao.fujii@gmail.com> — 2024-01-23T06:07:10Z

    On Tue, Jan 23, 2024 at 1:23 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Mon, 22 Jan 2024 13:29:10 -0800, Andres Freund <andres@anarazel.de> wrote in
> > Hi,
> >
> > On 2024-01-19 12:28:05 +0900, Michael Paquier wrote:
> > > On Thu, Jan 18, 2024 at 03:42:28PM +0200, Heikki Linnakangas wrote:
> > > > Given that commit 728f86fec6 that introduced this issue was not strictly
> > > > required, perhaps we should just revert it for v16.
> > >
> > > Is there a point in keeping 728f86fec6 as well on HEAD?  That does not
> > > strike me as wise to keep that in the tree for now.  If it needs to be
> > > reworked, looking at this problem from scratch would be a safer
> > > approach.
> >
> > IDK, I think we'll introduce this type of bug over and over if we don't fix it
> > properly.
>
> Just to clarify my position, I thought that 728f86fec6 was heading the
> right direction. Considering the current approach to signal handling
> in walreceiver, I believed that it would be better to further
> generalize in this direction rather than reverting. That's why I
> proposed that patch.

Regarding the patch, here are the review comments.

+/*
+ * Is current process a wal receiver?
+ */
+bool
+IsWalReceiver(void)
+{
+ return WalRcv != NULL;
+}

This looks wrong because WalRcv can be non-NULL in processes other
than walreceiver.

- pqsignal(SIGTERM, SignalHandlerForShutdownRequest); /* request shutdown */
+ pqsignal(SIGTERM, WalRcvShutdownSignalHandler); /* request shutdown */

Can't we just use die(), instead?

Regards,

-- 
Fujii Masao

  11. Re: Network failure may prevent promotion

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-01-23T08:24:10Z

    Thank you for looking this!

At Tue, 23 Jan 2024 15:07:10 +0900, Fujii Masao <masao.fujii@gmail.com> wrote in 
> Regarding the patch, here are the review comments.
> 
> +/*
> + * Is current process a wal receiver?
> + */
> +bool
> +IsWalReceiver(void)
> +{
> + return WalRcv != NULL;
> +}
> 
> This looks wrong because WalRcv can be non-NULL in processes other
> than walreceiver.

Mmm. Sorry for the silly mistake. We can use B_WAL_RECEIVER
instead. I'm not sure if the new function IsWalReceiver() is
required. The expression "MyBackendType == B_WAL_RECEIVER" is quite
descriptive. However, the function does make ProcessInterrupts() more
aligned regarding process types.

> - pqsignal(SIGTERM, SignalHandlerForShutdownRequest); /* request shutdown */
> + pqsignal(SIGTERM, WalRcvShutdownSignalHandler); /* request shutdown */
> 
> Can't we just use die(), instead?

There was a comment explaining the problems associated with exiting
within a signal handler;

- * Currently, only SIGTERM is of interest.  We can't just exit(1) within the
- * SIGTERM signal handler, because the signal might arrive in the middle of
- * some critical operation, like while we're holding a spinlock.  Instead, the

And I think we should keep the considerations it suggests. The patch
removes the comment itself, but it does so because it implements our
standard process exit procedure, which incorporates points suggested
by the now-removed comment.

--
Kyotaro Horiguchi
NTT Open Source Software Center

  12. Re: Network failure may prevent promotion

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-01-23T08:57:16Z

    On 23/01/2024 06:23, Kyotaro Horiguchi wrote:
> At Mon, 22 Jan 2024 13:29:10 -0800, Andres Freund <andres@anarazel.de> wrote in
>> Hi,
>>
>> On 2024-01-19 12:28:05 +0900, Michael Paquier wrote:
>>> On Thu, Jan 18, 2024 at 03:42:28PM +0200, Heikki Linnakangas wrote:
>>>> Given that commit 728f86fec6 that introduced this issue was not strictly
>>>> required, perhaps we should just revert it for v16.
>>>
>>> Is there a point in keeping 728f86fec6 as well on HEAD?  That does not
>>> strike me as wise to keep that in the tree for now.  If it needs to be
>>> reworked, looking at this problem from scratch would be a safer
>>> approach.
>>
>> IDK, I think we'll introduce this type of bug over and over if we don't fix it
>> properly.
> 
> Just to clarify my position, I thought that 728f86fec6 was heading the
> right direction. Considering the current approach to signal handling
> in walreceiver, I believed that it would be better to further
> generalize in this direction rather than reverting. That's why I
> proposed that patch.

I reverted commit 728f86fec6 from REL_16_STABLE and master.

I agree it was the right direction, so let's develop a complete patch, 
and re-apply it to master when we have the patch ready.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  13. Re: Network failure may prevent promotion

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-01-23T09:43:43Z

    On 23/01/2024 10:24, Kyotaro Horiguchi wrote:
> Thank you for looking this!
> 
> At Tue, 23 Jan 2024 15:07:10 +0900, Fujii Masao <masao.fujii@gmail.com> wrote in
>> Regarding the patch, here are the review comments.
>>
>> +/*
>> + * Is current process a wal receiver?
>> + */
>> +bool
>> +IsWalReceiver(void)
>> +{
>> + return WalRcv != NULL;
>> +}
>>
>> This looks wrong because WalRcv can be non-NULL in processes other
>> than walreceiver.
> 
> Mmm. Sorry for the silly mistake. We can use B_WAL_RECEIVER
> instead. I'm not sure if the new function IsWalReceiver() is
> required. The expression "MyBackendType == B_WAL_RECEIVER" is quite
> descriptive. However, the function does make ProcessInterrupts() more
> aligned regarding process types.

There's an existing AmWalReceiverProcess() macro too. Let's use that.

(See also 
https://www.postgresql.org/message-id/f3ecd4cb-85ee-4e54-8278-5fabfb3a4ed0%40iki.fi 
for refactoring in this area)

Here's a patch set summarizing the changes so far. They should be 
squashed, but I kept them separate for now to help with review:

1. revert the revert of 728f86fec6.
2. your walrcv_shutdown_deblocking_v2-2.patch
3. Also replace libpqrcv_PQexec() and libpqrcv_PQgetResult() with the 
wrappers from libpq-be-fe-helpers.h
4. Replace IsWalReceiver() with AmWalReceiverProcess()

>> - pqsignal(SIGTERM, SignalHandlerForShutdownRequest); /* request shutdown */
>> + pqsignal(SIGTERM, WalRcvShutdownSignalHandler); /* request shutdown */
>>
>> Can't we just use die(), instead?
> 
> There was a comment explaining the problems associated with exiting
> within a signal handler;
> 
> - * Currently, only SIGTERM is of interest.  We can't just exit(1) within the
> - * SIGTERM signal handler, because the signal might arrive in the middle of
> - * some critical operation, like while we're holding a spinlock.  Instead, the
> 
> And I think we should keep the considerations it suggests. The patch
> removes the comment itself, but it does so because it implements our
> standard process exit procedure, which incorporates points suggested
> by the now-removed comment.

die() doesn't call exit(1). Unless DoingCommandRead is set, but it never 
is in the walreceiver. It looks just like the new 
WalRcvShutdownSignalHandler() function. Am I missing something?

Hmm, but doesn't bgworker_die() have that problem with exit(1)ing in the 
signal handler?

I also wonder if we should replace SignalHandlerForShutdownRequest() 
completely with die(), in all processes? The difference is that 
SignalHandlerForShutdownRequest() uses ShutdownRequestPending, while 
die() uses ProcDiePending && InterruptPending to indicate that the 
signal was received. Or do some of the processes want to check for 
ShutdownRequestPending only at specific places, and don't want to get 
terminated at the any random CHECK_FOR_INTERRUPTS()?

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  14. Re: Network failure may prevent promotion

    Fujii Masao <masao.fujii@gmail.com> — 2024-01-24T11:29:07Z

    On Tue, Jan 23, 2024 at 6:43 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> There's an existing AmWalReceiverProcess() macro too. Let's use that.

+1

> Hmm, but doesn't bgworker_die() have that problem with exit(1)ing in the
> signal handler?

Yes, that's a problem. This issue was raised sometimes so far,
but has not been resolved yet.

> I also wonder if we should replace SignalHandlerForShutdownRequest()
> completely with die(), in all processes? The difference is that
> SignalHandlerForShutdownRequest() uses ShutdownRequestPending, while
> die() uses ProcDiePending && InterruptPending to indicate that the
> signal was received. Or do some of the processes want to check for
> ShutdownRequestPending only at specific places, and don't want to get
> terminated at the any random CHECK_FOR_INTERRUPTS()?

For example, checkpointer seems to want to handle a shutdown request
only when no other checkpoint is in progress because initiating a shutdown
checkpoint while another checkpoint is running could lead to issues.

Also I just wonder if even walreceiver can exit safely at any random
CHECK_FOR_INTERRUPTS()...

Regards,

-- 
Fujii Masao

  15. Re: Network failure may prevent promotion

    Fujii Masao <masao.fujii@gmail.com> — 2024-01-24T13:05:44Z

    On Wed, Jan 24, 2024 at 8:29 PM Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Tue, Jan 23, 2024 at 6:43 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> > There's an existing AmWalReceiverProcess() macro too. Let's use that.
>
> +1
>
> > Hmm, but doesn't bgworker_die() have that problem with exit(1)ing in the
> > signal handler?
>
> Yes, that's a problem. This issue was raised sometimes so far,
> but has not been resolved yet.
>
> > I also wonder if we should replace SignalHandlerForShutdownRequest()
> > completely with die(), in all processes? The difference is that
> > SignalHandlerForShutdownRequest() uses ShutdownRequestPending, while
> > die() uses ProcDiePending && InterruptPending to indicate that the
> > signal was received. Or do some of the processes want to check for
> > ShutdownRequestPending only at specific places, and don't want to get
> > terminated at the any random CHECK_FOR_INTERRUPTS()?
>
> For example, checkpointer seems to want to handle a shutdown request
> only when no other checkpoint is in progress because initiating a shutdown
> checkpoint while another checkpoint is running could lead to issues.

This my comment is not right... Sorry for noise.

Regards,

-- 
Fujii Masao

  16. Re: Network failure may prevent promotion

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-01-29T07:32:06Z

    Thank you fixing the issue.

At Tue, 23 Jan 2024 11:43:43 +0200, Heikki Linnakangas <hlinnaka@iki.fi> wrote i
n 
> There's an existing AmWalReceiverProcess() macro too. Let's use that.

Mmm. I sought an Is* function becuase "IsLogicalWorker()" is placed on
the previous line. Our convention regarding those functions (macros)
and variables seems inconsistent. However, I can't say for sure that
we should unify all of them.

> (See also
> https://www.postgresql.org/message-id/f3ecd4cb-85ee-4e54-8278-5fabfb3a4ed0%40iki.fi
> for refactoring in this area)
> 
> Here's a patch set summarizing the changes so far. They should be
> squashed, but I kept them separate for now to help with review:
> 
> 1. revert the revert of 728f86fec6.
> 2. your walrcv_shutdown_deblocking_v2-2.patch
> 3. Also replace libpqrcv_PQexec() and libpqrcv_PQgetResult() with the
> wrappers from libpq-be-fe-helpers.h

Both replacements look fine. I didn't find another instance of similar
code.

> 4. Replace IsWalReceiver() with AmWalReceiverProcess()

Just look fine.

> >> - pqsignal(SIGTERM, SignalHandlerForShutdownRequest); /* request
> >> - shutdown */
> >> + pqsignal(SIGTERM, WalRcvShutdownSignalHandler); /* request shutdown
> >> */
> >>
> >> Can't we just use die(), instead?
> > There was a comment explaining the problems associated with exiting
> > within a signal handler;
> > - * Currently, only SIGTERM is of interest.  We can't just exit(1) within
> > - * the
> > - * SIGTERM signal handler, because the signal might arrive in the middle
> > - * of
> > - * some critical operation, like while we're holding a spinlock.
> > - * Instead, the
> > And I think we should keep the considerations it suggests. The patch
> > removes the comment itself, but it does so because it implements our
> > standard process exit procedure, which incorporates points suggested
> > by the now-removed comment.
> 
> die() doesn't call exit(1). Unless DoingCommandRead is set, but it
> never is in the walreceiver. It looks just like the new
> WalRcvShutdownSignalHandler() function. Am I missing something?

Ugh.. Doesn't the name 'die()' suggest exit()?
I agree that die() can be used instad.

> Hmm, but doesn't bgworker_die() have that problem with exit(1)ing in
> the signal handler?

I noticed that but ignored for this time.

> I also wonder if we should replace SignalHandlerForShutdownRequest()
> completely with die(), in all processes? The difference is that
> SignalHandlerForShutdownRequest() uses ShutdownRequestPending, while
> die() uses ProcDiePending && InterruptPending to indicate that the
> signal was received. Or do some of the processes want to check for
> ShutdownRequestPending only at specific places, and don't want to get
> terminated at the any random CHECK_FOR_INTERRUPTS()?

At least, pg_log_backend_memory_context(<chkpt_pid>) causes a call to
ProcessInterrupts via "ereport(LOG_SERVER_ONLY" which can lead to an
exit due to ProcDiePending. In this regard, checkpointer clearly
requires the distinction.

Rather than merely consolidating the notification variables and
striving to annihilate CFI calls in the execution path, I
believe we need a shutdown mechanism that CFI doesn't react
to. However, as for the method to achieve this, whether we should keep
the notification variables separate as they are now, or whether it
would be better to introduce a variable that causes CFI to ignore
ProcDiePending, is a matter I think is open to discussion.

Attached patches are the rebased version of v3 (0003 is updated) and
additional 0005 that makes use of die() instead of walreceiver's
custom function.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  17. Re: Network failure may prevent promotion

    Robert Haas <robertmhaas@gmail.com> — 2024-05-14T14:16:00Z

    On Mon, Jan 29, 2024 at 2:32 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> [ new patch set ]

Hi,

I think it would be helpful to make it more clear exactly what's going
on here. It looks 0001 is intended to revert
21ef4d4d897563adb2f7920ad53b734950f1e0a4, which was itself a revert of
728f86fec65537eade8d9e751961782ddb527934, and then I guess the
remaining patches are to fix up issues created by that commit, but the
commit messages aren't meaningful so it's hard to understand what is
being fixed.

I think it would also be useful to clarify whether this is imagined to
be for master only, or something to be back-patched. In addition to
mentioning that here, it would be good to add that information to the
target version field of  https://commitfest.postgresql.org/48/4748/

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  18. Re: Network failure may prevent promotion

    Yura Sokolov <y.sokolov@postgrespro.ru> — 2025-03-21T13:24:16Z

    I've just rebased patches and merged last two fix-commits (0003 and 0004)
into 0002.

-- 
regards
Yura Sokolov aka funny-falcon

  19. Re: Network failure may prevent promotion

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-04-04T09:58:15Z

    On 21/03/2025 15:24, Yura Sokolov wrote:
> I've just rebased patches and merged last two fix-commits (0003 and 0004)
> into 0002.

Thanks for reviving this! I looked over this again and committed.

On May 14, 2024 at 14:16 Robert Haas <robertmhaas@gmail.com> wrote:
> I think it would be helpful to make it more clear exactly what's going
> on here. It looks 0001 is intended to revert
> 21ef4d4d897563adb2f7920ad53b734950f1e0a4, which was itself a revert of
> 728f86fec65537eade8d9e751961782ddb527934, and then I guess the
> remaining patches are to fix up issues created by that commit, but the
> commit messages aren't meaningful so it's hard to understand what is
> being fixed.

Yes, that's exactly what's going on here :-). I squashed the patches 
together and explained that in the commit message.

> I think it would also be useful to clarify whether this is imagined to
> be for master only, or something to be back-patched. In addition to
> mentioning that here, it would be good to add that information to the
> target version field of  https://commitfest.postgresql.org/48/4748/

A lot of time and v17 release passed since you wrote this, but for sake 
of completeness: This is for v18 only. It's not a bug fix. There was an 
bug with the original commit 728f86fec6, which was resolved by reverting 
it in v16 and v17. This commit brings it back in v17, hopefully bug-free 
this time.

-- 
Heikki Linnakangas
Neon (https://neon.tech)