Re: pgstat: remove delayed destroy / pipe: socketerror fix
BTW, what's with the change to all the error msgs?
Ah, I'd assumed the %ui was a typo in the format string. If
the intent was to print e.g. 10022i, I'll change it back.
Eh. No, it certainly wasn't. Keep the change :)
And finally, the error handling looks a bit off? We specifically
*don't*
want it to log an error for the WSAECONNRESET state - it's a normal
state. Or am I reading the patch wrong?If error is WSAECONNRESET, the return value is reset to zero.
That will prevent an error message from being displayed
(unless I'm reading something wrong).
Ah. Right. Reading it again I see that you're correct there as well. My
bad.
Do you get anything at all in the logs when this happens? Are you
sure
the reason is that it picks up an empty file, or could it be
something
else?
There is nothing in the logs. It could definitely be something else.
A rename race condition was just our best theory.
Ok. Well, I won't say it's impossible, but MoveFile() *should* be
atomic. It could well have something to do with sharing violations
though - our code should deal with that, but there's always a risk.
//Magnus
That's probably not a bad idea. AFAIK we haven't had reports of it
elsewhere, but it oculd happen. Want to code up a new patch, and run
some tests?
//Magnus
Show quoted text
-----Original Message-----
Also, do we want to move the retry loop to pgwin32_recv?
That seems like a good idea. I'm not sure users of recv
should ever have to deal with WSAEWOULDBLOCK as it's not
really an error.Pete
"Magnus Hagander" <mha@sollentuna.net> 04/06/06 9:58 pm >>>
Attached are two patches which in combination make
pg_stat_activity
work reliably for us on Windows.
...
pgstat.patch removes the delayed destroy code for backends,databases,
and tables. Database and table entries are cleaned up immediately
upon receipt of the appropriate message.
I'll go ahead and apply the delayed-destroy-removal part
(just to HEAD for the time being --- seems a bit risky to
apply it to the stable branches). The Windows-specific
change sounds like it may need more review.Actually, I think that's mostly me being confused and taking others
with
me ;-)It's definitly a problem, and we have a solution there. The one thing
I'm still a bit concerned about is: Do we need a CHECK_FOR_INTERRUPTS,
and do we need an upper limit on the spinning. In theory we can spin
with 100% CPU usage, which is not good. So we should either spin a
limited amount of times, or we should perhaps introduce a tiny delay?//Magnus
---------------------------(end of
broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that
your
message can get through to the mailing list cleanly
Import Notes
Resolved by subject fallback
Sounds good. I'll check how much we're actually looping too.
Pete
"Magnus Hagander" <mha@sollentuna.net> 04/06/06 10:27 pm >>>
That's probably not a bad idea. AFAIK we haven't had reports of it
elsewhere, but it oculd happen. Want to code up a new patch, and run
some tests?
//Magnus
-----Original Message-----
Also, do we want to move the retry loop to pgwin32_recv?
That seems like a good idea. I'm not sure users of recv
should ever have to deal with WSAEWOULDBLOCK as it's not
really an error.Pete
"Magnus Hagander" <mha@sollentuna.net> 04/06/06 9:58 pm >>>
Attached are two patches which in combination make
pg_stat_activity
work reliably for us on Windows.
...
pgstat.patch removes the delayed destroy code for backends,databases,
and tables. Database and table entries are cleaned up
immediately
upon receipt of the appropriate message.
I'll go ahead and apply the delayed-destroy-removal part
(just to HEAD for the time being --- seems a bit risky to
apply it to the stable branches). The Windows-specific
change sounds like it may need more review.Actually, I think that's mostly me being confused and taking others
with
me ;-)It's definitly a problem, and we have a solution there. The one
thing
I'm still a bit concerned about is: Do we need a
CHECK_FOR_INTERRUPTS,
and do we need an upper limit on the spinning. In theory we can spin
with 100% CPU usage, which is not good. So we should either spin a
limited amount of times, or we should perhaps introduce a tiny
delay?
Show quoted text
//Magnus
---------------------------(end of
broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that
your
message can get through to the mailing list cleanly
Would someone generate a patch that includes all the new ideas and post
it here? Thanks.
---------------------------------------------------------------------------
Peter Brant wrote:
Sounds good. I'll check how much we're actually looping too.
Pete
"Magnus Hagander" <mha@sollentuna.net> 04/06/06 10:27 pm >>>
That's probably not a bad idea. AFAIK we haven't had reports of it
elsewhere, but it oculd happen. Want to code up a new patch, and run
some tests?//Magnus
-----Original Message-----
Also, do we want to move the retry loop to pgwin32_recv?
That seems like a good idea. I'm not sure users of recv
should ever have to deal with WSAEWOULDBLOCK as it's not
really an error.Pete
"Magnus Hagander" <mha@sollentuna.net> 04/06/06 9:58 pm >>>
Attached are two patches which in combination make
pg_stat_activity
work reliably for us on Windows.
...
pgstat.patch removes the delayed destroy code for backends,databases,
and tables. Database and table entries are cleaned up
immediately
upon receipt of the appropriate message.
I'll go ahead and apply the delayed-destroy-removal part
(just to HEAD for the time being --- seems a bit risky to
apply it to the stable branches). The Windows-specific
change sounds like it may need more review.Actually, I think that's mostly me being confused and taking others
with
me ;-)It's definitly a problem, and we have a solution there. The one
thing
I'm still a bit concerned about is: Do we need a
CHECK_FOR_INTERRUPTS,
and do we need an upper limit on the spinning. In theory we can spin
with 100% CPU usage, which is not good. So we should either spin a
limited amount of times, or we should perhaps introduce a tinydelay?
//Magnus
---------------------------(end of
broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that
your
message can get through to the mailing list cleanly---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?
--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
I'm still planning on doing this.
I did add a loop around the second WSARecv in pgwin32_recv() as that
was our best guess about where the error was coming from. As it turns
out, that's apparently not the case and I haven't had a chance to come
back to it yet.
It might be that my original patch (putting the loop in piperead()) is
the best approach. All indications are that it fixes the problem and
it's the lowest impact change. I'm still quite curious what API call is
returning the WSAEWOULDBLOCK though...
Pete
Bruce Momjian <pgman@candle.pha.pa.us> 04/19/06 5:07 am >>>
Would someone generate a patch that includes all the new ideas and
post
it here? Thanks.
It turns out the problem is that port/pipe.c is compiled with -DFRONTEND
and include/port/win32.h wraps the recv to pgwin32_recv macro in a
#ifndef FRONTEND. We've actually been using the WinSock recv function
directly (verified with gcc -E).
If somebody else could take over actually fixing this, that would be
great. As I mentioned before, we're heading away from Windows for the
time being.
Pete
Peter Brant 04/19/06 5:16 pm >>>
I'm still planning on doing this.
I did add a loop around the second WSARecv in pgwin32_recv() as that
was our best guess about where the error was coming from. As it turns
out, that's apparently not the case and I haven't had a chance to come
back to it yet.
It might be that my original patch (putting the loop in piperead()) is
the best approach. All indications are that it fixes the problem and
it's the lowest impact change. I'm still quite curious what API call is
returning the WSAEWOULDBLOCK though...
Pete
Bruce Momjian <pgman@candle.pha.pa.us> 04/19/06 5:07 am >>>
Would someone generate a patch that includes all the new ideas and
post
it here? Thanks.
It turns out the problem is that port/pipe.c is compiled with
-DFRONTEND and include/port/win32.h wraps the recv to
pgwin32_recv macro in a #ifndef FRONTEND. We've actually
been using the WinSock recv function directly (verified with gcc -E).
That's definitly wrong.
Looks like this file needs a _srv version in the Makefile. Bruce?
Just a thought - might this affect more things that rely on FRONTEND
defines in the headers? How bad would it be to just make libpgport build
two complete sets of object files, one for server and one for frontend,
instead of special-casing which files are rebuilt?
If somebody else could take over actually fixing this, that
would be great. As I mentioned before, we're heading away
from Windows for the time being.
I can try - though I only have development servers on win32 these days,
and have never been bitten by this actual problem. So we'd still much
appreciate yuor help in testing a solution once it's there..
//Magnus
Import Notes
Resolved by subject fallback
As long as we have a Win32 test server around, I'm happy to help.
If you have a multi-processor development server, it's easy to recreate
though. Just load up the server (pgbench is fine) and wait a few
minutes. Single processor machines don't seem to be affected.
Pete
"Magnus Hagander" <mha@sollentuna.net> 05/02/06 10:13 am >>>
I can try - though I only have development servers on win32 these
days,
and have never been bitten by this actual problem. So we'd still much
appreciate yuor help in testing a solution once it's there..
I take that back. My local Postgres just didn't have stats_block_level
and stats_row_level turned on. It happens on single processor machines
too.
Pete
"Peter Brant" <Peter.Brant@wicourts.gov> 05/02/06 6:29 pm >>>
minutes. Single processor machines don't seem to be affected.
Magnus Hagander wrote:
It turns out the problem is that port/pipe.c is compiled with
-DFRONTEND and include/port/win32.h wraps the recv to
pgwin32_recv macro in a #ifndef FRONTEND. We've actually
been using the WinSock recv function directly (verified with gcc -E).That's definitly wrong.
Looks like this file needs a _srv version in the Makefile. Bruce?
Wow! That is disturbing. I tried to minimize the affect of FRONTEND,
but obviously I never though about the affect on include files that use
FRONTEND. Seems it is only Win32 that used FRONTEND in includes (except
for pg_wchar.h, which is used by libpq and already built independently).
Just a thought - might this affect more things that rely on FRONTEND
defines in the headers? How bad would it be to just make libpgport build
two complete sets of object files, one for server and one for frontend,
instead of special-casing which files are rebuilt?
The attached patch does exactly what you suggest. I think it has to be
patched to 8.0.X, 8.1.X, and HEAD. I will apply in 24-48 hours.
--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Attachments:
/pgpatches/pgporttext/plainDownload+16-16
Right, and the FRONTEND fix should correct this. Sorry you had to do so
much legwork to find my mistake.
---------------------------------------------------------------------------
Peter Brant wrote:
I'm still planning on doing this.
I did add a loop around the second WSARecv in pgwin32_recv() as that
was our best guess about where the error was coming from. As it turns
out, that's apparently not the case and I haven't had a chance to come
back to it yet.It might be that my original patch (putting the loop in piperead()) is
the best approach. All indications are that it fixes the problem and
it's the lowest impact change. I'm still quite curious what API call is
returning the WSAEWOULDBLOCK though...Pete
Bruce Momjian <pgman@candle.pha.pa.us> 04/19/06 5:07 am >>>
Would someone generate a patch that includes all the new ideas and
post
it here? Thanks.---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match
--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Patch applied to HEAD, 8.1.X and 8.0.X.
---------------------------------------------------------------------------
Bruce Momjian wrote:
Magnus Hagander wrote:
It turns out the problem is that port/pipe.c is compiled with
-DFRONTEND and include/port/win32.h wraps the recv to
pgwin32_recv macro in a #ifndef FRONTEND. We've actually
been using the WinSock recv function directly (verified with gcc -E).That's definitly wrong.
Looks like this file needs a _srv version in the Makefile. Bruce?Wow! That is disturbing. I tried to minimize the affect of FRONTEND,
but obviously I never though about the affect on include files that use
FRONTEND. Seems it is only Win32 that used FRONTEND in includes (except
for pg_wchar.h, which is used by libpq and already built independently).Just a thought - might this affect more things that rely on FRONTEND
defines in the headers? How bad would it be to just make libpgport build
two complete sets of object files, one for server and one for frontend,
instead of special-casing which files are rebuilt?The attached patch does exactly what you suggest. I think it has to be
patched to 8.0.X, 8.1.X, and HEAD. I will apply in 24-48 hours.--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com+ If your life is a hard drive, Christ can be your backup. +
Index: src/port/Makefile =================================================================== RCS file: /cvsroot/pgsql/src/port/Makefile,v retrieving revision 1.30 diff -c -c -r1.30 Makefile *** src/port/Makefile 9 Dec 2005 21:19:36 -0000 1.30 --- src/port/Makefile 7 May 2006 01:05:32 -0000 *************** *** 26,37 **** override CPPFLAGS := -I$(top_builddir)/src/port -DFRONTEND $(CPPFLAGS) LIBS += $(PTHREAD_LIBS)! # Replace object files that use FRONTEND define
! LIBOBJS_SRV := $(LIBOBJS)
! LIBOBJS_SRV := $(patsubst dirmod.o,dirmod_srv.o, $(LIBOBJS_SRV))
! LIBOBJS_SRV := $(patsubst exec.o,exec_srv.o, $(LIBOBJS_SRV))
! LIBOBJS_SRV := $(patsubst getaddrinfo.o,getaddrinfo_srv.o, $(LIBOBJS_SRV))
! LIBOBJS_SRV := $(patsubst thread.o,thread_srv.o, $(LIBOBJS_SRV))all: libpgport.a libpgport_srv.a
--- 26,33 ---- override CPPFLAGS := -I$(top_builddir)/src/port -DFRONTEND $(CPPFLAGS) LIBS += $(PTHREAD_LIBS)! # Replace all object files so they use FRONTEND define
! LIBOBJS_SRV := $(LIBOBJS:%.o=%_srv.o)all: libpgport.a libpgport_srv.a
***************
*** 60,72 ****
libpgport_srv.a: $(LIBOBJS_SRV)
$(AR) $(AROPT) $@ $^! dirmod_srv.o: dirmod.c
! $(CC) $(CFLAGS) $(subst -DFRONTEND,, $(CPPFLAGS)) -c $< -o $@
!
! exec_srv.o: exec.c
! $(CC) $(CFLAGS) $(subst -DFRONTEND,, $(CPPFLAGS)) -c $< -o $@
!
! getaddrinfo_srv.o: getaddrinfo.c
$(CC) $(CFLAGS) $(subst -DFRONTEND,, $(CPPFLAGS)) -c $< -o $@# No thread flags for server version --- 56,62 ---- libpgport_srv.a: $(LIBOBJS_SRV) $(AR) $(AROPT) $@ $^! %_srv.o: %.c
$(CC) $(CFLAGS) $(subst -DFRONTEND,, $(CPPFLAGS)) -c $< -o $@# No thread flags for server version
---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend
--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +