Re: [TODO] Allow commenting of variables ...

I would like to implement following item from TODO list:

* Allow commenting of variables in postgresql.conf to restore them to
defaults. Currently, if a variable is commented out, it keeps the
previous uncommented value until a server restarted.

Does anybody work on it?

I performed some investigation and I found that signal handler
(SIGHUP_handler) contents a big code and contents signal nonsafe
functions. It should generate deadlock or damage some internal data
structure in the standard c library. See
http://www.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html for detail. By my opinion is necessary to rewrite signal handling in postmaster to avoid postgres malfunction.

Zdenek

Zdenek Kotala wrote:

> I performed some investigation and I found that signal handler
> (SIGHUP_handler) contents a big code and contents signal nonsafe
> functions. It should generate deadlock or damage some internal data
> structure in the standard c library. See
> http://www.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html
> for detail. By my opinion is necessary to rewrite signal handling in
> postmaster to avoid postgres malfunction.

Perhaps you missed these lines:

/*
* Block all signals until we wait again. (This makes it safe for our
* signal handlers to do nontrivial work.)
*/
PG_SETMASK(&BlockSig);

postmaster.c 1227ff

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Here is some work someone has done:

http://archives.postgresql.org/pgsql-patches/2006-03/msg00258.php

See the followup for feedback. Seems it could be cleaned up.

---------------------------------------------------------------------------

Zdenek Kotala wrote:
>
> I would like to implement following item from TODO list:
>
> * Allow commenting of variables in postgresql.conf to restore them to
> defaults. Currently, if a variable is commented out, it keeps the
> previous uncommented value until a server restarted.
>
> Does anybody work on it?
>
> I performed some investigation and I found that signal handler
> (SIGHUP_handler) contents a big code and contents signal nonsafe
> functions. It should generate deadlock or damage some internal data
> structure in the standard c library. See
> http://www.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html for detail. By my opinion is necessary to rewrite signal handling in postmaster to avoid postgres malfunction.
>
>
> Zdenek
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: In versions below 8.0, the planner will ignore your desire to
> choose an index scan if your joining column's datatypes do not
> match
>

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

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

Alvaro Herrera wrote:
> Zdenek Kotala wrote:
>
>
>> I performed some investigation and I found that signal handler
>> (SIGHUP_handler) contents a big code and contents signal nonsafe
>> functions. It should generate deadlock or damage some internal data
>> structure in the standard c library. See
>> http://www.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html
>> for detail. By my opinion is necessary to rewrite signal handling in
>> postmaster to avoid postgres malfunction.
>>
>
> Perhaps you missed these lines:
>
> /*
> * Block all signals until we wait again. (This makes it safe for our
> * signal handlers to do nontrivial work.)
> */
> PG_SETMASK(&BlockSig);
>
> postmaster.c 1227ff
>
>
Blocking signal is false safe. It is race condition problem. You can
receive signal before you block it, but main problem is different. See
example:

If you have following functions and signal handler:

|char buffer[11];

void sig_handler(int signo)
{
f1('B');
}

void f1(char ch)
{
int n;
for( n = 0 ; n < 10; n++)
buffer[n] = ch;
}

|If you call function f1('A') you expect that content of buffer will be
"AAAAAAAAAA". It will be true until you don't receive signal. Signal is
asynchronous event and if you receive it during loop processing (for
example when n=3) then signal handler call f1 too, but with parametr
'B'. The result in buffer will be "BBBBBBBBBB" after signal processing.
And now f1 continue with n=3, 4, 5 ... And your expected result is
"BBBAAAAAAA". That is all. For example this is reason why you don't use
functions like printf, because they content static internal buffer. It
is reason why there are only small group of signal safe functions.

Decision is that Postgres uses signal dangerous functions (fopen, ...)
and its signal handler is not save and should generate unpredictable
behavior after signal processing. I would like to fix it, but there is
some waiting patches for this source and I don't know how to correctly
(with minimal merge complication) process.

Zdenek
|

|

On Thu, May 11, 2006 at 01:59:46PM +0200, Zdenek Kotala wrote:
> Decision is that Postgres uses signal dangerous functions (fopen, ...)
> and its signal handler is not save and should generate unpredictable
> behavior after signal processing. I would like to fix it, but there is
> some waiting patches for this source and I don't know how to correctly
> (with minimal merge complication) process.

Look at the code more carefully. The restriction is that it is unsafe
to call non-reentrant functions from within a signal handler while
there may be a non-reentrant function run by the main program.

If you look at the code in postmaster.c, the only place the signal
handler can run is between the block (line 1223) and unblock (line
1231), the only function there is select() which is specifically listed
as being safe.

Running unsafe functions within a signal handler is not unsafe per-se.
It's only unsafe if the main program could also be running unsafe
functions.

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

Martijn van Oosterhout <kleptog(at)svana(dot)org> writes:

> Running unsafe functions within a signal handler is not unsafe per-se.
> It's only unsafe if the main program could also be running unsafe
> functions.

I don't disagree with your reasoning, but does POSIX actually say
this?

-Doug

On Thu, May 11, 2006 at 08:24:02AM -0400, Douglas McNaught wrote:
> Martijn van Oosterhout <kleptog(at)svana(dot)org> writes:
>
> > Running unsafe functions within a signal handler is not unsafe per-se.
> > It's only unsafe if the main program could also be running unsafe
> > functions.
>
> I don't disagree with your reasoning, but does POSIX actually say
> this?

On my machine, signal(2) has the following:

The routine handler must be very careful, since processing
elsewhere was interrupted at some arbitrary point. POSIX has the
concept of "safe function". If a signal interrupts an unsafe
function, and handler calls an unsafe function, then the
behavior is undefined. Safe functions are listed explicitly in
the various standards. The POSIX 1003.1-2003 list is

<long list including select(), signal() and sigaction()>

I havn't read POSIX myself though...

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

Douglas McNaught <doug(at)mcnaught(dot)org> writes:
> Martijn van Oosterhout <kleptog(at)svana(dot)org> writes:
>> Running unsafe functions within a signal handler is not unsafe per-se.
>> It's only unsafe if the main program could also be running unsafe
>> functions.

> I don't disagree with your reasoning, but does POSIX actually say
> this?

The fact remains that the postmaster has *always* been coded like that,
and we have *never* seen any problems. Barring proof that there is a
problem, I'm uninterested in rewriting it just because someone doesn't
like it.

regards, tom lane

On Thu, May 11, 2006 at 10:11:00AM -0400, Tom Lane wrote:
> Douglas McNaught <doug(at)mcnaught(dot)org> writes:
> > I don't disagree with your reasoning, but does POSIX actually say
> > this?
>
> The fact remains that the postmaster has *always* been coded like that,
> and we have *never* seen any problems. Barring proof that there is a
> problem, I'm uninterested in rewriting it just because someone doesn't
> like it.

It should probably also be remembered that the "fix" would involve either
polling the status by having select() return more often, or using
sigsetjmp/siglongjmp. The cure is definitly worse than the disease.

In a sense the test for errno == EINTR there is redundant since the
backend has arranged that EINTR can never be returned (signals don't
interrupt system calls) and there's a fair bit of code that relies on
that...

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

* Martijn van Oosterhout:

>> The fact remains that the postmaster has *always* been coded like that,
>> and we have *never* seen any problems. Barring proof that there is a
>> problem, I'm uninterested in rewriting it just because someone doesn't
>> like it.
>
> It should probably also be remembered that the "fix" would involve either
> polling the status by having select() return more often, or using
> sigsetjmp/siglongjmp. The cure is definitly worse than the disease.

The standard trick is to add a pipe to the select and write to that
from the signal handler. I'm not sure if it's worth the effort,
though.

On Wed, May 10, 2006 at 08:46:41PM +0200, Zdenek Kotala wrote:
> I would like to implement following item from TODO list:

> * Allow commenting of variables in postgresql.conf to restore them to
> defaults. Currently, if a variable is commented out, it keeps the
> previous uncommented value until a server restarted.

> Does anybody work on it?

I have a patch that seems to work, it could need some more testing however.

I'll send it to you per PM.

Joachim

--
Joachim Wieland joe(at)mcknight(dot)de
C/ Usandizaga 12 1°B ICQ: 37225940
20002 Donostia / San Sebastian (Spain) GPG key available