libpq WSACleanup is not needed

Andrew Chernow wrote:

WSACleanup is not really needed during a PQfinish. Its horribly slow if
the library ref count is 0 and it actually unloads the winsock library,
adds 225ms to PQfinish.

Solution:
A) Call WSAStartup once and never clean it up. When the app dies, so do
the ref counts and winsock is automatically unloaded.

B) Have a way of specifying the behavior, the way it is now or tell
libpq to not initialize wsa at all (kinda like ssl init callbacks).
Leave it up to the application.

I think the WSA startup/cleanup stuff is silly. If I dynamically link
with a DLL, I want it automatically loaded and cleaned up.

Worst case, your app makes lots of connections to different backends.
So, it is constantly doing PQconnectdb and PQfinish; only has a single
conn open at a time. This means its constantly loading and unloading
winsock.

Option A will make us leak the reference to it though, won't it? And we
are supposed to clean up after ourselves...

If you want to override this behavior today, you can just call
WSAStartup() in your application, and it should never happen. Right?

Now, if we actually had libpq_init()/uninit() or something like it, it
would make sense to move it there. But I'm not sure we want to just leak
the reference. But I'm not entirely convinced either way :-)

//Magnus

Magnus Hagander wrote:

Andrew Chernow wrote:

WSACleanup is not really needed during a PQfinish. Its horribly slow if
the library ref count is 0 and it actually unloads the winsock library,
adds 225ms to PQfinish.

Solution:
A) Call WSAStartup once and never clean it up. When the app dies, so do
the ref counts and winsock is automatically unloaded.

B) Have a way of specifying the behavior, the way it is now or tell
libpq to not initialize wsa at all (kinda like ssl init callbacks).
Leave it up to the application.

I think the WSA startup/cleanup stuff is silly. If I dynamically link
with a DLL, I want it automatically loaded and cleaned up.

Worst case, your app makes lots of connections to different backends.
So, it is constantly doing PQconnectdb and PQfinish; only has a single
conn open at a time. This means its constantly loading and unloading
winsock.

Option A will make us leak the reference to it though, won't it? And we
are supposed to clean up after ourselves...

Personally, I don't think its the job of libpq to call wsa startup or shutdown.
Pulling it out now will surely break existing apps and piss people off, so I
don't think this is an option. If anything, it should not be a per conn thing.
Its really a library wide thing. Think about it, there is no gain in doing
this per conn. Not to mention, I just found a major issue with it.

Now, if we actually had libpq_init()/uninit() or something like it, it
would make sense to move it there. But I'm not sure we want to just leak
the reference. But I'm not entirely convinced either way :-)

There is probably someone out there that wants wsa to completely unload when
there done using libpq (maybe its a long-lived app like an NT service). The
only thing a leaked ref would do is never unload wsa until the app exited. So,
this is probably bad behavior.

libpq_init() is where an app should elect what libpq should load. If init is
never called, everything should work as it does now. Something like that.
openssl init stuff should be controlled by the same function, maybe some other
components. Auto-init'n is a nice feature most of the time, but it suffers from
ASSuming how and when an app wants to perfom the init.

If you want to override this behavior today, you can just call
WSAStartup() in your application, and it should never happen. Right?

Exactely what we did.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

Magnus Hagander wrote:

Andrew Chernow wrote:

WSACleanup is not really needed during a PQfinish. Its horribly slow if
the library ref count is 0 and it actually unloads the winsock library,
adds 225ms to PQfinish.

Solution:
A) Call WSAStartup once and never clean it up. When the app dies, so do
the ref counts and winsock is automatically unloaded.

If you want to override this behavior today, you can just call
WSAStartup() in your application, and it should never happen. Right?

Or perhaps use _init() and _fini() or the Win32 equivalents?

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera wrote:

Magnus Hagander wrote:

Andrew Chernow wrote:

WSACleanup is not really needed during a PQfinish. Its horribly slow if
the library ref count is 0 and it actually unloads the winsock library,
adds 225ms to PQfinish.

Solution:
A) Call WSAStartup once and never clean it up. When the app dies, so do
the ref counts and winsock is automatically unloaded.

If you want to override this behavior today, you can just call
WSAStartup() in your application, and it should never happen. Right?

Or perhaps use _init() and _fini() or the Win32 equivalents?

You are not allowed to call WSAStartup() and WSACleanup() from these,
since they may load/unload DLLs...

I think you can find traces of that in the cvs history if you're
interested :-D

//Magnus

Alvaro Herrera wrote:

Magnus Hagander wrote:

Andrew Chernow wrote:

WSACleanup is not really needed during a PQfinish. Its horribly slow if
the library ref count is 0 and it actually unloads the winsock library,
adds 225ms to PQfinish.

Solution:
A) Call WSAStartup once and never clean it up. When the app dies, so do
the ref counts and winsock is automatically unloaded.

If you want to override this behavior today, you can just call
WSAStartup() in your application, and it should never happen. Right?

Or perhaps use _init() and _fini() or the Win32 equivalents?

The Win32 equivalent is DllMain, which I believe only works when your a
dll. Although, from the WSAStartup docs:

"the WSAStartup function should not be called from the DllMain function
in a application DLL. This can potentially cause deadlocks."

That doesn't sound inviting. C++ static intializers would probably
work, if isolated in some small far away distant project file with an
ugly file name.

Outside of user-land work arounds, the real fix would be to have a libpq
library init and shutdown (shutdown only useful for those wanting to
free resources or re-init libpq differently). Not sure there's any
interest in this idea.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

On 1/16/09, Magnus Hagander <magnus@hagander.net> wrote:

Andrew Chernow wrote:

WSACleanup is not really needed during a PQfinish. Its horribly slow if
the library ref count is 0 and it actually unloads the winsock library,
adds 225ms to PQfinish.

Option A will make us leak the reference to it though, won't it? And we
are supposed to clean up after ourselves...

If you want to override this behavior today, you can just call
WSAStartup() in your application, and it should never happen. Right?

Now, if we actually had libpq_init()/uninit() or something like it, it
would make sense to move it there. But I'm not sure we want to just leak
the reference. But I'm not entirely convinced either way :-)

I think init/uninit is the answer. While writing libpqtypes, we noted
that libpq is just plain awkward in a few different ways and probably
deserves a rewrite at some point. not today though....

merlin

Alvaro Herrera <alvherre@commandprompt.com> writes:

Magnus Hagander wrote:

If you want to override this behavior today, you can just call
WSAStartup() in your application, and it should never happen. Right?

Or perhaps use _init() and _fini() or the Win32 equivalents?

I thought we were already relying on DLL load/unload-time calls
in the Win32 port? Or maybe that was a long time ago and we got
rid of it? I agree with Andrew that that would be a saner place
for this than connection start.

regards, tom lane

Tom Lane wrote:

Alvaro Herrera <alvherre@commandprompt.com> writes:

Magnus Hagander wrote:

If you want to override this behavior today, you can just call
WSAStartup() in your application, and it should never happen. Right?

Or perhaps use _init() and _fini() or the Win32 equivalents?

I thought we were already relying on DLL load/unload-time calls
in the Win32 port? Or maybe that was a long time ago and we got
rid of it? I agree with Andrew that that would be a saner place
for this than connection start.

We had it, and we removed it because the Win32 API docs say you're not
allowed to do it that way because of deadlock risks.

//Magnus

Andrew Chernow wrote:

Magnus Hagander wrote:

Andrew Chernow wrote:

WSACleanup is not really needed during a PQfinish. Its horribly slow if
the library ref count is 0 and it actually unloads the winsock library,
adds 225ms to PQfinish.

Solution:
A) Call WSAStartup once and never clean it up. When the app dies, so do
the ref counts and winsock is automatically unloaded.

B) Have a way of specifying the behavior, the way it is now or tell
libpq to not initialize wsa at all (kinda like ssl init callbacks).
Leave it up to the application.

I think the WSA startup/cleanup stuff is silly. If I dynamically link
with a DLL, I want it automatically loaded and cleaned up.

Worst case, your app makes lots of connections to different backends.
So, it is constantly doing PQconnectdb and PQfinish; only has a single
conn open at a time. This means its constantly loading and unloading
winsock.

Option A will make us leak the reference to it though, won't it? And we
are supposed to clean up after ourselves...

Personally, I don't think its the job of libpq to call wsa startup or
shutdown. Pulling it out now will surely break existing apps and piss

I'm afraid it is. Looking at the API docs, the very first paragraph says:
"The WSAStartup function must be the first Windows Sockets function
called by an application or DLL. It allows an application or DLL to
specify the version of Windows Sockets required and retrieve details of
the specific Windows Sockets implementation. The application or DLL can
only issue further Windows Sockets functions after successfully calling
WSAStartup."

Simply put: The API requires we do it.

people off, so I don't think this is an option. If anything, it should
not be a per conn thing. Its really a library wide thing. Think about
it, there is no gain in doing this per conn. Not to mention, I just
found a major issue with it.

That is true, however. I'm not sure I'll agree it's a major issue, but
it's certainly sub-optimal.

The use-case of rapidly creating and dropping connections isn't
particularly common, I think. And there is a perfectly functioning
workaround - something that we should perhaps document in the FAQ or
somewhere in the documentation?

Now, if we actually had libpq_init()/uninit() or something like it, it
would make sense to move it there. But I'm not sure we want to just leak
the reference. But I'm not entirely convinced either way :-)

There is probably someone out there that wants wsa to completely unload
when there done using libpq (maybe its a long-lived app like an NT
service). The only thing a leaked ref would do is never unload wsa
until the app exited. So, this is probably bad behavior.

No, it can also hold internal WSA references and structures.

libpq_init() is where an app should elect what libpq should load. If
init is never called, everything should work as it does now. Something
like that. openssl init stuff should be controlled by the same function,
maybe some other components. Auto-init'n is a nice feature most of the
time, but it suffers from ASSuming how and when an app wants to perfom
the init.

Yeah. So the question is, do we want to bite the bullet and create
init() and uninit() functions for libpq? It'll be a new version of
libpq, and apps will require the new version in order to use it, so it's
a fairly large change to the API.. However, having *had* one, would
certainly have made the SSL fixes that Bruce put in earlier a lot
simpler....

//Magnus

Magnus Hagander <magnus@hagander.net> writes:

Yeah. So the question is, do we want to bite the bullet and create
init() and uninit() functions for libpq?

I think if calling them is an optimization that improves performance
(by eliminating per-connection-start overhead), this could fly. If
the plan is "we are going to require applications to call these
or they'll break", it's not going to be acceptable ...

regards, tom lane

Tom Lane wrote:

Magnus Hagander <magnus@hagander.net> writes:

Yeah. So the question is, do we want to bite the bullet and create
init() and uninit() functions for libpq?

I think if calling them is an optimization that improves performance
(by eliminating per-connection-start overhead), this could fly. If
the plan is "we are going to require applications to call these
or they'll break", it's not going to be acceptable ...

regards, tom lane

My suggestion was to make calling the init/uninit optional (well uninit should
only be optional if init was not called). I think libpq should behave
identically if init() is never called. What init() gets you is the ability to
fine tune libpq (change the default behaviors). For instance: a bit mask
argument to init() called "options", that allows one to toggle things on/off in
libpq: like PG_OPT_NOWSAINIT or PG_OPT_NOSSLINIT. It may requrie something like
to be expandable:

int
PQinit(int info_type, void *info_struct, int options);

I'm just spit-ball'n here. My point is, its could be a good place to allow run
time configuration of libpq.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

Personally, I don't think its the job of libpq to call wsa startup or
shutdown. Pulling it out now will surely break existing apps and piss

I'm afraid it is. Looking at the API docs, the very first paragraph says:
"The WSAStartup function must be the first Windows Sockets function
called by an application or DLL. It allows an application or DLL to
specify the version of Windows Sockets required and retrieve details of
the specific Windows Sockets implementation. The application or DLL can
only issue further Windows Sockets functions after successfully calling
WSAStartup."

I just think libpq should provide a way of turning off built in startups.
Outside of my original per-conn performance concern, an application may want
libpq to use a version of winsock that is different than the one libpq is using.
After reading the very handy doc blurb you so graciously supplied, it appears
to be one of the main reasons wsastartup exists ;-)

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

Magnus Hagander wrote:

The use-case of rapidly creating and dropping connections isn't
particularly common, I think. And there is a perfectly functioning
workaround - something that we should perhaps document in the FAQ or
somewhere in the documentation?

Would it be accetable to do initialise if the number of connections is
changing from 0, and
tidy if the cumber goes back to 0? Applications that retain a
connection would not
suffer the cost on subsequent connect/disconnect.

The init/term is the tidiest way to do it, but the above might help -
perhaps init could just
add a phantom usecount and work the same way.

If you have a DLL for libpq, could you do it in process attach and
detach? Wouldn't
that be the most common case anyway?

James

James Mansion wrote:

If you have a DLL for libpq, could you do it in process attach and
detach? Wouldn't
that be the most common case anyway?

m$ docs indicate that wsastartup can't be called from dllmain :(

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

Andrew Chernow wrote:

m$ docs indicate that wsastartup can't be called from dllmain :(

OK, fair cop. Says it in the MSDN online version but not in the SDK 6.1
version. :-( Some helper(s)
must start threads I guess.

Re the counting and doing it on first/last socket - of course WSAStartup
counts internally. Prsumably
its only slow when the count is actually going to zero?

Is there a need for a new API to control this - can't you just interpret
another parameter keyword
in PQconnectdb (or the pgoptions string in PQsetdbLogin I guess)?
(Having said that, how
do you control send and receive buffer sizes? Presumably nagle is
always disabled, but those
features could be controlled the same way? Would presumably allow
PGOPTIONS to be
parsed too, though docs 30.12 says that does runtime options for the
server, though
PQconnectdb in 30.1 suggests that it might be parsed for the client
connect too. Maybe.).

James

James Mansion wrote:

Andrew Chernow wrote:

m$ docs indicate that wsastartup can't be called from dllmain :(

OK, fair cop. Says it in the MSDN online version but not in the SDK 6.1
version. :-( Some helper(s)
must start threads I guess.

That, and it loads other DLLs as well.

Re the counting and doing it on first/last socket - of course WSAStartup
counts internally. Prsumably
its only slow when the count is actually going to zero?

Yes.

Is there a need for a new API to control this - can't you just interpret
another parameter keyword
in PQconnectdb (or the pgoptions string in PQsetdbLogin I guess)?
(Having said that, how
do you control send and receive buffer sizes? Presumably nagle is
always disabled, but those
features could be controlled the same way? Would presumably allow
PGOPTIONS to be
parsed too, though docs 30.12 says that does runtime options for the
server, though
PQconnectdb in 30.1 suggests that it might be parsed for the client
connect too. Maybe.).

You can always get the socket descriptor back and modify it directly, if
you are sure that what you're changing is supported..

//Magnus

James Mansion wrote:

Is there a need for a new API to control this - can't you just interpret
another parameter keyword
in PQconnectdb (or the pgoptions string in PQsetdbLogin I guess)?

That's an interesting idea. I don't know if its the correct place to control
this, but it does allow turning off wsastartup and indicating which winsock
version to load.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/

Andrew Chernow wrote:

James Mansion wrote:

Is there a need for a new API to control this - can't you just
interpret another parameter keyword
in PQconnectdb (or the pgoptions string in PQsetdbLogin I guess)?

That's an interesting idea. I don't know if its the correct place to
control this, but it does allow turning off wsastartup and indicating
which winsock version to load.

From a design perspective it seems like the wrong place to put it if you
think of it as a general initialization. From the narrow perspective of
wsastartup, it could work to add an option to inhibit it. But will it
"scale" to future needs?

//Magnus

Magnus Hagander wrote:

Andrew Chernow wrote:

James Mansion wrote:

Is there a need for a new API to control this - can't you just
interpret another parameter keyword
in PQconnectdb (or the pgoptions string in PQsetdbLogin I guess)?

That's an interesting idea. I don't know if its the correct place to
control this, but it does allow turning off wsastartup and indicating
which winsock version to load.

From a design perspective it seems like the wrong place to put it if you

think of it as a general initialization. From the narrow perspective of
wsastartup, it could work to add an option to inhibit it. But will it
"scale" to future needs?

//Magnus

yeah, it might be a stretch. It also ignores the other needs for a
library init().

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/