Horton

Thread

Commits

  1. Fix signal handler setup for SIGHUP in the apply launcher process.

  2. Use PostgresSigHupHandler in more places.

  1. Have SIGHUP instead of SIGTERM for config reload in logical replication launcher

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2020-07-15T12:46:43Z

    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

  2. Re: Have SIGHUP instead of SIGTERM for config reload in logical replication launcher

    Dilip Kumar <dilipbalaut@gmail.com> — 2020-07-15T12:51:20Z

    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

  3. Re: Have SIGHUP instead of SIGTERM for config reload in logical replication launcher

    Amit Kapila <amit.kapila16@gmail.com> — 2020-07-15T13:16:19Z

    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

  4. Re: Have SIGHUP instead of SIGTERM for config reload in logical replication launcher

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2020-07-15T15:03:59Z

    >
> +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

  5. Re: Have SIGHUP instead of SIGTERM for config reload in logical replication launcher

    Andres Freund <andres@anarazel.de> — 2020-07-15T15:51:37Z

    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

  6. Re: Have SIGHUP instead of SIGTERM for config reload in logical replication launcher

    Robert Haas <robertmhaas@gmail.com> — 2020-07-16T15:14:28Z

    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

  7. Re: Have SIGHUP instead of SIGTERM for config reload in logical replication launcher

    Amit Kapila <amit.kapila16@gmail.com> — 2020-07-17T05:57:03Z

    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