Обсуждение: Win32 signals & sockets

Finally, here is the patch:

* Create the signal pipe in the postmaster and then inherit it into the
child
* Duplicate sockets using WSADuplicateSocket/WSASocket to get around
buggy LSP implementations. From my testing this does not solve *all*
problems, but it does solve a lot of them.
* Change pgstat so it signals the collector when the bufferer
terminates. Because some LSPs are still broken and process termination
just won't work when the pipe goes away.
* Clean up handling of the backend parameter file some. There was no
error checking at all in large parts of it before, which bit me during
testing, so I figured I might as well take a stab at that. This moves it
all into a structure and writes that one - which is also preparation for
the move of the parameter structure away from a file and into shared
memory.


If this is accepted I also plan to do a patch to split out the forkexec
code into a separate file and try to clean up the dependencies a bit
further. It'd be nice if I could get that into 8.0.0 (which would
probably mean this beta, since it seems to be the last one), but it's
not critical.
I'd also like to take a look at moving the parameter file into shared
memory at least on win32, since it gives thefilesystem quite a bit of
trashing. I think that can be done pretty easily, but I'm not sure if
there'll be tinme for that in 8.0.0 either.

//Magnus

Magnus Hagander wrote:

>
>If this is accepted I also plan to do a patch to split out the forkexec
>code into a separate file and try to clean up the dependencies a bit
>further. It'd be nice if I could get that into 8.0.0 (which would
>probably mean this beta, since it seems to be the last one), but it's
>not critical.
>I'd also like to take a look at moving the parameter file into shared
>memory at least on win32, since it gives thefilesystem quite a bit of
>trashing. I think that can be done pretty easily, but I'm not sure if
>there'll be tinme for that in 8.0.0 either.
>
>
>

Both excellent things to do. The use of the file system for this is ...
ugly, although it was quite a sensible thing to do while we got other
parts of the mechanism right.

cheers

andrew

This patch *replaces* the previous one. Contains the exact same changes,
except it *also* contains the move of the backend parameter file to
shared memory on win32.

This gives a speed boost of about 10% on a program that calls
PQconnect/PQfinish in a tight loop on 10 parallell threads on my system
(client and server both on the same machine, desktop system).

I figured this was the most important thing to get in for now :-) I
still plan to do the cleanups of the forkexec code later.

//Magnus


>-----Original Message-----
>From: Magnus Hagander
>Sent: den 12 november 2004 22:21
>To: PostgreSQL Patches
>Subject: [PATCHES] Win32 signals & sockets
>
>
>Finally, here is the patch:
>
>* Create the signal pipe in the postmaster and then inherit it into the
>child
>* Duplicate sockets using WSADuplicateSocket/WSASocket to get around
>buggy LSP implementations. From my testing this does not solve *all*
>problems, but it does solve a lot of them.
>* Change pgstat so it signals the collector when the bufferer
>terminates. Because some LSPs are still broken and process termination
>just won't work when the pipe goes away.
>* Clean up handling of the backend parameter file some. There was no
>error checking at all in large parts of it before, which bit me during
>testing, so I figured I might as well take a stab at that.
>This moves it
>all into a structure and writes that one - which is also
>preparation for
>the move of the parameter structure away from a file and into shared
>memory.
>
>
>If this is accepted I also plan to do a patch to split out the forkexec
>code into a separate file and try to clean up the dependencies a bit
>further. It'd be nice if I could get that into 8.0.0 (which would
>probably mean this beta, since it seems to be the last one), but it's
>not critical.
>I'd also like to take a look at moving the parameter file into shared
>memory at least on win32, since it gives thefilesystem quite a bit of
>trashing. I think that can be done pretty easily, but I'm not sure if
>there'll be tinme for that in 8.0.0 either.
>
>//Magnus
>
>

"Magnus Hagander" <mha@sollentuna.net> writes:
> This patch *replaces* the previous one. Contains the exact same changes,
> except it *also* contains the move of the backend parameter file to
> shared memory on win32.

I think it's way too late in the beta cycle for significant changes in
the fork mechanism ... especially if the gain is only 10% on something
that isn't a performance issue for properly written applications anyway.

Can I safely ignore this patch and use the prior one, or are there
additional bug fixes in this?

            regards, tom lane

>> This patch *replaces* the previous one. Contains the exact
>same changes,
>> except it *also* contains the move of the backend parameter file to
>> shared memory on win32.
>
>I think it's way too late in the beta cycle for significant changes in
>the fork mechanism ... especially if the gain is only 10% on something
>that isn't a performance issue for properly written
>applications anyway.
>
>Can I safely ignore this patch and use the prior one, or are there
>additional bug fixes in this?

You can, there are no additional bug fixes.

But let me give it a shot to try to convince you to put it in there
anyway :-)

I realise it's late in the beta. But all the actually *complicated* code
in this patch is in the first patch - the splitting up of the
CreateProcess/ResumeThread steps and the WSADuplicateSocket code. The
part that moves the param file -> shared memory is a very small and
simple part of the patch. It's basically two API calls int he postmaster
(parent) and one in the backend.

Considering that this is very little code, uses only very core features
on the win32 API, and will be exercised *all the time* (every backend
startup, ever), we should see errors in thise codepath pretty soon. If
we do, we can always back it out before RC instead of trying to fix it.

Just the speedup is not the only reason to apply it. Continually
creating and deleting a file in the directory will also cause the
filesystem journal and MFT entries to build up significantly. (Every
file commit also requires a flush of the filesystem journal, which I
beleive is where those 10% are found. The difference would probably be
greater than 10% on a system that is I/O loaded, but I haven't tested
that)

(The reason the patch looks pretty much different from the first patch
is that a couple of structures had to be moved higher up in postmaster.c
causing the diff to be larger)


But the summary still holds - this is not a *critical* bug fix. I'd
argue it's a bug fix, but we *can* do without it.


//Magnus

(No, I'm not going to push for the next stage of forkexec cleanups to go
in 8.0 :P)

"Magnus Hagander" <mha@sollentuna.net> writes:
>> I think it's way too late in the beta cycle for significant changes in
>> the fork mechanism ...

> I realise it's late in the beta. But all the actually *complicated* code
> in this patch is in the first patch - the splitting up of the
> CreateProcess/ResumeThread steps and the WSADuplicateSocket code. The
> part that moves the param file -> shared memory is a very small and
> simple part of the patch.

Maybe so, but it also puts the final nail in the coffin of the illusion
that testing EXEC_BACKEND behavior on Unix will give any confidence
about not having broken the code on Windows.  Also I think your thumb is
on the scales a bit because the initial patch is doing more than it had
to in this area (you didn't need to invent struct BackendParameters,
did you?)

It's the increase in variance between the Unix and Windows code paths
that's really bothering me.  We went into this project on the promise
that there weren't going to be thousands of lines of #ifdef WIN32 stuff,
and I'm not happy in the least with the way postmaster.c looks now, let
alone after applying this patch.

            regards, tom lane

>> I realise it's late in the beta. But all the actually
>*complicated* code
>> in this patch is in the first patch - the splitting up of the
>> CreateProcess/ResumeThread steps and the WSADuplicateSocket code. The
>> part that moves the param file -> shared memory is a very small and
>> simple part of the patch.
>
>Maybe so, but it also puts the final nail in the coffin of the illusion
>that testing EXEC_BACKEND behavior on Unix will give any confidence
>about not having broken the code on Windows.

Not sure how it will be that much worse than what we have now. It will
certainly not check if the actual exec backend code is broken, but if
it's missing to pass a long a variable or so (which I beleive would be
the most common things in the future) it should catch it just as well as
today.

> Also I think your thumb is
>on the scales a bit because the initial patch is doing more than it had
>to in this area (you didn't need to invent struct BackendParameters,
>did you?)

No, I didn't need to do that, but I figured it was the easiest way to be
able to check the error values. Previously, the return code from
fwrite() was never checked, which cannot possibly be said to be good.

But sure, it could be changed to check the result code of every fwrite()
and fread() instead. And sure, I may have been influenced by my plans
(or the plans in general, since it's been on the TODO for ages) to
eventually move it to shared memory when I chose that path - but I still
think it's the easiest option regardless.


>It's the increase in variance between the Unix and Windows code paths
>that's really bothering me.  We went into this project on the promise
>that there weren't going to be thousands of lines of #ifdef
>WIN32 stuff,
>and I'm not happy in the least with the way postmaster.c looks now, let
>alone after applying this patch.

Right. This is what I was planning to address in the "cleanup step", but
that's a lot larger changes than these... The plan to get more #ifdefs
out of there and into a port-specific file instead. It would still be
different code, but it would be in more clearly defined parts. Similar
to the rest of the win32 specific code that goes in backend/port.


//Magnus

Tom Lane wrote:
> It's the increase in variance between the Unix and Windows code paths
> that's really bothering me.  We went into this project on the promise
> that there weren't going to be thousands of lines of #ifdef WIN32
stuff,
> and I'm not happy in the least with the way postmaster.c looks now,
let
> alone after applying this patch.

I've been following this thread for a bit and I have to admit I wouldn't
mind seeing the shmmem part of Magnus's patch go in.  Windows suffers vs
unix generally on process creation times and any improvement here would
be welcome.

Merlin

"Merlin Moncure" <merlin.moncure@rcsonline.com> writes:
> I've been following this thread for a bit and I have to admit I wouldn't
> mind seeing the shmmem part of Magnus's patch go in.  Windows suffers vs
> unix generally on process creation times and any improvement here would
> be welcome.

[ grumble... ]  OK, as long as Magnus is promising a code-beautification
patch.  postmaster.c is rapidly approaching a condition of unreadability
== unmaintainability.

            regards, tom lane

>
> [ grumble... ]  OK, as long as Magnus is promising a code-beautification
> patch.  postmaster.c is rapidly approaching a condition of unreadability
> == unmaintainability == perl ;).
>
>             regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: you can get off all lists at once with the unregister command
>     (send "unregister YourEmailAddressHere" to majordomo@postgresql.org)


--
Command Prompt, Inc., home of PostgreSQL Replication, and plPHP.
Postgresql support, programming shared hosting and dedicated hosting.
+1-503-667-4564 - jd@commandprompt.com - http://www.commandprompt.com
Mammoth PostgreSQL Replicator. Integrated Replication for PostgreSQL

"Magnus Hagander" <mha@sollentuna.net> writes:
> This patch *replaces* the previous one. Contains the exact same changes,
> except it *also* contains the move of the backend parameter file to
> shared memory on win32.

Committed with some small editorializing.  Possibly the weight of my
concern about further dividing the Unix and Win32 cases can be brought
home to you by pointing out that the non-Windows EXEC_BACKEND case had
three different compile failures after applying the patch.

I think the error recovery in the Windows internal_forkexec() routine
is pretty shoddy --- most of the failure paths leak resources of one
variety or another.  Perhaps you can fix that during your upcoming
code refactoring.

            regards, tom lane

> > This patch *replaces* the previous one. Contains the exact same
> > changes, except it *also* contains the move of the backend
> parameter
> > file to shared memory on win32.
>
> Committed with some small editorializing.  Possibly the
> weight of my concern about further dividing the Unix and
> Win32 cases can be brought home to you by pointing out that
> the non-Windows EXEC_BACKEND case had three different compile
> failures after applying the patch.

Really? I did test it on Linux before I sent it in.. Or rather, I
thought I did. Must have something badly broken in my EXEC_BACKEND setup
on Linux then - need to go check that.

What's the recommended way to compile in Unix with exec_backend? I've
just been editing headers/makefiles, and those get overwritetn at
configure - probably something like that happened in this case and I
didn't notice. There is no switch to configure from what I can see?


> I think the error recovery in the Windows internal_forkexec()
> routine is pretty shoddy --- most of the failure paths leak
> resources of one variety or another.  Perhaps you can fix
> that during your upcoming code refactoring.

Yes. Some if it is leftover from before, and some is definitly new. But
most of these cases will either happen every time (= no backend startup
at all, so you'll notice real fast) or are "cannot happen" cases (like
can't close something). But yes, I'll try to get that into the
refactoring.

Just to make sure I'm following you completely - you are still saying
this refactoring waits until after 8.0, right? That's how I read it, but
I want to make sure you're not going to hold up anything further in in
the 8.0 beta waiting for something like this.


Anyway. Thanks!

//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:
> What's the recommended way to compile in Unix with exec_backend?

I don't think we have a recommended way (or need one really).
Personally I add #define EXEC_BACKEND to pg_config.h after configuring.

> Just to make sure I'm following you completely - you are still saying
> this refactoring waits until after 8.0, right? That's how I read it, but
> I want to make sure you're not going to hold up anything further in in
> the 8.0 beta waiting for something like this.

Yeah, it's probably too late in the 8.0 cycle for major changes that are
only for cosmetic purposes ...

            regards, tom lane

> > What's the recommended way to compile in Unix with exec_backend?
>
> I don't think we have a recommended way (or need one really).
> Personally I add #define EXEC_BACKEND to pg_config.h after
> configuring.

Right. That's what I've been doing. Just need to be more careful
checking the result of it, I guess. :-(

//Magnus