Обсуждение: 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

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: http://postgr.es/m/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