Use standard SIGHUP and SIGTERM handlers in autoprewarm module

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
34 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Use standard SIGHUP and SIGTERM handlers in autoprewarm module

Bharath Rupireddy
Hi,

Autoprewarm module is using it's own SIGHUP(apw_sigterm_handler, got_sigterm) and SIGTERM(apw_sighup_handler, got_sighup) handlers which are similar to standard signal handlers(except for a difference [1]). Isn't it good to remove them and  use standard SignalHandlerForConfigReload and SignalHandlerForShutdownRequest?

Attaching the patch for the above changes.

Looks like the commit[2] replaced custom handlers with standard handlers.

Thoughts?

[1] apw_sigterm_handler() and apw_sighup_handler() use MyProc->procLatch
if (MyProc)
        SetLatch(&MyProc->procLatch);
where as standard handlers use MyLatch
    SetLatch(MyLatch);
Both MyProc->procLatch and MyLatch point to same, see comment from global.c
/*
 * MyLatch points to the latch that should be used for signal handling by the
 * current process. It will either point to a process local latch if the
 * current process does not have a PGPROC entry in that moment, or to
 * PGPROC->procLatch if it has. Thus it can always be used in signal handlers,
 * without checking for its existence.

 */
struct Latch *MyLatch;

(gdb) p MyProc->procLatch
$6 = {is_set = 0, is_shared = true, owner_pid = 1448807}
(gdb) p MyLatch
$7 = (struct Latch *) 0x7fcacc6d902c
(gdb) p &MyProc->procLatch
$8 = (Latch *) 0x7fcacc6d902c
(gdb) p *MyLatch
$9 = {is_set = 0, is_shared = true, owner_pid = 1448807}

[2] commit 1e53fe0e70f610c34f4c9e770d108cd94151342c
Author: Robert Haas <[hidden email]>
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.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

v1-Use-Standard-SIGTERM-SIGHUP-Handlers-In-AutoPrewarm-Module.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module

Fujii Masao-4


On 2020/10/05 19:45, Bharath Rupireddy wrote:
> Hi,
>
> Autoprewarm module is using it's own SIGHUP(apw_sigterm_handler, got_sigterm) and SIGTERM(apw_sighup_handler, got_sighup) handlers which are similar to standard signal handlers(except for a difference [1]). Isn't it good to remove them and  use standard SignalHandlerForConfigReload and SignalHandlerForShutdownRequest?
>
> Attaching the patch for the above changes.
>
> Looks like the commit[2] replaced custom handlers with standard handlers.
>
> Thoughts?

+1

The patch looks good to me.

ISTM that we can also replace StartupProcSigHupHandler() in startup.c
with SignalHandlerForConfigReload() by making the startup process use
the general shared latch instead of its own one. POC patch attached.
Thought?

Probably we can also replace sigHupHandler() in syslogger.c with
SignalHandlerForConfigReload()? This would be separate patch, though.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

make_startup_use_general_shared_latch.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module

Bharath Rupireddy
On Mon, Oct 5, 2020 at 8:04 PM Fujii Masao <[hidden email]> wrote:

>
> On 2020/10/05 19:45, Bharath Rupireddy wrote:
> > Hi,
> >
> > Autoprewarm module is using it's own SIGHUP(apw_sigterm_handler, got_sigterm) and SIGTERM(apw_sighup_handler, got_sighup) handlers which are similar to standard signal handlers(except for a difference [1]). Isn't it good to remove them and  use standard SignalHandlerForConfigReload and SignalHandlerForShutdownRequest?
> >
> > Attaching the patch for the above changes.
> >
> > Looks like the commit[2] replaced custom handlers with standard handlers.
> >
> > Thoughts?
>
> +1
>
> The patch looks good to me.
>

Thanks.

>
> ISTM that we can also replace StartupProcSigHupHandler() in startup.c
> with SignalHandlerForConfigReload() by making the startup process use
> the general shared latch instead of its own one. POC patch attached.
> Thought?
>

I'm not quite sure whether it breaks something or not. I see that
WakeupRecovery() with XLogCtl->recoveryWakeupLatch latch from the
startup process is also being used in the walreceiver process. I may
be wrong, but have some concern if the behaviour is different in case
of EXEC_BACKEND and Windows?

Another concern is that we are always using
XLogCtl->recoveryWakeupLatch in shared mode, this makes sense as this
latch is also being used in walrecevier. But sometimes, MyLatch is
created in non-shared mode as well(see InitLatch(MyLatch)).

Others may have better thoughts though.

>
> Probably we can also replace sigHupHandler() in syslogger.c with
> SignalHandlerForConfigReload()? This would be separate patch, though.
>

+1 to replace sigHupHandler() with SignalHandlerForConfigReload() as
the latch and the functionality are pretty much the same.

WalReceiverMai(): I think we can also replace WalRcvShutdownHandler()
with SignalHandlerForShutdownRequest() because walrcv->latch point to
&MyProc->procLatch which in turn point to MyLatch.

Thoughts? If okay, we can combine these into a single patch. I will
post an updated patch soon.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module

Fujii Masao-4


On 2020/10/06 1:18, Bharath Rupireddy wrote:

> On Mon, Oct 5, 2020 at 8:04 PM Fujii Masao <[hidden email]> wrote:
>>
>> On 2020/10/05 19:45, Bharath Rupireddy wrote:
>>> Hi,
>>>
>>> Autoprewarm module is using it's own SIGHUP(apw_sigterm_handler, got_sigterm) and SIGTERM(apw_sighup_handler, got_sighup) handlers which are similar to standard signal handlers(except for a difference [1]). Isn't it good to remove them and  use standard SignalHandlerForConfigReload and SignalHandlerForShutdownRequest?
>>>
>>> Attaching the patch for the above changes.
>>>
>>> Looks like the commit[2] replaced custom handlers with standard handlers.
>>>
>>> Thoughts?
>>
>> +1
>>
>> The patch looks good to me.
>>
>
> Thanks.
>
>>
>> ISTM that we can also replace StartupProcSigHupHandler() in startup.c
>> with SignalHandlerForConfigReload() by making the startup process use
>> the general shared latch instead of its own one. POC patch attached.
>> Thought?
>>
>
> I'm not quite sure whether it breaks something or not. I see that
> WakeupRecovery() with XLogCtl->recoveryWakeupLatch latch from the
> startup process is also being used in the walreceiver process. I may
> be wrong, but have some concern if the behaviour is different in case
> of EXEC_BACKEND and Windows?

Unless I'm wrong, regarding MyLatch, the behavior is not different
whether in EXEC_BACKEND or not. In both cases, SwitchToSharedLatch()
is called and MyLatch is set to the shared latch, i.e., MyProc->procLatch.


>
> Another concern is that we are always using
> XLogCtl->recoveryWakeupLatch in shared mode, this makes sense as this
> latch is also being used in walrecevier. But sometimes, MyLatch is
> created in non-shared mode as well(see InitLatch(MyLatch)).

Yes, and then the startup process calls SwitchToSharedLatch() and
repoint MyLatch to the shared one.

>
> Others may have better thoughts though.

Okay, I will reconsider the patch and post it separately later if necessary.


>
>>
>> Probably we can also replace sigHupHandler() in syslogger.c with
>> SignalHandlerForConfigReload()? This would be separate patch, though.
>>
>
> +1 to replace sigHupHandler() with SignalHandlerForConfigReload() as
> the latch and the functionality are pretty much the same.
>
> WalReceiverMai(): I think we can also replace WalRcvShutdownHandler()
> with SignalHandlerForShutdownRequest() because walrcv->latch point to
> &MyProc->procLatch which in turn point to MyLatch.
>
> Thoughts? If okay, we can combine these into a single patch. I will
> post an updated patch soon.

+1 Or it's also ok to make each patch separately.
Anyway, thanks!

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION


Reply | Threaded
Open this post in threaded view
|

Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module

Michael Paquier-2
In reply to this post by Fujii Masao-4
On Mon, Oct 05, 2020 at 11:34:05PM +0900, Fujii Masao wrote:
> ISTM that we can also replace StartupProcSigHupHandler() in startup.c
> with SignalHandlerForConfigReload() by making the startup process use
> the general shared latch instead of its own one. POC patch attached.
> Thought?

That looks good to me.  Nice cleanup.

> Probably we can also replace sigHupHandler() in syslogger.c with
> SignalHandlerForConfigReload()? This would be separate patch, though.

+1.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module

Bharath Rupireddy
In reply to this post by Fujii Masao-4
On Tue, Oct 6, 2020 at 11:20 AM Fujii Masao <[hidden email]> wrote:

>
> >>
> >> Probably we can also replace sigHupHandler() in syslogger.c with
> >> SignalHandlerForConfigReload()? This would be separate patch, though.
> >>
> >
> > +1 to replace sigHupHandler() with SignalHandlerForConfigReload() as
> > the latch and the functionality are pretty much the same.
> >
> > WalReceiverMai(): I think we can also replace WalRcvShutdownHandler()
> > with SignalHandlerForShutdownRequest() because walrcv->latch point to
> > &MyProc->procLatch which in turn point to MyLatch.
> >
> > Thoughts? If okay, we can combine these into a single patch. I will
> > post an updated patch soon.
>
> +1 Or it's also ok to make each patch separately.
> Anyway, thanks!
>

Thanks. +1 to have separate patches. I will post two separate patches
for autoprewarm and WalRcvShutdownHandler for further review. The
other two patches can be for startup.c and syslogger.c.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module

Bharath Rupireddy
On Tue, Oct 6, 2020 at 11:41 AM Bharath Rupireddy
<[hidden email]> wrote:

>
> On Tue, Oct 6, 2020 at 11:20 AM Fujii Masao <[hidden email]> wrote:
> >
> > +1 Or it's also ok to make each patch separately.
> > Anyway, thanks!
> >
>
> Thanks. +1 to have separate patches. I will post two separate patches
> for autoprewarm and WalRcvShutdownHandler for further review. The
> other two patches can be for startup.c and syslogger.c.
>
I'm attaching all the 4 patches here together.

1. v1-use-standard-SIGHUP-and-SIGTERM-handlers-in-autoprewarm-process.patch
--  This is the patch initially sent in this mail by me, I just
renamed it.
2. v1-use-MyLatch-and-standard-SIGHUP-handler-in-startup-process.patch
-- This is the patch proposed by Fuji Masao, I just renamed it.
3. v1-use-standard-SIGHUP-hanlder-in-syslogger-process.patch
4. v1-use-standard-SIGTERM-handler-in-walreceiver-process.patch

Please consider these patches for further review.


With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

v1-use-standard-SIGHUP-and-SIGTERM-handlers-in-autoprewarm-process.patch (3K) Download Attachment
v1-use-MyLatch-and-standard-SIGHUP-handler-in-startup-process.patch (8K) Download Attachment
v1-use-standard-SIGHUP-hanlder-in-syslogger-process.patch (3K) Download Attachment
v1-use-standard-SIGTERM-handler-in-walreceiver-process.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module

Bharath Rupireddy
On Wed, Oct 7, 2020 at 8:00 AM Bharath Rupireddy
<[hidden email]> wrote:

>
> On Tue, Oct 6, 2020 at 11:41 AM Bharath Rupireddy
> <[hidden email]> wrote:
> >
> > On Tue, Oct 6, 2020 at 11:20 AM Fujii Masao <[hidden email]> wrote:
> > >
> > > +1 Or it's also ok to make each patch separately.
> > > Anyway, thanks!
> > >
> >
> > Thanks. +1 to have separate patches. I will post two separate patches
> > for autoprewarm and WalRcvShutdownHandler for further review. The
> > other two patches can be for startup.c and syslogger.c.
> >
>
> I'm attaching all the 4 patches here together.
>
> 1. v1-use-standard-SIGHUP-and-SIGTERM-handlers-in-autoprewarm-process.patch
> --  This is the patch initially sent in this mail by me, I just
> renamed it.
> 2. v1-use-MyLatch-and-standard-SIGHUP-handler-in-startup-process.patch
> -- This is the patch proposed by Fuji Masao, I just renamed it.
> 3. v1-use-standard-SIGHUP-hanlder-in-syslogger-process.patch
> 4. v1-use-standard-SIGTERM-handler-in-walreceiver-process.patch
>
> Please consider these patches for further review.
>

Added this to the commitfest for further review.

https://commitfest.postgresql.org/30/2756/



With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module

Craig Ringer-5


On Wed, Oct 7, 2020 at 8:39 PM Bharath Rupireddy <[hidden email]> wrote:
On Wed, Oct 7, 2020 at 8:00 AM Bharath Rupireddy
<[hidden email]> wrote:
>
> On Tue, Oct 6, 2020 at 11:41 AM Bharath Rupireddy
> <[hidden email]> wrote:
> >
> > On Tue, Oct 6, 2020 at 11:20 AM Fujii Masao <[hidden email]> wrote:
> > >
> > > +1 Or it's also ok to make each patch separately.
> > > Anyway, thanks!
> > >
> >
> > Thanks. +1 to have separate patches. I will post two separate patches
> > for autoprewarm and WalRcvShutdownHandler for further review. The
> > other two patches can be for startup.c and syslogger.c.


[ CC'd Robert Haas since he's the main name in interrupt.c, test_shm_mq/worker.c,  ]

src/test/modules/test_shm_mq/worker.c appears to do the right thing the wrong way - it has its own custom handler instead of using die() or SignalHandlerForShutdownRequest().

In contrast  src/test/modules/worker_spi/worker_spi.c looks plain wrong. Especially since it's quoted as an example of how to do things right. It won't respond to SIGTERM at all while it's executing a query from its queue, no matter how long that query takes or whether it blocks. It can inhibit even postmaster shutdown as a result.

I was going to lob off a quick patch to fix this by making both use quickdie() for SIGQUIT and die() for SIGTERM, but after reading for a bit I'm no longer sure what the right choice even is. I'd welcome some opinions.


The problem is that but interrupt.c and interrupt.h actually define and recommend different and simpler handlers for these jobs - ones that don't actually work properly for code that calls into Pg core and might rely on CHECK_FOR_INTERRUPTS(), InterruptsPending and ProcDiePending to properly respect SIGTERM.

And to add to the confusion the bgworker infra adds its own different default SIGTERM handler bgworker_die() that's weirdly in-between the interrupt.c and postgres.c signal handling.

So I'm no longer sure how the example code should even be fixed. I'm not convinced everyone using die() and quickdie() is good given they currently seem to be assumed to be mainly for user backends. Maybe wwe should move them to interrupt.c along with CHECK_FOR_INTERRUPTS(), ProcessInterrupts, etc and document them as for all database-connected or shmem-connected backends to use.

So in the medium term, interrupt.c's SignalHandlerForShutdownRequest() and SignalHandlerForCrashExit() should be combined with die() and quickdie(), integrating properly with CHECK_FOR_INTERRUPTS(), ProcessInterrupts(), etc. We can add a hook we call before we proc_exit() in response to ProcDiePending so backends can choose to mask it if there's a period during which they wish to defer responding to SIGTERM, but by default everything will respect SIGTERM -> die() sets ProcDiePending -> CHECK_FOR_INTERRUPTS() -> ProcessInterrupts() -> proc_exit() . interrupt.c's SignalHandlerForCrashExit() and SignalHandlerForShutdownRequest() become deprecated/legacy. We add a separate temporary handler that's installed by init.c for early SIGQUIT handling but document it as to be replaced after backends start properly. We'd delete the bgw-specific signal handlers and install die() and procdie() instead during StartBackgroundWorker - at least if the bgw is connecting to shmem or a database. interrupt.c's HandleMainLoopInterrupts() could be static inlined, and adopted in the bgworker examples and all the other places that currently do ConfigReloadPending / ProcessConfigFile() etc themselves.

It wouldn't be a clean sweep of consistent signal handling, given all the funky stuff we have in the checkpointer, walsender, etc. But I think it might help...

(And maybe I could even combine the various am_foo and is_bar globals we use to identify different sorts of backend, while doing such cleanups).
Reply | Threaded
Open this post in threaded view
|

Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module

Fujii Masao-4
In reply to this post by Bharath Rupireddy


On 2020/10/07 11:30, Bharath Rupireddy wrote:

> On Tue, Oct 6, 2020 at 11:41 AM Bharath Rupireddy
> <[hidden email]> wrote:
>>
>> On Tue, Oct 6, 2020 at 11:20 AM Fujii Masao <[hidden email]> wrote:
>>>
>>> +1 Or it's also ok to make each patch separately.
>>> Anyway, thanks!
>>>
>>
>> Thanks. +1 to have separate patches. I will post two separate patches
>> for autoprewarm and WalRcvShutdownHandler for further review. The
>> other two patches can be for startup.c and syslogger.c.
>>
>
> I'm attaching all the 4 patches here together.
>
> 1. v1-use-standard-SIGHUP-and-SIGTERM-handlers-in-autoprewarm-process.patch
> --  This is the patch initially sent in this mail by me, I just
> renamed it.
> 2. v1-use-MyLatch-and-standard-SIGHUP-handler-in-startup-process.patch
> -- This is the patch proposed by Fuji Masao, I just renamed it.
> 3. v1-use-standard-SIGHUP-hanlder-in-syslogger-process.patch
> 4. v1-use-standard-SIGTERM-handler-in-walreceiver-process.patch
>
> Please consider these patches for further review.

Thanks for the patches! They look good to me. So barring any objections,
I will commit them one by one.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION


Reply | Threaded
Open this post in threaded view
|

Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module

Fujii Masao-4


On 2020/10/29 15:21, Fujii Masao wrote:

>
>
> On 2020/10/07 11:30, Bharath Rupireddy wrote:
>> On Tue, Oct 6, 2020 at 11:41 AM Bharath Rupireddy
>> <[hidden email]> wrote:
>>>
>>> On Tue, Oct 6, 2020 at 11:20 AM Fujii Masao <[hidden email]> wrote:
>>>>
>>>> +1 Or it's also ok to make each patch separately.
>>>> Anyway, thanks!
>>>>
>>>
>>> Thanks. +1 to have separate patches. I will post two separate patches
>>> for autoprewarm and WalRcvShutdownHandler for further review. The
>>> other two patches can be for startup.c and syslogger.c.
>>>
>>
>> I'm attaching all the 4 patches here together.
>>
>> 1. v1-use-standard-SIGHUP-and-SIGTERM-handlers-in-autoprewarm-process.patch
>> --  This is the patch initially sent in this mail by me, I just
>> renamed it.
>> 2. v1-use-MyLatch-and-standard-SIGHUP-handler-in-startup-process.patch
>> -- This is the patch proposed by Fuji Masao, I just renamed it.
>> 3. v1-use-standard-SIGHUP-hanlder-in-syslogger-process.patch
>> 4. v1-use-standard-SIGTERM-handler-in-walreceiver-process.patch
>>
>> Please consider these patches for further review.
>
> Thanks for the patches! They look good to me. So barring any objections,
> I will commit them one by one.
I pushed the following two patches.

- v1-use-standard-SIGHUP-hanlder-in-syslogger-process.patch
- v1-use-MyLatch-and-standard-SIGHUP-handler-in-startup-process.patch

Regarding other two patches, I think that it's better to use MyLatch
rather than MyProc->procLatch or walrcv->latch in WaitLatch() and
ResetLatch(), like other code does. Attached are the updated versions
of the patches. Thought?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

v2-use-standard-SIGHUP-and-SIGTERM-handlers-in-autoprewarm-process.patch (3K) Download Attachment
v2-use-standard-SIGTERM-handler-in-walreceiver-process.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module

Bharath Rupireddy
On Wed, Nov 4, 2020 at 2:36 PM Fujii Masao <[hidden email]> wrote:
>
> Regarding other two patches, I think that it's better to use MyLatch
> rather than MyProc->procLatch or walrcv->latch in WaitLatch() and
> ResetLatch(), like other code does. Attached are the updated versions
> of the patches. Thought?
>

+1 for replacing MyProc->procLatch with MyLatch in the autoprewarm
module, and the patch looks good to me.

I'm not quite sure to replace all the places in the walreceiver
process, for instance in WalRcvForceReply() we are using spinlock to
acquire the latch pointer and . Others may have better thoughts on
this.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module

Kyotaro Horiguchi-4
At Wed, 4 Nov 2020 21:16:29 +0530, Bharath Rupireddy <[hidden email]> wrote in

> On Wed, Nov 4, 2020 at 2:36 PM Fujii Masao <[hidden email]> wrote:
> >
> > Regarding other two patches, I think that it's better to use MyLatch
> > rather than MyProc->procLatch or walrcv->latch in WaitLatch() and
> > ResetLatch(), like other code does. Attached are the updated versions
> > of the patches. Thought?
> >
>
> +1 for replacing MyProc->procLatch with MyLatch in the autoprewarm
> module, and the patch looks good to me.

Looks good to me, too.

> I'm not quite sure to replace all the places in the walreceiver
> process, for instance in WalRcvForceReply() we are using spinlock to
> acquire the latch pointer and . Others may have better thoughts on
> this.

The SIGTERM part looks good. The only difference between
WalRcvSigHupHandler and SignalHandlerForConfigReload is whether latch
is set or not.  I don't think it's a problem that config-reload causes
an extra wakeup.  Couldn't we do the same thing for SIGHUP?

We might even be able to reload config file in
ProcessWalRcvInterrupts().

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center


Reply | Threaded
Open this post in threaded view
|

Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module

Fujii Masao-4


On 2020/11/05 12:12, Kyotaro Horiguchi wrote:

> At Wed, 4 Nov 2020 21:16:29 +0530, Bharath Rupireddy <[hidden email]> wrote in
>> On Wed, Nov 4, 2020 at 2:36 PM Fujii Masao <[hidden email]> wrote:
>>>
>>> Regarding other two patches, I think that it's better to use MyLatch
>>> rather than MyProc->procLatch or walrcv->latch in WaitLatch() and
>>> ResetLatch(), like other code does. Attached are the updated versions
>>> of the patches. Thought?
>>>
>>
>> +1 for replacing MyProc->procLatch with MyLatch in the autoprewarm
>> module, and the patch looks good to me.
>
> Looks good to me, too.

Thanks for the review! I pushed the patch.

>
>> I'm not quite sure to replace all the places in the walreceiver
>> process, for instance in WalRcvForceReply() we are using spinlock to
>> acquire the latch pointer and . Others may have better thoughts on
>> this.
>
> The SIGTERM part looks good. The only difference between
> WalRcvSigHupHandler and SignalHandlerForConfigReload is whether latch
> is set or not.  I don't think it's a problem that config-reload causes
> an extra wakeup.  Couldn't we do the same thing for SIGHUP?

I agree that we can use even standard SIGHUP handler in walreceiver.
I'm not sure why SetLatch() was not called in walreceiver's SIGHUP
handler so far.

Regards,


--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION


Reply | Threaded
Open this post in threaded view
|

Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module

Bharath Rupireddy
On Fri, Nov 6, 2020 at 11:00 PM Fujii Masao <[hidden email]> wrote:

>
> >> I'm not quite sure to replace all the places in the walreceiver
> >> process, for instance in WalRcvForceReply() we are using spinlock to
> >> acquire the latch pointer and . Others may have better thoughts on
> >> this.
> >
> > The SIGTERM part looks good. The only difference between
> > WalRcvSigHupHandler and SignalHandlerForConfigReload is whether latch
> > is set or not.  I don't think it's a problem that config-reload causes
> > an extra wakeup.  Couldn't we do the same thing for SIGHUP?
>
> I agree that we can use even standard SIGHUP handler in walreceiver.
> I'm not sure why SetLatch() was not called in walreceiver's SIGHUP
> handler so far.
>

The main reason for having SetLatch() in
SignalHandlerForConfigReload() is to wake up the calling process if
waiting in WaitLatchOrSocket() or WaitLatch() and reload the new
config file and use the reloaded config variables. Maybe we should
give a thought on the scenarios in which the walreceiver process
waits, and what happens in case the latch is set when SIGHUP is
received.

And also, I think it's worth having a look at the commit 40f908bdcdc7
that introduced WalRcvSigHupHandler() and 597a87ccc that replaced
custom latch with procLatch.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module

Kyotaro Horiguchi-4
At Sat, 7 Nov 2020 19:31:21 +0530, Bharath Rupireddy <[hidden email]> wrote in
> The main reason for having SetLatch() in
> SignalHandlerForConfigReload() is to wake up the calling process if
> waiting in WaitLatchOrSocket() or WaitLatch() and reload the new
> config file and use the reloaded config variables. Maybe we should
> give a thought on the scenarios in which the walreceiver process
> waits, and what happens in case the latch is set when SIGHUP is
> received.

The difference is whether the config file is processed at the next
wakeup (by force-reply-request or SIGTERM) of walreceiver or
immediately. If server-reload happened frequently, say, several times
per second(?), we should consider to reduce the useless reloading, but
actually that's not the case.

> And also, I think it's worth having a look at the commit 40f908bdcdc7
> that introduced WalRcvSigHupHandler() and 597a87ccc that replaced
> custom latch with procLatch.

At the time of the first patch, PostgreSQL processes used arbitrarily
implemented SIGHUP handlers for their own so it is natural that
walreceiver used its own one.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center


Reply | Threaded
Open this post in threaded view
|

Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module

Fujii Masao-4


On 2020/11/10 16:17, Kyotaro Horiguchi wrote:

> At Sat, 7 Nov 2020 19:31:21 +0530, Bharath Rupireddy <[hidden email]> wrote in
>> The main reason for having SetLatch() in
>> SignalHandlerForConfigReload() is to wake up the calling process if
>> waiting in WaitLatchOrSocket() or WaitLatch() and reload the new
>> config file and use the reloaded config variables. Maybe we should
>> give a thought on the scenarios in which the walreceiver process
>> waits, and what happens in case the latch is set when SIGHUP is
>> received.
>
> The difference is whether the config file is processed at the next
> wakeup (by force-reply-request or SIGTERM) of walreceiver or
> immediately. If server-reload happened frequently, say, several times
> per second(?), we should consider to reduce the useless reloading, but
> actually that's not the case.
So, attached is the patch that makes walreceiver use both standard
SIGTERM and SIGHUP handlers. Currently I've not found any actual
issues by making walreceiver use standard SIGHUP handler, yet.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

v3-use-standard-SIGTERM-and-SIGHUP-handlers-in-walreceiver-process.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module

Bharath Rupireddy
On Tue, Nov 10, 2020 at 3:04 PM Fujii Masao <[hidden email]> wrote:

>
> >> The main reason for having SetLatch() in
> >> SignalHandlerForConfigReload() is to wake up the calling process if
> >> waiting in WaitLatchOrSocket() or WaitLatch() and reload the new
> >> config file and use the reloaded config variables. Maybe we should
> >> give a thought on the scenarios in which the walreceiver process
> >> waits, and what happens in case the latch is set when SIGHUP is
> >> received.
> >
> > The difference is whether the config file is processed at the next
> > wakeup (by force-reply-request or SIGTERM) of walreceiver or
> > immediately. If server-reload happened frequently, say, several times
> > per second(?), we should consider to reduce the useless reloading, but
> > actually that's not the case.
>
> So, attached is the patch that makes walreceiver use both standard
> SIGTERM and SIGHUP handlers. Currently I've not found any actual
> issues by making walreceiver use standard SIGHUP handler, yet.
>

I think it makes sense to replace WalRcv->latch with
MyProc->procLatch(as they point to the same latch) in the functions
that are called in the walreceiver process. However, we are using
walrcv->latch with spinlock, say in WalRcvForceReply() and
RequestXLogStreaming() both are called from the startup process. See
commit 45f9d08684, that made the access to the walreceiver's
latch(WalRcv->latch) by the other process(startup) spinlock protected

And looks like, in general it's a standard practice to set latch to
wake up the process if waiting in case a SIGHUP signal is received and
reload the relevant config variables.

Going by the above analysis, the v3 patch looks good to me.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module

Bharath Rupireddy
Hi,

Attaching a patch that replaces custom signal handlers for SIGHUP and
SIGTERM in worker_spi.c.

Thoughts?


With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

v1-0001-Use-standard-SIGHUP-SIGTERM-handlers-in-worker_sp.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module

Fujii Masao-4
In reply to this post by Bharath Rupireddy


On 2020/11/10 21:30, Bharath Rupireddy wrote:

> On Tue, Nov 10, 2020 at 3:04 PM Fujii Masao <[hidden email]> wrote:
>>
>>>> The main reason for having SetLatch() in
>>>> SignalHandlerForConfigReload() is to wake up the calling process if
>>>> waiting in WaitLatchOrSocket() or WaitLatch() and reload the new
>>>> config file and use the reloaded config variables. Maybe we should
>>>> give a thought on the scenarios in which the walreceiver process
>>>> waits, and what happens in case the latch is set when SIGHUP is
>>>> received.
>>>
>>> The difference is whether the config file is processed at the next
>>> wakeup (by force-reply-request or SIGTERM) of walreceiver or
>>> immediately. If server-reload happened frequently, say, several times
>>> per second(?), we should consider to reduce the useless reloading, but
>>> actually that's not the case.
>>
>> So, attached is the patch that makes walreceiver use both standard
>> SIGTERM and SIGHUP handlers. Currently I've not found any actual
>> issues by making walreceiver use standard SIGHUP handler, yet.
>>
>
> I think it makes sense to replace WalRcv->latch with
> MyProc->procLatch(as they point to the same latch) in the functions
> that are called in the walreceiver process. However, we are using
> walrcv->latch with spinlock, say in WalRcvForceReply() and
> RequestXLogStreaming() both are called from the startup process. See
> commit 45f9d08684, that made the access to the walreceiver's
> latch(WalRcv->latch) by the other process(startup) spinlock protected
>
> And looks like, in general it's a standard practice to set latch to
> wake up the process if waiting in case a SIGHUP signal is received and
> reload the relevant config variables.
>
> Going by the above analysis, the v3 patch looks good to me.

Thanks for the analysis! I pushed the patch.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION


12