Re: pgsql: Remove unsafe calling of WSAStartup and WSA Cleanup from DllMain.

No, it shouldn't.

First, when on mingw, the file with WSAStartup() in it wasn't even
linked in. And this is the DLL that we've been distributing in the MSI.

It wasn't? Ok... Anything else we've missed? :-p

Probably :-) Let me know when you find it.

Second, they really shouldn't rely on that anyway - I don't think we've
documented anywhere that libpq does this ;-) Because I assume you are
referring to applications that use *other* winsock functions, but don't
call WSAStartup() themselves?

Yeah - I'm not saying it's right having now seen the arguments
against, but it happens. For example, do you recall us being confused
when we found we needed to call it in slon.exe?

Eh, no, actually not. Sorry.

Because libpq still calls wsastartup on
the first attempt to open a connection.

Hmm, ok - did it always?

Seems it was added in 1.255 which was back in July 2003. For 7.4. So not
always, but it's not exactly new.

//Magnus

Magnus Hagander wrote:

For example, do you recall us being confused
when we found we needed to call it in slon.exe?

Eh, no, actually not. Sorry.

Well, it was only a couple of years ago!! Seriously though, from what I
recall that was the origin of this code - you left it out because libpq
called WSAStartup on your system, and I added it because on mine it
didn't. Or something like that. I remember us getting confused about it
on IM anyway.

#ifdef WIN32
/*
* Startup the network subsystem, in case our libpq doesn't
*/
err = WSAStartup(MAKEWORD(1, 1), &wsaData);
if (err != 0) {
slon_log(SLON_FATAL, "main: Cannot start the network
subsystem - %d\n", err);
slon_exit(-1);
}
#endif

/D

On Fri, Mar 09, 2007 at 08:16:12AM +0000, Dave Page wrote:

Magnus Hagander wrote:

For example, do you recall us being confused
when we found we needed to call it in slon.exe?

Eh, no, actually not. Sorry.

Well, it was only a couple of years ago!! Seriously though, from what I
recall that was the origin of this code - you left it out because libpq
called WSAStartup on your system, and I added it because on mine it
didn't. Or something like that. I remember us getting confused about it
on IM anyway.

Hmm. Was that actually fort he libpq stuff, though? I don't recall it
clearly, but somethign tells me the problem was around the pipe
emulation and not around libpq.

In which case it can simply be because I was building against a libpq
built with MSVC = it had the broken startup code, and you used a mingw
one, which didn't have it.

Per the docs, an application like slon *should* make it's own call to
WSAStartup() because it uses socket functions diretly (port/pipe.c)...

Another question related to backpatching - should I backpatch this to
8.1 and 8.0 as well? I know we said we more or less don't maintain the
win32 port back there because it was too new, but this is all code in
the client libpq, which has been around no win32 much longer. The reason
I'm asking is that the original reporter of this problem is on 8.1...

I'm leaning towards yes, but would like to hear further comments...

//Magnus

Magnus Hagander wrote:

In which case it can simply be because I was building against a libpq
built with MSVC = it had the broken startup code, and you used a mingw
one, which didn't have it.

Maybe - but it does imply it's potentially easy to break code with this
change.

Per the docs, an application like slon *should* make it's own call to
WSAStartup() because it uses socket functions diretly (port/pipe.c)...

Another question related to backpatching - should I backpatch this to
8.1 and 8.0 as well? I know we said we more or less don't maintain the
win32 port back there because it was too new, but this is all code in
the client libpq, which has been around no win32 much longer. The reason
I'm asking is that the original reporter of this problem is on 8.1...

I'm leaning towards yes, but would like to hear further comments...

I'm far from convinced it should be backpatched at all.

/D

Dave Page <dpage@postgresql.org> writes:

Magnus Hagander wrote:

Another question related to backpatching - should I backpatch this to
8.1 and 8.0 as well?

I'm far from convinced it should be backpatched at all.

I tend to agree with Dave --- I think this change needs to go through a
beta-testing cycle before we unleash it on the world.

regards, tom lane