Re: [HACKERS] BUG #4961: pg_standby.exe crashes with no args

The following bug has been logged online:

Bug reference: 4961
Logged by:
Email address: wader2(at)jcom(dot)home(dot)ne(dot)jp
PostgreSQL version: 8.4.0
Operating system: Windows XP/2000
Description: pg_standby.exe crashes with no args
Details:

also clashes when run with recovery.conf,
after logged message below.

could not restore file "00000001.history" from archive: return code
-1073741811

Hi,

On Mon, Aug 3, 2009 at 9:18 PM, <wader2(at)jcom(dot)home(dot)ne(dot)jp> wrote:
> also clashes when run with recovery.conf,
> after logged message below.
>
> could not restore file "00000001.history" from archive: return code
> -1073741811

Does pg_standby (e.g., pg_standby.exe --version) succeed when you
execute it alone? If not, you might have failed the installation of it.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

thx,

> Does pg_standby (e.g., pg_standby.exe --version) succeed when you

--version and --help are no problem.

wader2 wrote:
> thx,
>
> > Does pg_standby (e.g., pg_standby.exe --version) succeed when you
>
> --version and --help are no problem.

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?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

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

Hi.

Yes, I also reproduce it. It is very strange......
==
Windows-XP Home Edition Version2002 Service Pack3(Japanese)
CPU N270 @ 1.60Ghz 1GB RAM
==

C:\Program Files\PostgreSQL\8.4\bin>pg_standby.exe --help
pg_standby allows PostgreSQL warm standby servers to be configured.

Usage:
<snip>

C:\Program Files\PostgreSQL\8.4\bin>pg_standby.exe
...crash...

However, MinGW+gcc returns a normal result.

$ pg_standby
pg_standby: not enough command-line arguments

I don't have the margin time which still investigates it.

Regards,
Hiroshi Saito

----- Original Message -----
From: "wader2" <wader2(at)jcom(dot)home(dot)ne(dot)jp>

> 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
>
> --
> Sent via pgsql-bugs mailing list (pgsql-bugs(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-bugs
>

On Mon, Aug 10, 2009 at 16:10, wader2<wader2(at)jcom(dot)home(dot)ne(dot)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 Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)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 Hagander
Me: 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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Magnus Hagander <magnus(at)hagander(dot)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(at)jcom(dot)home(dot)ne(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>
>>> Magnus Hagander <magnus(at)hagander(dot)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 Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Hi,

On Tue, Aug 11, 2009 at 3:10 AM, Magnus Hagander<magnus(at)hagander(dot)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(dot)fujii(at)gmail(dot)com> wrote:
> Hi,
>
> On Tue, Aug 11, 2009 at 3:10 AM, Magnus Hagander<magnus(at)hagander(dot)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 Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Hi,

On Thu, Aug 13, 2009 at 2:24 AM, Magnus Hagander<magnus(at)hagander(dot)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(dot)fujii(at)gmail(dot)com> wrote:
> On Thu, Aug 13, 2009 at 2:24 AM, Magnus Hagander<magnus(at)hagander(dot)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 Hagander
Me: 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(dot)linnakangas(at)enterprisedb(dot)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 Hagander
Me: 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(dot)linnakangas(at)enterprisedb(dot)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

Fujii Masao wrote:
> On Wed, Aug 19, 2009 at 6:45 PM, Heikki Linnakangas
> <heikki(dot)linnakangas(at)enterprisedb(dot)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(dot)linnakangas(at)enterprisedb(dot)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