Thread: Signals inheritance work - major problems

Hi!

So I did a proof-of-concept implementation of the inherit-signals code,
and hit some major problems.

As it is now, we start signals as the very first thing when the backend
starts (actually, we do WSAStartup() first, but I already changed the
order of those).
We clearly cannot continue doing this if we want to use an inherited
handle, since we haven't read the backend file at this time, and the
backend file is what tells us about the signals pipe to use.

But if I move the signals initialization code to after the backend file
read, that is a *lot* later. This is also *after* the backend has set up
it's signal handlers, which will crash if things aren't initialized.

In order to work around this, things have to be made much uglier than we
thought. We'd have to put a separate parameter on the commandline, not
in the backend file. And this commandline has to be parsed in main.c,
before we do pretty much anything else at all.

Looking at things, I can't help but think: If we create the pipe as the
very first command run in the new backend, the window where we'd lose a
signal is *extremely* small. Perhaps the better change is to make any
points that need to signal the backends at this point try more than once
tos end the signal? (kill() will return an error code in this case).

As long as we get the pipe set up, we will not lose any signals, even if
we haven't set up the handler yet. Because the signal mask is only
checked when running pgwin32_dispatch_queued_signals(), and we don't do
that until after we have set up the signal handler.

Do we go down the uglier path, or do you perhaps have a third idea? It
certainly can be done, but it's ugly enough that I figure it'd better be
discussed before I go ahead and do something.

//Magnus

> Hi!
>
> So I did a proof-of-concept implementation of the
> inherit-signals code, and hit some major problems.
>
> As it is now, we start signals as the very first thing when
> the backend starts (actually, we do WSAStartup() first, but I
> already changed the order of those).
> We clearly cannot continue doing this if we want to use an
> inherited handle, since we haven't read the backend file at
> this time, and the backend file is what tells us about the
> signals pipe to use.
>
> But if I move the signals initialization code to after the
> backend file read, that is a *lot* later. This is also
> *after* the backend has set up it's signal handlers, which
> will crash if things aren't initialized.
>
> In order to work around this, things have to be made much
> uglier than we thought. We'd have to put a separate parameter
> on the commandline, not in the backend file. And this
> commandline has to be parsed in main.c, before we do pretty
> much anything else at all.
>
> Looking at things, I can't help but think: If we create the
> pipe as the very first command run in the new backend, the
> window where we'd lose a signal is *extremely* small. Perhaps
> the better change is to make any points that need to signal
> the backends at this point try more than once tos end the
> signal? (kill() will return an error code in this case).
>
> As long as we get the pipe set up, we will not lose any
> signals, even if we haven't set up the handler yet. Because
> the signal mask is only checked when running
> pgwin32_dispatch_queued_signals(), and we don't do that until
> after we have set up the signal handler.
>
> Do we go down the uglier path, or do you perhaps have a third
> idea? It certainly can be done, but it's ugly enough that I
> figure it'd better be discussed before I go ahead and do something.
>

Eh. Of course, another option is to moev the read_backend_variables() up
much higher in the file. Not sure if that will cause other problems, not
sitting around the code right now but the thought came up. It'd have to
be before we do our calls to set up signal handlers. But that may not be
too bad?

//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:
> But if I move the signals initialization code to after the backend file
> read, that is a *lot* later. This is also *after* the backend has set up
> it's signal handlers, which will crash if things aren't initialized.

Hm?  read_backend_variables() happens in SubPostmasterMain(), which
certainly hasn't yet set up any signal handlers.  (It has done a
PG_SETMASK, but that could be moved down a few lines if it's a problem;
but my guess is that you want to do that before starting the signals thread
anyway.)

I'd be inclined to think in terms of just moving the
pgwin32_signal_initialize() call out of main.c and into
SubPostmasterMain; you'd also need it in PostmasterMain and
BootstrapMain I suppose, but that's not as bad as any of the
alternatives you mention.

            regards, tom lane

> > But if I move the signals initialization code to after the backend
> > file read, that is a *lot* later. This is also *after* the
> backend has
> > set up it's signal handlers, which will crash if things
> aren't initialized.
>
> Hm?  read_backend_variables() happens in SubPostmasterMain(),
> which certainly hasn't yet set up any signal handlers.  (It
> has done a PG_SETMASK, but that could be moved down a few
> lines if it's a problem; but my guess is that you want to do
> that before starting the signals thread
> anyway.)

Right. It's the PG_SETMASK that causes teh crash.


> I'd be inclined to think in terms of just moving the
> pgwin32_signal_initialize() call out of main.c and into
> SubPostmasterMain; you'd also need it in PostmasterMain and
> BootstrapMain I suppose, but that's not as bad as any of the
> alternatives you mention.

Hmm. I thought I looked at that and for some reason thought it was bad.
Can't remember what it was any more. I'll try something along this line
and let you know how it works out.


//Magnus

Tom Lane wrote:
> "Magnus Hagander" <mha@sollentuna.net> writes:
> > But if I move the signals initialization code to after the backend file
> > read, that is a *lot* later. This is also *after* the backend has set up
> > it's signal handlers, which will crash if things aren't initialized.
>
> Hm?  read_backend_variables() happens in SubPostmasterMain(), which
> certainly hasn't yet set up any signal handlers.  (It has done a
> PG_SETMASK, but that could be moved down a few lines if it's a problem;
> but my guess is that you want to do that before starting the signals thread
> anyway.)
>
> I'd be inclined to think in terms of just moving the
> pgwin32_signal_initialize() call out of main.c and into
> SubPostmasterMain; you'd also need it in PostmasterMain and
> BootstrapMain I suppose, but that's not as bad as any of the
> alternatives you mention.

I talked to Magnus via chat and I suggested there was little reason to
have the pipe name be based on the process id.  Rather a single counter
could be used that is passed on the command line or in the config file
and that can be used to create the pipe in the child so process creation
can be cleaner.  The parent can create the pipe and queue up any signals
in there until the child opens the other end.

This would prevent pg_ctl kill from working (it doesn't know the counter
value) but talking to Magnus there is little reason to have pg_ctl kill
work because we don't support admins sending termination signals to
individual backends anyway.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I talked to Magnus via chat and I suggested there was little reason to
> have the pipe name be based on the process id.  Rather a single counter
> could be used that is passed on the command line or in the config file
> and that can be used to create the pipe in the child so process creation
> can be cleaner.  The parent can create the pipe and queue up any signals
> in there until the child opens the other end.

This doesn't really help, because we do not want to have to interpret
any command-line stuff at the very top of main.c.  The signals thread
start has to be moved to someplace after we've parsed the command line.
(There isn't any harm in doing so, because we'd ordinarily not open up
the signal mask to accept signals until long after that anyway.)

Also, if the pipe name isn't based on the PID, how the heck are other
processes going to implement kill()?  They won't have any way to
translate a PID into some random counter value.

> This would prevent pg_ctl kill from working (it doesn't know the counter
> value)

Not to mention NOTIFY, SendPostmasterSignal, RequestCheckpoint, and
probably a few other things that escape me at the moment.

            regards, tom lane