Thread: Have SIGHUP instead of SIGTERM for config reload in logical replication launcher
Have SIGHUP instead of SIGTERM for config reload in logical replication launcher
From
Bharath Rupireddy
Date:
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
Attachment
Re: Have SIGHUP instead of SIGTERM for config reload in logical replication launcher
From
Dilip Kumar
Date:
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
Re: Have SIGHUP instead of SIGTERM for config reload in logical replication launcher
From
Amit Kapila
Date:
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
Re: Have SIGHUP instead of SIGTERM for config reload in logical replication launcher
From
Bharath Rupireddy
Date:
> > +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
Re: Have SIGHUP instead of SIGTERM for config reload in logical replication launcher
From
Andres Freund
Date:
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: http://postgr.es/m/CA+TgmoZwDk=BguVDVa+qdA6SBKef=PKbaKDQALTC_9qoz1mJqg@mail.gmail.com
Indeed looks like a typo. Robert, do you concur?
Andres
Re: Have SIGHUP instead of SIGTERM for config reload in logical replication launcher
From
Robert Haas
Date:
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
Re: Have SIGHUP instead of SIGTERM for config reload in logical replication launcher
From
Amit Kapila
Date:
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