Thread: libpqsrv_connect_params should call ProcessWalRcvInterrupts
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
Attachment
(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
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=0kB, 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
Attachment
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)
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
Attachment
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.
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
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
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
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
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
Attachment
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)
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)
Attachment
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
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
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
Attachment
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
I've just rebased patches and merged last two fix-commits (0003 and 0004) into 0002. -- regards Yura Sokolov aka funny-falcon
Attachment
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)