Re: New win32 signals patch (3)

> > ATM, for review, not for applying.
>
> Comments:
>
> * Nice to see the fix for pqselect. Still want to object to
> the memcpy "on the record" :-) (though, as we've discussed,
> they'll work as things are now)

Actually, I noticed we *already* do memcpy on fd_sets in the postmaster,
so it's nothing new. I don't think it'll be a problem.

> * memcpy of an array of HANDLES will work seems to be taking
> advantage of knowledge of the implementation. Use DuplicateHandle?

That should be very unnecessary, no? It is knowledge that it is a fixed
size var and not a pointer, both of which are documented (since pointers
are all prefixed by P or LP, and HANDLE clearly is not).
It's pretty much the same issue as the one about fd_sets above. Also, it
should go away if we change method per your suggestions below.

> * Better yet, cooperation with the other functions that use
> the win32_childHND/PIDArrays, by reserving the first slot for
> event1, could see us doing away with both event 2 and the
> need for copying.

No, that won't do away with the need for copying. The backend thread is
not going to be happy if the main thread goes and changes the array
underneath it. So you still need a copy. But yes, it could be
integreated in the win32_childHWND array.

> * Alternatively, I personally think the best solution is to
> simply fire off a "baby-sitting" thread, in the parent, for
> each process that gets created, passing it a duplicate of the
> (single) child handle. Each thread would just sit there
> forever, waiting for the child handle to be signalled, and
> then signal (ie. kill/raise) its process and terminate.
> AFAICS, a 2 line addition to win32_forkexec, and a 5 line
> thread function to do the waiting/signalling, and we are
> done. This also has the advantage that it'll work just fine
> for the pgstat buffer process, which fires off the pgstat
> collector process... which won't be covered by the proposed
> approach (and before anyone jumps in to say this is a penalty
> in creating backends, consider that the previous call would
> have been to CreateProcess, so a _beginthreadex aint gonna hurt!)
>
> Suffice to say that I'd *really* like to see this approach explored.

Ok. One thread per forked backend, right? Now that I think of it, this
certainly sounds like a simpler solution :) I'll take a look at this one
and see what it gives. Should give simpler code, yes.

> * Suggest using a switch statement in win32_sigchld_sender,
> instead of a nested if.
I started out with that, but removed it since it is just a single case
we care about... (I cuold do != on LOGOFF events, but that would be
unsafe in case they add some other events in a future version)

> Here's hoping you agree on the "baby-sitting" thread approach, Claudio

I definitly agree it's worth trying :-)

Thanks for the quick feedback!

//Magnus