Re: libpq WSACleanup is not needed - Mailing list pgsql-hackers

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