Thread: Can we simplify win32 threading code

Currently PG win32 port mainly does the following to simulate signals:

(*) the process who kill the signal:- put the signal in a named pipe, then it is done;

(*) the process who should receive the signal:- a non-stop thread "pg_signal_thread" will read the signal from the
pipe,
and start another thread "pg_signal_dispatch_thread", which puts the signal
in a local memory variable "pg_signal_queue" and
SetEvent(local_memory_event_variable);- the main thread of this process could be awakened by the event from
waiting status(like semop()) or CHECK_FOR_INTERRUPTS() actively;


Could we simplify this process like this:

(*) the process who kill the signal:- put the signal in a *shared memory variable pg_signal_queue* and
SetEvent(*shared_memory_event_variable*), then it is done;

(*) the process who should receive the signal:- the main thread of this process could be awakened by the event from
waiting status(like semop()) or CHECK_FOR_INTERRUPTS() actively; -- there is
no other threads of this process;

Any show-stop reasons of not doing this?

Regards,
Qingqing

> Currently PG win32 port mainly does the following to simulate signals:
>
> (*) the process who kill the signal:
>  - put the signal in a named pipe, then it is done;
>
> (*) the process who should receive the signal:
>  - a non-stop thread "pg_signal_thread" will read the signal
> from the pipe, and start another thread
> "pg_signal_dispatch_thread", which puts the signal in a local
> memory variable "pg_signal_queue" and
> SetEvent(local_memory_event_variable);
>  - the main thread of this process could be awakened by the
> event from waiting status(like semop()) or
> CHECK_FOR_INTERRUPTS() actively;
>
>
> Could we simplify this process like this:
>
> (*) the process who kill the signal:
>  - put the signal in a *shared memory variable
> pg_signal_queue* and
> SetEvent(*shared_memory_event_variable*), then it is done;
>
> (*) the process who should receive the signal:
>  - the main thread of this process could be awakened by the
> event from waiting status(like semop()) or
> CHECK_FOR_INTERRUPTS() actively; -- there is no other threads
> of this process;
>
> Any show-stop reasons of not doing this?

Yeah, that should work. With one shared memory segment and one event for
each process, of course. The event can be the same one as is used now,
only it has to be named so it can be accessed externally.

It would do away with the thread, certainly. But it's not quite as
simple as you outline above - you'll need to replace the critical
section locking (easy, lightweight) with a mutex or something like that
(more complex, more heavy weight). But probably named pipes is more
heavy, yes.

You'll also need some way of delivering the feedback, I think - kill(0)
is supposed to tell you if there is a live process in th eother end, so
you can't just throw the signal out and hope for the best.

I think the named pipe parts of things is a leftover from back when we
were using APCs to interrupt the main thread - which required a separate
thread. But since we can't do that, this sounds like a reasonable
simplification.

//Magnus

Magnus Hagander wrote:

>>(*) the process who kill the signal:
>> - put the signal in a *shared memory variable 
>>pg_signal_queue* and 
>>SetEvent(*shared_memory_event_variable*), then it is done;
>>
>>(*) the process who should receive the signal:
>> - the main thread of this process could be awakened by the 
>>event from waiting status(like semop()) or 
>>CHECK_FOR_INTERRUPTS() actively; -- there is no other threads 
>>of this process;
>>
>>Any show-stop reasons of not doing this?
>>    
>>
>
>Yeah, that should work. With one shared memory segment and one event for
>each process, of course. The event can be the same one as is used now,
>only it has to be named so it can be accessed externally.
>
>
>  
>


I assume that this will not break the use of pg_ctl to deliver 
pseudo-signals. That would be a show-stopper.

cheers

andrew

> >>(*) the process who kill the signal:
> >> - put the signal in a *shared memory variable
> >>pg_signal_queue* and
> >>SetEvent(*shared_memory_event_variable*), then it is done;
> >>
> >>(*) the process who should receive the signal:
> >> - the main thread of this process could be awakened by the
> event from
> >>waiting status(like semop()) or
> >>CHECK_FOR_INTERRUPTS() actively; -- there is no other
> threads of this
> >>process;
> >>
> >>Any show-stop reasons of not doing this?
> >>
> >>
> >
> >Yeah, that should work. With one shared memory segment and one event
> >for each process, of course. The event can be the same one
> as is used
> >now, only it has to be named so it can be accessed externally.
>
>
> I assume that this will not break the use of pg_ctl to
> deliver pseudo-signals. That would be a show-stopper.

It shouldn't, but there is one concern: it has to be created in the
global namespace. On older windows there is no different, but on modern
windows with terminal services in it it does. It might require some
permissions hackings - I don't know what the default permissinos are on
these things. But I *think* it should work fine.

//Magnus

""Magnus Hagander"" <mha@sollentuna.net> writes
>
> Yeah, that should work. With one shared memory segment and one event for
> each process, of course. The event can be the same one as is used now,
> only it has to be named so it can be accessed externally.
>

Yes, the shared memory segment size could be controlled by MaxBackends.

> It would do away with the thread, certainly. But it's not quite as
> simple as you outline above - you'll need to replace the critical
> section locking (easy, lightweight) with a mutex or something like that
> (more complex, more heavy weight). But probably named pipes is more
> heavy, yes.
>

Yes, use mutex.

> You'll also need some way of delivering the feedback, I think - kill(0)
> is supposed to tell you if there is a live process in th eother end, so
> you can't just throw the signal out and hope for the best.
>

To simulate kill(0) we can test the process handle, just like we handle
waitpid().

""Magnus Hagander"" <mha@sollentuna.net> writes
> > I assume that this will not break the use of pg_ctl to
> > deliver pseudo-signals. That would be a show-stopper.
>
> It shouldn't, but there is one concern: it has to be created in the
> global namespace. On older windows there is no different, but on modern
> windows with terminal services in it it does. It might require some
> permissions hackings - I don't know what the default permissinos are on
> these things. But I *think* it should work fine.
>

Yes, the mutex/event stuff should be in the global namespace. What do you
mean the permission hacks?

Regards,
Qingqing

> > > I assume that this will not break the use of pg_ctl to deliver
> > > pseudo-signals. That would be a show-stopper.
> >
> > It shouldn't, but there is one concern: it has to be created in the
> > global namespace. On older windows there is no different, but on
> > modern windows with terminal services in it it does. It
> might require
> > some permissions hackings - I don't know what the default
> permissinos
> > are on these things. But I *think* it should work fine.
> >
>
> Yes, the mutex/event stuff should be in the global namespace.
> What do you mean the permission hacks?

It may not be necessary at all. But what I meant is I don't know what
the default permissinos ar eon a shared mem/event object in the global
namespace. On the naemd pipe it's "creator/owner full" (so the postgres
account itself can issue kills), "administrators full" (so anybody who
is admin on the box can) and "system full" (so the local system can
issue them). If this is not the default on the other class of objects,
we'll need to set specific permissions.

//Magnus

""Magnus Hagander"" <mha@sollentuna.net>  writes
>
> It may not be necessary at all. But what I meant is I don't know what
> the default permissinos ar eon a shared mem/event object in the global
> namespace. On the naemd pipe it's "creator/owner full" (so the postgres
> account itself can issue kills), "administrators full" (so anybody who
> is admin on the box can) and "system full" (so the local system can
> issue them). If this is not the default on the other class of objects,
> we'll need to set specific permissions.
>

Ok, I will come up with a patch per discussion. One more thing, any tips to
test the PG win32 signal part?

Regards,
Qingqing

I have simplified the code according to the discussion. Now there is no pipe
or signaling threads, process access shared memory directly to pass signals.
Seems everything works fine except pg_ctl. I now have two choices to fix it:

(1) Record Shared memory name (it is already there) and the offset of
signaling shared memory in postmaster.pid; So the pg_ctl program can access
the shared memory (since it has write down the signal number there) and
SetEvent target process;

(2) Postmaster will startup a thread monitoring messages, pg_ctl simulate
"kill" by sending postmaster a message <target_pid, signum>, then postmaster
will forward this "signum" to "target_pid";

What do you think?

Regards,
Qingqing

> I have simplified the code according to the discussion. Now
> there is no pipe or signaling threads, process access shared
> memory directly to pass signals.
> Seems everything works fine except pg_ctl. I now have two
> choices to fix it:
>
> (1) Record Shared memory name (it is already there) and the
> offset of signaling shared memory in postmaster.pid; So the
> pg_ctl program can access the shared memory (since it has
> write down the signal number there) and SetEvent target process;

Why not just use the pid in teh name, and have one segment per backend?
Remember on Win32 having a couple of hundred shared memory segments is
not a problem. (I assume you are creating these using MapViewOfFile()
etc, and not using the shmget() emulators)

Then if I want to kill pid 1234 I just do:
mtx = OpenMutex(..,"pqsignalmtx1234");
WaitForSingleObject(mtx);
m = OpenFileMapping(...,"pqsignal1234");
v = MapViewOfFile();
-- write signal number --
UnmapViewOfFile(v);
CloseHandle(m);
e = OpenEvent(..,"pqsignalevt1234");
SetEvent(e);
CloseHandle(e);
ReleaseMutex(mtx);
CloseHandle(mtx);

(pseudo-code. You'll need to look for deadlocks in the event processor,
but it should be fairly easy to write code that won't deadlock)



>
> (2) Postmaster will startup a thread monitoring messages,
> pg_ctl simulate "kill" by sending postmaster a message
> <target_pid, signum>, then postmaster will forward this
> "signum" to "target_pid";

I don't like that. If the postmaster dies, how will you signal the
remaining backends? The signal processing has to take place in the
actual backend affected, and only there. But that should be no problem
with the solution I outlined above.

This way you'll also be able to pg_kill a process without knowing where
the postmaster.pid file is, which at least I would expect. Needing to
specify the full data path just to run pg_kill is not good.

//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:
> Why not just use the pid in teh name, and have one segment per backend?

Being used only for signals you mean?  That might work.

I dislike fooling around with the contents of postmaster.pid, as that
will inject platform-specific code into places where there is none now.
If that's what the patch ends up requiring, I for one will vote to leave
things as they are now.

>> (2) Postmaster will startup a thread monitoring messages, 
>> pg_ctl simulate "kill" by sending postmaster a message 
>> <target_pid, signum>, then postmaster will forward this 
>> "signum" to "target_pid";

> I don't like that. If the postmaster dies, how will you signal the
> remaining backends?

Agreed, this seems pretty fragile ... and one thing you want from signal
processing is robustness.  It needs to be possible to signal a given
process without any support from any other.
        regards, tom lane

> > Why not just use the pid in teh name, and have one segment
> per backend?
>
> Being used only for signals you mean?  That might work.

That was my idea. We'll end up using three global namespace objects
(mutex+event+shared memory) instead of one (named pipe), but as we're
not talking thousands and thousands of backends in the normal case, this
shuold not be a problem I think. And if you do thousands and thousands
of backends, you'd better have the memory to support it anyway. I think
you'd hit other limits in the win32 port before you hit this one.


> I dislike fooling around with the contents of postmaster.pid,
> as that will inject platform-specific code into places where
> there is none now.

My thoughts exactly.


//Magnus

""Magnus Hagander"" <mha@sollentuna.net>
> >  Why not just use the pid in teh name, and have one segment
> > per backend?
> >
> > Being used only for signals you mean?  That might work.
>
> That was my idea. We'll end up using three global namespace objects
> (mutex+event+shared memory) instead of one (named pipe), but as we're
> not talking thousands and thousands of backends in the normal case, this
> shuold not be a problem I think. And if you do thousands and thousands
> of backends, you'd better have the memory to support it anyway. I think
> you'd hit other limits in the win32 port before you hit this one.
>
>
> > I dislike fooling around with the contents of postmaster.pid,
> > as that will inject platform-specific code into places where
> > there is none now.
>
> My thoughts exactly.
>

Ok, understood. In this way, that's more like the real Unix signals ...

Regards,
Qingqing