APC/socket fix (final?)

Thanks. I was getting to that, but hadn't started :-)

Per our discussion off-list, I agree with this method, and the patch
looks fine to me.

//Magnus

Claudio Natoli <claudio.natoli@memetrics.com> writes:

+ #ifdef WIN32
+ 	/* Interrupted by socket/APC interaction? */
+ 	if (n < 0 && GetLastError() == ERROR_IO_PENDING)
+ 		errno = EINTR;
+ #endif

This seems a bit schizophrenic; if you can assign to errno, why can't you
read from it? Would look more consistent if the code looked like

if (n < 0 && errno == ERROR_IO_PENDING)
errno = EINTR;

regards, tom lane

Claudio Natoli <claudio.natoli@memetrics.com> writes:

+ #ifdef WIN32
+ 	/* Interrupted by socket/APC interaction? */
+ 	if (n < 0 && GetLastError() == ERROR_IO_PENDING)
+ 		errno = EINTR;
+ #endif

This seems a bit schizophrenic; if you can assign to errno,
why can't you
read from it? Would look more consistent if the code looked like

if (n < 0 && errno == ERROR_IO_PENDING)
errno = EINTR;

The problem is that winsock does not *set* the errno variable in the
case when it's interrupted by an APC. errno is left at 0.
GetLastError() is the win32 replacement for errno. In most cases errno
is set correctly when you use the "unix api functions", but not in this
case (which is arguably a bug, but it's there nevertheless). If you use
"native win32 functions", you get GetLastError() set only.

//Magnus

Would look more consistent if the code looked like

if (n < 0 && errno == ERROR_IO_PENDING)
errno = EINTR;

It would be more consistent, but unfortunately GetLastError() != errno.

--- 
Certain disclaimers and policies apply to all email sent from Memetrics.
For the full text of these disclaimers and policies see 
<a
href="http://www.memetrics.com/emailpolicy.html">http://www.memetrics.com/em
ailpolicy.html</a>

Claudio Natoli <claudio.natoli@memetrics.com> writes:

It would be more consistent, but unfortunately GetLastError() != errno.

Yeah, I saw Magnus' explanation. So essentially this is a workaround
for a bug in Windows' select() emulation.

Rather than hoping that we'll remember to decorate every select() call
with this workaround, would it make sense to use a wrapper function?
I'm loath to invent pg_select() but it might be cleaner than this.

regards, tom lane

Rather than hoping that we'll remember to decorate every select() call
with this workaround, would it make sense to use a wrapper function?
I'm loath to invent pg_select() but it might be cleaner than this.

We'd also need pg_recv() and pg_send(). Chances are it can happen with every
blocking socket call :-(

Cheers,
Claudio

--- 
Certain disclaimers and policies apply to all email sent from Memetrics.
For the full text of these disclaimers and policies see 
<a
href="http://www.memetrics.com/emailpolicy.html">http://www.memetrics.com/em
ailpolicy.html</a>

Claudio Natoli <claudio.natoli@memetrics.com> writes:

I'm loath to invent pg_select() but it might be cleaner than this.

We'd also need pg_recv() and pg_send(). Chances are it can happen with every
blocking socket call :-(

Ugh. Is there a way we can insert a wrapper layer without modifying the
call sites? I'm thinking of some kind of macro hack, say

#ifdef WIN32
#define select(...) pg_select(...)
#endif

and then provide a port module that goes roughly like

#undef select

pg_select(...)
{
foo = select(...);
// fix errno here;
return foo;
}

The fewer places that have to know about this sort of thing, the better
off we will be.

regards, tom lane

Ugh. Is there a way we can insert a wrapper layer without modifying the
call sites? I'm thinking of some kind of macro hack, say
[snip]

Sure. Think we've even done this before (also, prevents developers needing
to remember to use pg_*).

The reason I think it was avoided for select(), in preference for a thread
to invoke the socket op during the signal/APC, was a fear that perhaps the
Windows Sockets internals could get mashed. AFAICS, the discussion Magnus
had with the Microsoft guys (and, from memory, those I've had with Magnus
off-list) suggests this isn't true. If mashing the internals is still a
possibility, then clearly the patch I've submitted might do more harm than
good.

(Magnus, can you confirm?)

If so, I'll submit a patch for select/recv/send over the weekend, which will
also remove the recent fixes for pgstat.

Cheers,
Claudio

--- 
Certain disclaimers and policies apply to all email sent from Memetrics.
For the full text of these disclaimers and policies see 
<a
href="http://www.memetrics.com/emailpolicy.html">http://www.memetrics.com/em
ailpolicy.html</a>

Ugh. Is there a way we can insert a wrapper layer without

modifying the

call sites? I'm thinking of some kind of macro hack, say
[snip]

Sure. Think we've even done this before (also, prevents
developers needing to remember to use pg_*).

Yup, it's done to redefine kill() to pqkill(). See include/port/win32.h.

The reason I think it was avoided for select(), in preference
for a thread
to invoke the socket op during the signal/APC, was a fear that
perhaps the
Windows Sockets internals could get mashed. AFAICS, the
discussion Magnus
had with the Microsoft guys (and, from memory, those I've had
with Magnus
off-list) suggests this isn't true. If mashing the internals is still a
possibility, then clearly the patch I've submitted might do
more harm than
good.

(Magnus, can you confirm?)

Yes, it can be done (for select, so I guess for recv and send). It is
recommended that we do not do it that way, though.

There is also a third option, which I'm leaning more and more towards. I
was in that direction in the beginning, but turned away from it thinking
it would be too much work. Also, I was under the impression that we
would rather have some "workarounds checking errors etc" than
reimplement code. I'm now getting the idea that we'd rather have it the
other way around, assuming the code is in the port dir. Fair enough.

The third option is to redefine all these functions into our own, and
implement our own emulation layer. This means our own select(), send(),
recv() (more? I don't think so). And have these call the native winsock
APIs (WSAEventSelect(), WSASend(), WSARecv() etc). These functions are
designed to work in an APC environment.

This is going to be a bit more code that can go wrong, but it carries
the advatages that (1) it's win32 only code, and (2) the code will all
be in the port directory. We implement it along with the standard Unix
semantics, so the main code should not notice. Doing this, we can back
out the changes done so far (the thread fix, the errno fix).

It's the "sledgehammer fix", but I think it's probably time to bite the
bullet and do this one. This way, it's *not* a workaround. Any surprises
we end up with should be from our own code.

If so, I'll submit a patch for select/recv/send over the
weekend, which will
also remove the recent fixes for pgstat.

If you think my suggestion above is not a good one then yes, we can do
this one. But I don't feel all that good about it (and specifically, I
have not had it confirmed about recv() and send() yet - have asked, but
not received a response yet).

If not, I'm going to get started on an implementation using the WSA
functions for you to check over :-)

//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:

The third option is to redefine all these functions into our own, and
implement our own emulation layer. This means our own select(), send(),
recv() (more? I don't think so). And have these call the native winsock
APIs (WSAEventSelect(), WSASend(), WSARecv() etc). These functions are
designed to work in an APC environment.

Pardon me for not having paid close enough attention, but what versions
of select() and friends are we relying on now? Is it really reasonable
to think that we can do a better job without a large amount of effort?

regards, tom lane

The third option is to redefine all these functions into our own, and
implement our own emulation layer. This means our own

select(), send(),

recv() (more? I don't think so). And have these call the

native winsock

APIs (WSAEventSelect(), WSASend(), WSARecv() etc). These

functions are

designed to work in an APC environment.

Pardon me for not having paid close enough attention, but what versions
of select() and friends are we relying on now? Is it really reasonable
to think that we can do a better job without a large amount of effort?

oh yeah, I should add that these "new-style" functions are very similar
to the original functions in syntax and semantics - they're just changed
to interface with the win32 style of blocking/signalling/waiting.

//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:

I *think* we can do a better job with a not-so-large amount of effort,
...
I'm willing to give it a try. If it turns out to be a lot of work, or
way too much code, I'll drop the idea.

Sure, give it a shot and see what you get.

regards, tom lane

Hi all,

Magnus Hagander wrote:

The third option is to redefine all these functions into our own, and
implement our own emulation layer. This means our own select(), send(),
recv() (more? I don't think so). And have these call the native winsock
APIs (WSAEventSelect(), WSASend(), WSARecv() etc). These functions are
designed to work in an APC environment.

I personally think this is the last possible option; I'd much rather stay as
close to *nix as possible. As you said, and I agree whole-heartedly that
there is a lot more code that can go wrong, and it seems unnecessary if we
have a suitable workaround, which I think we might.

If you think my suggestion above is not a good one then yes, we can do
this one. But I don't feel all that good about it (and specifically, I
have not had it confirmed about recv() and send() yet - have asked, but
not received a response yet).

Reading back through our discussions, I'm not at all concerned about recv()
+ send(), as they are well behaved. When interrupted, they SetLastError()
reasonably, and return < 0.

select(), however, is a different story. On APC interrupt, it returns a
seemingly valid value, doesn't set any of
errno/WSAGetLastError/GetLastError, and leaves the FD_SETs set, which we
concluded was impossible to work-around. However, it just occured to me that
we could wrap select() by augmenting the read_mask with an addition socket,
that we know will never be touched, and checking this socket on a "valid"
return. If this socket is still set, we know we got bitten by the APC/socket
interaction bug, and can set errno accordingly.

I've attached a proposal for /port. Comments?

Cheers,
Claudio

--- 
Certain disclaimers and policies apply to all email sent from Memetrics.
For the full text of these disclaimers and policies see 
<a
href="http://www.memetrics.com/emailpolicy.html">http://www.memetrics.com/em
ailpolicy.html</a>

Claudio Natoli <claudio.natoli@memetrics.com> writes:

... However, it just occured to me that
we could wrap select() by augmenting the read_mask with an addition socket,
that we know will never be touched, and checking this socket on a "valid"
return. If this socket is still set, we know we got bitten by the APC/socket
interaction bug, and can set errno accordingly.

What happens if the socket() call fails? Even if it succeeds, I don't
know what the semantics are of selecting on an un-bound socket ... it
might perhaps show as error state, for instance.

regards, tom lane

Tom Lane wrote:

Claudio Natoli <claudio.natoli@memetrics.com> writes:

... However, it just occured to me that
we could wrap select() by augmenting the read_mask with an

addition socket,

that we know will never be touched, and checking this

socket on a "valid"

return. If this socket is still set, we know we got bitten

by the APC/socket

interaction bug, and can set errno accordingly.

What happens if the socket() call fails?

Then we return in error. Refer to attachment.

Even if it succeeds, I don't know what the semantics are of selecting on

an un-bound socket ... it

might perhaps show as error state, for instance.

select doesn't need a socket to be bound to select on it, afaik. In any
case, it isn't necessary under win32, which is what we are discussing.

Cheers,
Claudio

--- 
Certain disclaimers and policies apply to all email sent from Memetrics.
For the full text of these disclaimers and policies see 
<a
href="http://www.memetrics.com/emailpolicy.html">http://www.memetrics.com/em
ailpolicy.html</a>

Tom Lane wrote:

What happens if the socket() call fails?

Then we return in error. Refer to attachment.

Doh. You *were* referring to the attachment. Ok, so it'll need a check to
make sure the socket actually does get created, but this isn't a fatal flaw,
right? [is a work in progress I bashed out about 30 minutes ago].

I'm actually pretty hopeful over this patch, passes regression tests just
fine, lets us remove the win32_pgstat cruft, let's us easily mirror the unix
semantics, ...

Hopeless, or cute, work-around?

Cheers,
Claudio

--- 
Certain disclaimers and policies apply to all email sent from Memetrics.
For the full text of these disclaimers and policies see 
<a
href="http://www.memetrics.com/emailpolicy.html">http://www.memetrics.com/em
ailpolicy.html</a>

Claudio Natoli <claudio.natoli@memetrics.com> writes:

Hopeless, or cute, work-around?

It's possibly workable in the limited context of the postmaster, but
I've got doubts about doing it in libpq where we can't assume we know
what the surrounding application will do.

regards, tom lane

Hopeless, or cute, work-around?

It's possibly workable in the limited context of the postmaster, but
I've got doubts about doing it in libpq where we can't assume we know
what the surrounding application will do.

No need to touch the frontend parts at all. Our APCs are server side
only, so it's not a problem there.

It should be just as with the kill defrine in win32.h - "#ifndef
FRONTEND".

To the other point - yes, it's a cute workaround. I'm a bit afraid that
we'll end up with another surprise down the road though, since the
behaviour of the select() function is known to be undefined in this
situation (or at leas tthe return value of it is). I'm not sure we can
rely on the pattern we're seeing now. We also don't know for sure about
send() and recv() (though I'm working on that).

I'm almost done with the "our own select()/recv()/send() functions"
attempt. I feel that's a more correct solution (not just a workaround),
*if* I can get the code good enough that we feel comfortable with it. If
not, then a workaround seems the way to go, and then Claudios one looks
good (except we might need to go with a global variable instead - but
the general way, and the localization of it looks good)

//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:

Hopeless, or cute, work-around?

It's possibly workable in the limited context of the postmaster, but
I've got doubts about doing it in libpq where we can't assume we know
what the surrounding application will do.

No need to touch the frontend parts at all. Our APCs are server side
only, so it's not a problem there.

Oh really? What if the surrounding app uses APCs?

regards, tom lane