Thread: Re: [BUGS] BUG #4961: pg_standby.exe crashes with no args
On Mon, Aug 10, 2009 at 16:10, wader2<wader2@jcom.home.ne.jp> wrote: > Bruce Momjian wrote: >> >> I can't reproduce a crash here on BSD: >> >> $ pg_standby >> pg_standby: not enough command-line arguments >> >> Can you show us the command and the crash text? > > I guess this occurs on only windows (Japanese envionment?). > > C:\Program Files\PostgreSQL\8.4\bin>pg_standby.exe > > results no text on command line, Windows error dialog. > > AppName:pg_standby.exe AppVer:0.0.0.0 ModName:msvcr80.dll > ModVer:8.0.50727.762 Offset:000091ad I have reproduced this. The problem is:(void) signal(SIGUSR1, sighandler);(void) signal(SIGINT, sighandler); /* deprecated,use SIGUSR1 */ None of these signals exist on WIN32. I think the only reason it compiles at all is that we bring in *some* of our signals emulation code, but certainly not all of it. If I just move those two lines into the #ifndef WIN32 block just around it, it compiles and doesn't crash on running-with-no-arguments. I haven't tried to actually use it though - can someone confirm if this will actually make pg_standby not work properly? -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
> If I just move those two lines into the #ifndef WIN32 block just
> around it, it compiles and doesn't crash on running-with-no-arguments.
> I haven't tried to actually use it though - can someone confirm if
> this will actually make pg_standby not work properly?
It would mean there's no way to trigger failover via signal.
I think what we need is for pg_ctl to be able to send these signals...
regards, tom lane
On Mon, Aug 10, 2009 at 20:44, Tom Lane<tgl@sss.pgh.pa.us> wrote: > Magnus Hagander <magnus@hagander.net> writes: >> If I just move those two lines into the #ifndef WIN32 block just >> around it, it compiles and doesn't crash on running-with-no-arguments. >> I haven't tried to actually use it though - can someone confirm if >> this will actually make pg_standby not work properly? > > It would mean there's no way to trigger failover via signal. > > I think what we need is for pg_ctl to be able to send these signals... Those signals don't *exist* on Windows. The whole idea of cross-process signals don't *exist* on Windows. We emulate it in the main backend, by creating a background thread that sets a global variable. That is then polled in the CHECK_FOR_INTERRUPTS macro. pg_ctl is perfectly capable of sending these signals, but pg_standby can't receive them. We could implement the same type of check in pg_standby, but it requires something like CHECK_FOR_INTERRUPTS. And these interrupts won't, by default, cause any kind of interruption of the process. In the backend, we interrupt socket calls because we have the socket wrapper layer, and nothing else. I don't know how doable this would be in pg_standby - does it always block on a single thing where we could stick some win32 synchronization code? If it's a single, or limited, places we could implement something similar to the backend. But if we need to interrupt at arbitrary locations, that's just not possible. -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
I can't compile nor read source, but can tell you that pg_standby.exe in 8.3.7 works fine. Magnus Hagander wrote: > On Mon, Aug 10, 2009 at 20:44, Tom Lane<tgl@sss.pgh.pa.us> wrote: >> Magnus Hagander <magnus@hagander.net> writes: >>> If I just move those two lines into the #ifndef WIN32 block just >>> around it, it compiles and doesn't crash on running-with-no-arguments. >>> I haven't tried to actually use it though - can someone confirm if >>> this will actually make pg_standby not work properly? >> It would mean there's no way to trigger failover via signal. >> >> I think what we need is for pg_ctl to be able to send these signals... > > Those signals don't *exist* on Windows. The whole idea of > cross-process signals don't *exist* on Windows. > > We emulate it in the main backend, by creating a background thread > that sets a global variable. That is then polled in the > CHECK_FOR_INTERRUPTS macro. pg_ctl is perfectly capable of sending > these signals, but pg_standby can't receive them. > > We could implement the same type of check in pg_standby, but it > requires something like CHECK_FOR_INTERRUPTS. And these interrupts > won't, by default, cause any kind of interruption of the process. In > the backend, we interrupt socket calls because we have the socket > wrapper layer, and nothing else. I don't know how doable this would be > in pg_standby - does it always block on a single thing where we could > stick some win32 synchronization code? If it's a single, or limited, > places we could implement something similar to the backend. But if we > need to interrupt at arbitrary locations, that's just not possible. > >
Yeah, the patch I think breaks things isn't included in 8.3.7 - it will be in 8.3.8 though... //Magnus On Wed, Aug 12, 2009 at 11:08, wader2<wader2@jcom.home.ne.jp> wrote: > I can't compile nor read source, but can tell you that > pg_standby.exe in 8.3.7 works fine. > > Magnus Hagander wrote: >> >> On Mon, Aug 10, 2009 at 20:44, Tom Lane<tgl@sss.pgh.pa.us> wrote: >>> >>> Magnus Hagander <magnus@hagander.net> writes: >>>> >>>> If I just move those two lines into the #ifndef WIN32 block just >>>> around it, it compiles and doesn't crash on running-with-no-arguments. >>>> I haven't tried to actually use it though - can someone confirm if >>>> this will actually make pg_standby not work properly? >>> >>> It would mean there's no way to trigger failover via signal. >>> >>> I think what we need is for pg_ctl to be able to send these signals... >> >> Those signals don't *exist* on Windows. The whole idea of >> cross-process signals don't *exist* on Windows. >> >> We emulate it in the main backend, by creating a background thread >> that sets a global variable. That is then polled in the >> CHECK_FOR_INTERRUPTS macro. pg_ctl is perfectly capable of sending >> these signals, but pg_standby can't receive them. >> >> We could implement the same type of check in pg_standby, but it >> requires something like CHECK_FOR_INTERRUPTS. And these interrupts >> won't, by default, cause any kind of interruption of the process. In >> the backend, we interrupt socket calls because we have the socket >> wrapper layer, and nothing else. I don't know how doable this would be >> in pg_standby - does it always block on a single thing where we could >> stick some win32 synchronization code? If it's a single, or limited, >> places we could implement something similar to the backend. But if we >> need to interrupt at arbitrary locations, that's just not possible. >> >> > > -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Hi, On Tue, Aug 11, 2009 at 3:10 AM, Magnus Hagander<magnus@hagander.net> wrote: > I have reproduced this. The problem is: > (void) signal(SIGUSR1, sighandler); > (void) signal(SIGINT, sighandler); /* deprecated, use SIGUSR1 */ > > > None of these signals exist on WIN32. I think the only reason it > compiles at all is that we bring in *some* of our signals emulation > code, but certainly not all of it. SIGINT has been used in pg_standby for a while (e.g., v8.3.7). I wonder why this problem arises only in v8.4.0. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
On Wed, Aug 12, 2009 at 19:08, Fujii Masao<masao.fujii@gmail.com> wrote: > Hi, > > On Tue, Aug 11, 2009 at 3:10 AM, Magnus Hagander<magnus@hagander.net> wrote: >> I have reproduced this. The problem is: >> (void) signal(SIGUSR1, sighandler); >> (void) signal(SIGINT, sighandler); /* deprecated, use SIGUSR1 */ >> >> >> None of these signals exist on WIN32. I think the only reason it >> compiles at all is that we bring in *some* of our signals emulation >> code, but certainly not all of it. > > SIGINT has been used in pg_standby for a while (e.g., v8.3.7). I wonder > why this problem arises only in v8.4.0. Not sure. Potentially pure luck. SIGINT has never *worked*, though, it just hasn't crashed. -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Hi,
On Thu, Aug 13, 2009 at 2:24 AM, Magnus Hagander<magnus@hagander.net> wrote:
> Not sure. Potentially pure luck. SIGINT has never *worked*, though, it
> just hasn't crashed.
OK.
> We could implement the same type of check in pg_standby, but it
> requires something like CHECK_FOR_INTERRUPTS. And these interrupts
> won't, by default, cause any kind of interruption of the process. In
> the backend, we interrupt socket calls because we have the socket
> wrapper layer, and nothing else. I don't know how doable this would be
> in pg_standby - does it always block on a single thing where we could
> stick some win32 synchronization code? If it's a single, or limited,
> places we could implement something similar to the backend. But if we
> need to interrupt at arbitrary locations, that's just not possible.
I think that CHECK_FOR_INTERRUPTS should be placed just before
checking the flag 'signaled' which may be enabled by the signal handler.
Here is the pseudo-code.
-------------------- { /* Check for trigger file or signal first */
CheckForExternalTrigger();
+ #ifdef WIN32
+ CHECK_FOR_INTERRUPTS();
+ #endif /* WIN32 */ if (signaled) { Failover = FastFailover;
--------------------
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
On Wed, Aug 19, 2009 at 08:38, Fujii Masao<masao.fujii@gmail.com> wrote:
> On Thu, Aug 13, 2009 at 2:24 AM, Magnus Hagander<magnus@hagander.net> wrote:
>> Not sure. Potentially pure luck. SIGINT has never *worked*, though, it
>> just hasn't crashed.
>
> OK.
>
>> We could implement the same type of check in pg_standby, but it
>> requires something like CHECK_FOR_INTERRUPTS. And these interrupts
>> won't, by default, cause any kind of interruption of the process. In
>> the backend, we interrupt socket calls because we have the socket
>> wrapper layer, and nothing else. I don't know how doable this would be
>> in pg_standby - does it always block on a single thing where we could
>> stick some win32 synchronization code? If it's a single, or limited,
>> places we could implement something similar to the backend. But if we
>> need to interrupt at arbitrary locations, that's just not possible.
>
> I think that CHECK_FOR_INTERRUPTS should be placed just before
> checking the flag 'signaled' which may be enabled by the signal handler.
> Here is the pseudo-code.
>
> --------------------
> {
> /* Check for trigger file or signal first */
> CheckForExternalTrigger();
> + #ifdef WIN32
> + CHECK_FOR_INTERRUPTS();
> + #endif /* WIN32 */
> if (signaled)
> {
> Failover = FastFailover;
> --------------------
It's going to take a lot more than that, really. I looked a bit more
at it, and I think the best way is to do a win32 specific thing all
the way, not just emulate signals. Given that we only have a couple of
signal() calls anyway. I think that's going to be much clearer in what
it does, and also a lot simpler code in the end. The reason we have
the full emulation layer is that we use signals all over the place in
the backend.
From what I can tell, we don't actually need to interrupt anything
other than the sleep call on line 810, because we only check for the
signaled=true state in that loop anyway. Can someone more
knowledgeable in the pg_standby code comment on that?
This would amount to fairly major surgery for pg_standby on Win32. Is
that something we'd want to backpatch, or do we want to backpatch just
the removal of the signal() calls which would amount to not supporting
signals in pg_standby on win32?
-- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Magnus Hagander wrote: > This would amount to fairly major surgery for pg_standby on Win32. Is > that something we'd want to backpatch, or do we want to backpatch just > the removal of the signal() calls which would amount to not supporting > signals in pg_standby on win32? I think we should just remove the signals support for win32. The trigger file method still works, and the signal method has always been a bit iffy (it doesn't work when pg_standby isn't running, for example, which happens between processing of each WAL file, because there's no process to signal). Is pg_standby killed correctly when postmaster dies? -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On Wed, Aug 19, 2009 at 11:45, Heikki Linnakangas<heikki.linnakangas@enterprisedb.com> wrote: > Magnus Hagander wrote: >> This would amount to fairly major surgery for pg_standby on Win32. Is >> that something we'd want to backpatch, or do we want to backpatch just >> the removal of the signal() calls which would amount to not supporting >> signals in pg_standby on win32? > > I think we should just remove the signals support for win32. The trigger > file method still works, and the signal method has always been a bit > iffy (it doesn't work when pg_standby isn't running, for example, which > happens between processing of each WAL file, because there's no process > to signal). Fair enough. > Is pg_standby killed correctly when postmaster dies? No idea. I can set up and env to test, but I don't have one ready. Anybody else have tried this? -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Hi, Sorry for this late reply. On Wed, Aug 19, 2009 at 6:45 PM, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > Is pg_standby killed correctly when postmaster dies? No. In my test (v8.3.7 on Win), an immediate shutdown was able to kill postmaster, but not pg_standby and the startup process. Though we should change those to handle the shutdown signals correctly, this change is not imperative need. At first, we should just get rid of the signal support from pg_standby on Win, in order to avoid the pg_standby crash. The attached is the patch to do so. If this patch is OK, the backport is required for 8.3.x. Thought? Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Attachment
Fujii Masao wrote: > On Wed, Aug 19, 2009 at 6:45 PM, Heikki Linnakangas > <heikki.linnakangas@enterprisedb.com> wrote: >> Is pg_standby killed correctly when postmaster dies? > > No. In my test (v8.3.7 on Win), an immediate shutdown was able to > kill postmaster, but not pg_standby and the startup process. Hmm. We should do something about that.. Magnus? > Though we should change those to handle the shutdown signals > correctly, this change is not imperative need. At first, we should > just get rid of the signal support from pg_standby on Win, in order > to avoid the pg_standby crash. The attached is the patch to do so. > If this patch is OK, the backport is required for 8.3.x. Ok, committed to HEAD, 8.4 and 8.3. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Hi, On Wed, Nov 4, 2009 at 9:53 PM, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: >> Though we should change those to handle the shutdown signals >> correctly, this change is not imperative need. At first, we should >> just get rid of the signal support from pg_standby on Win, in order >> to avoid the pg_standby crash. The attached is the patch to do so. >> If this patch is OK, the backport is required for 8.3.x. > > Ok, committed to HEAD, 8.4 and 8.3. Thanks a lot! Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center