Re: Win32 signals & sockets
Andrew Dunstan
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
---------------------------(end of broadcast)---------------------------
TIP 6: Have you searched our list archives?
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?
regards, tom lane
---------------------------(end of broadcast)---------------------------
TIP 3: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majo...@postgresql.org so that your
message can get through to the mailing list cleanly
Magnus Hagander
>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)
Tom Lane
>> 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
---------------------------(end of broadcast)---------------------------
TIP 4: Don't 'kill -9' the postmaster
Magnus Hagander
>*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
---------------------------(end of broadcast)---------------------------
TIP 9: the planner will ignore your desire to choose an index scan if your
joining column's datatypes do not match
Merlin Moncure
> 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
Tom Lane
> 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
---------------------------(end of broadcast)---------------------------
TIP 2: you can get off all lists at once with the unregister command
(send "unregister YourEmailAddressHere" to majo...@postgresql.org)
Joshua D. Drake
>
> [ grumble... ] OK, as long as Magnus is promising a code-beautification
> patch. postmaster.c is rapidly approaching a condition of unreadability
>
> regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: you can get off all lists at once with the unregister command
> (send "unregister YourEmailAddressHere" to majo...@postgresql.org)
--
Command Prompt, Inc., home of PostgreSQL Replication, and plPHP.
Postgresql support, programming shared hosting and dedicated hosting.
+1-503-667-4564 - j...@commandprompt.com - http://www.commandprompt.com
Mammoth PostgreSQL Replicator. Integrated Replication for PostgreSQL