Have SIGHUP instead of SIGTERM for config reload in logical replication launcher
Hi,
In ApplyLauncherMain, it seems like we are having SIGTERM signal
mapped for config reload. I think we should be having SIGHUP for
SignalHandlerForConfigReload(). Otherwise we miss to take the updated
value for wal_retrieve_retry_interval in ApplyLauncherMain.
Attached is a patch having this change.
Thoughts?
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
v1-Have-SIGHUP-for-config-reload-in-logical-replicat.patchapplication/octet-stream; name=v1-Have-SIGHUP-for-config-reload-in-logical-replicat.patchDownload
From fe7d136311b2ff60c8e144f4aed8eff886645e10 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Wed, 15 Jul 2020 18:09:12 +0530
Subject: [PATCH v1] Have SIGHUP for config reload in logical replication
launcher
Currently, SIGTERM is mapped to config reload. Have SIGHUP instead
of SIGTERM, to not miss the updated value for
wal_retrieve_retry_interval in ApplyLauncherMain.
---
src/backend/replication/logical/launcher.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c
index aec885e987..ff985b9b24 100644
--- a/src/backend/replication/logical/launcher.c
+++ b/src/backend/replication/logical/launcher.c
@@ -956,7 +956,7 @@ ApplyLauncherMain(Datum main_arg)
LogicalRepCtx->launcher_pid = MyProcPid;
/* Establish signal handlers. */
- pqsignal(SIGTERM, SignalHandlerForConfigReload);
+ pqsignal(SIGHUP, SignalHandlerForConfigReload);
pqsignal(SIGTERM, die);
BackgroundWorkerUnblockSignals();
--
2.25.1
On Wed, Jul 15, 2020 at 6:17 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
Hi,
In ApplyLauncherMain, it seems like we are having SIGTERM signal
mapped for config reload. I think we should be having SIGHUP for
SignalHandlerForConfigReload(). Otherwise we miss to take the updated
value for wal_retrieve_retry_interval in ApplyLauncherMain.Attached is a patch having this change.
Thoughts?
Yeah, it just looks like a typo so your fix looks good to me.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Wed, Jul 15, 2020 at 6:21 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Wed, Jul 15, 2020 at 6:17 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:Hi,
In ApplyLauncherMain, it seems like we are having SIGTERM signal
mapped for config reload. I think we should be having SIGHUP for
SignalHandlerForConfigReload(). Otherwise we miss to take the updated
value for wal_retrieve_retry_interval in ApplyLauncherMain.Attached is a patch having this change.
Thoughts?
Yeah, it just looks like a typo so your fix looks good to me.
+1. I will commit this tomorrow unless someone thinks otherwise.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
+1. I will commit this tomorrow unless someone thinks otherwise.
I think versions <= 12, have "pqsignal(SIGHUP,
logicalrep_launcher_sighup)", not sure why and which commit removed
logicalrep_launcher_sighup().
We might have to also backpatch this patch to version 13.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Hi,
On 2020-07-15 20:33:59 +0530, Bharath Rupireddy wrote:
+1. I will commit this tomorrow unless someone thinks otherwise.
I think versions <= 12, have "pqsignal(SIGHUP,
logicalrep_launcher_sighup)", not sure why and which commit removed
logicalrep_launcher_sighup().
commit 1e53fe0e70f610c34f4c9e770d108cd94151342c
Author: Robert Haas <rhaas@postgresql.org>
Date: 2019-12-17 13:03:57 -0500
Use PostgresSigHupHandler in more places.
There seems to be no reason for every background process to have
its own flag indicating that a config-file reload is needed.
Instead, let's just use ConfigFilePending for that purpose
everywhere.
Patch by me, reviewed by Andres Freund and Daniel Gustafsson.
Discussion: /messages/by-id/CA+TgmoZwDk=BguVDVa+qdA6SBKef=PKbaKDQALTC_9qoz1mJqg@mail.gmail.com
Indeed looks like a typo. Robert, do you concur?
Andres
On Wed, Jul 15, 2020 at 11:51 AM Andres Freund <andres@anarazel.de> wrote:
Indeed looks like a typo. Robert, do you concur?
Yes, that's definitely unintentional. Oops.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Thu, Jul 16, 2020 at 8:44 PM Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Jul 15, 2020 at 11:51 AM Andres Freund <andres@anarazel.de> wrote:
Indeed looks like a typo. Robert, do you concur?
Yes, that's definitely unintentional. Oops.
Pushed the fix.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com