Re: libpq WSACleanup is not needed

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.

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

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(at)hagander(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)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(at)hagander(dot)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(at)hagander(dot)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/

James Mansion wrote:
> 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.

Yes, we do that now to clear the SSL callbacks in libpq (see
variable 'ssl_open_connections'):

CRYPTO_set_locking_callback(NULL);
CRYPTO_set_id_callback(NULL);

If we don't remove them when going to zero connections then unloading
libpq can cause PHP to crash because it thinks the callback functions
are still loaded.

We could have gone with a more elegant init/uninit solution but there is
a history of slow upstream adoption of libpq API changes.

In this case I am thinking WSACleanup() should probably follow the same
pattern. Having load/unload is superior, but if adoption of that API is
<10%, you will probably get the most advantage for the most users in
making it automatic.

The one big difference with SSL is that the SSL callback unload calls
where cheap, while WSACleanup() is expensive.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
>
> We could have gone with a more elegant init/uninit solution but there is
> a history of slow upstream adoption of libpq API changes.
>
>

If that's the case, adding a connectdb option seems like a good
alternative. Orignally suggested here:

http://archives.postgresql.org/pgsql-hackers/2009-01/msg01358.php

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

Andrew Chernow wrote:
> Bruce Momjian wrote:
> >
> > We could have gone with a more elegant init/uninit solution but there is
> > a history of slow upstream adoption of libpq API changes.
> >
> >
>
> If that's the case, adding a connectdb option seems like a good
> alternative. Orignally suggested here:
>
> http://archives.postgresql.org/pgsql-hackers/2009-01/msg01358.php

Right, well the big question is how many people are going to use the
connection option vs. doing it for everyone automatically.

One possible approach might be to do it automatically, and allow a
connection option to disable the WSACleanup() call.

Actually, right now, if you have two libpq connections, and close one,
does WSACleanup() get called, and does it affect the existing
connection?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Andrew Chernow wrote:
>> Bruce Momjian wrote:
>>> We could have gone with a more elegant init/uninit solution but there is
>>> a history of slow upstream adoption of libpq API changes.
>>>
>>>
>> If that's the case, adding a connectdb option seems like a good
>> alternative. Orignally suggested here:
>>
>> http://archives.postgresql.org/pgsql-hackers/2009-01/msg01358.php
>
> Right, well the big question is how many people are going to use the
> connection option vs. doing it for everyone automatically.
>
> One possible approach might be to do it automatically, and allow a
> connection option to disable the WSACleanup() call.

I think that was the suggestion. Have an option that would disable
*both* the startup and the cleanup call, leaving the responsibility to
the app.

You can do this for SSL today by calling PQinitSSL().

> Actually, right now, if you have two libpq connections, and close one,
> does WSACleanup() get called, and does it affect the existing
> connection?

WSACleanup() gets called, but it has an internal reference count so it
does not have any effect on existing connections.

//Magnus

Magnus Hagander wrote:
> Bruce Momjian wrote:
> > Andrew Chernow wrote:
> >> Bruce Momjian wrote:
> >>> We could have gone with a more elegant init/uninit solution but there is
> >>> a history of slow upstream adoption of libpq API changes.
> >>>
> >>>
> >> If that's the case, adding a connectdb option seems like a good
> >> alternative. Orignally suggested here:
> >>
> >> http://archives.postgresql.org/pgsql-hackers/2009-01/msg01358.php
> >
> > Right, well the big question is how many people are going to use the
> > connection option vs. doing it for everyone automatically.
> >
> > One possible approach might be to do it automatically, and allow a
> > connection option to disable the WSACleanup() call.
>
> I think that was the suggestion. Have an option that would disable
> *both* the startup and the cleanup call, leaving the responsibility to
> the app.
>
> You can do this for SSL today by calling PQinitSSL().

Right.

> > Actually, right now, if you have two libpq connections, and close one,
> > does WSACleanup() get called, and does it affect the existing
> > connection?
>
> WSACleanup() gets called, but it has an internal reference count so it
> does not have any effect on existing connections.

Ah, OK, so it does its own cleanup on last close, great. I agree a
connection option for this would be good.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Merlin Moncure wrote:

> 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....

Would there be any serious harm in:

1. doing the WSAStartup() when the first connection is opened, but
2. cleaning up from either
2a. some kind of on-unload version of DllMain() if possible, or
2b. an explicit new cleanup function

?

(I know it says you can't set the thing up from DllMain, but does it say
something similar about shutdown?)

Jeroen

Jeroen Vermeulen wrote:
>
> Would there be any serious harm in:
>
> 1. doing the WSAStartup() when the first connection is opened, but
>

The only problem is how to detect the first connection. In a threaded
environment you'd have to perform locking in connectdb, which is
probably not going to fly.

>>but does it say something similar about shutdown?

From the WSACleanup docs:

"The WSACleanup function typically leads to protocol-specific helper
DLLs being unloaded. As a result, the WSACleanup function should not be
called from the DllMain function in a application DLL. This can
potentially cause deadlocks"

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

Bruce Momjian wrote:
>
> Ah, OK, so it does its own cleanup on last close, great. I agree a
> connection option for this would be good.
>

What would the option be? "wsainit = [enable | disable]"? Maybe it
should allow setting the version to load: "wsa_version = 2.0". Maybe
the two should be combined: "wsa_version = [default | disable | 2.0]".

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

Andrew Chernow wrote:
> Bruce Momjian wrote:
>>
>> Ah, OK, so it does its own cleanup on last close, great. I agree a
>> connection option for this would be good.
>>
>
> What would the option be? "wsainit = [enable | disable]"? Maybe it
> should allow setting the version to load: "wsa_version = 2.0". Maybe
> the two should be combined: "wsa_version = [default | disable | 2.0]".
>

I will say, the cleanest solution is still an optional init()/uninit()
for libpq. Has this been ruled out? IMHO, the next best solution is a
connection option.

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

Andrew Chernow wrote:
> Bruce Momjian wrote:
> >
> > Ah, OK, so it does its own cleanup on last close, great. I agree a
> > connection option for this would be good.
> >
>
> What would the option be? "wsainit = [enable | disable]"? Maybe it
> should allow setting the version to load: "wsa_version = 2.0". Maybe
> the two should be combined: "wsa_version = [default | disable | 2.0]".

I assumed it would be like SSL, which is a libpq function call, not a
connection option, e.g. PQinitSSL(), and I think true/false is probably
enough. PQinitSSL info:

If you are using <acronym>SSL</> inside your application (in addition
to inside <application>libpq</application>), you can use
<function>PQinitSSL(int)</> to tell <application>libpq</application>
that the <acronym>SSL</> library has already been initialized by your
application.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Andrew Chernow wrote:
> Andrew Chernow wrote:
>> Bruce Momjian wrote:
>>>
>>> Ah, OK, so it does its own cleanup on last close, great. I agree a
>>> connection option for this would be good.
>>>
>>
>> What would the option be? "wsainit = [enable | disable]"? Maybe it
>> should allow setting the version to load: "wsa_version = 2.0". Maybe
>> the two should be combined: "wsa_version = [default | disable | 2.0]".
>>
>
> I will say, the cleanest solution is still an optional init()/uninit()
> for libpq. Has this been ruled out? IMHO, the next best solution is
> a connection option.

What happened to the idea of counting connections? That seemed a
relatively clean way to go, I thought, although I haven't followed the
discussion very closely.

cheers

andrew

Andrew Dunstan wrote:
>
>
> Andrew Chernow wrote:
> > Andrew Chernow wrote:
> >> Bruce Momjian wrote:
> >>>
> >>> Ah, OK, so it does its own cleanup on last close, great. I agree a
> >>> connection option for this would be good.
> >>>
> >>
> >> What would the option be? "wsainit = [enable | disable]"? Maybe it
> >> should allow setting the version to load: "wsa_version = 2.0". Maybe
> >> the two should be combined: "wsa_version = [default | disable | 2.0]".
> >>
> >
> > I will say, the cleanest solution is still an optional init()/uninit()
> > for libpq. Has this been ruled out? IMHO, the next best solution is
> > a connection option.
>
> What happened to the idea of counting connections? That seemed a
> relatively clean way to go, I thought, although I haven't followed the
> discussion very closely.

I was told WSACleanup does connection counting internally (only the
final close has a performance impact) so there is no need to do the
counting like we do for SSL callbacks.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Andrew Chernow wrote:
>> Bruce Momjian wrote:
>>> Ah, OK, so it does its own cleanup on last close, great. I agree a
>>> connection option for this would be good.
>>>
>> What would the option be? "wsainit = [enable | disable]"? Maybe it
>> should allow setting the version to load: "wsa_version = 2.0". Maybe
>> the two should be combined: "wsa_version = [default | disable | 2.0]".
>
> I assumed it would be like SSL, which is a libpq function call, not a
> connection option, e.g. PQinitSSL(), and I think true/false is probably
> enough. PQinitSSL info:
>
> If you are using <acronym>SSL</> inside your application (in addition
> to inside <application>libpq</application>), you can use
> <function>PQinitSSL(int)</> to tell <application>libpq</application>
> that the <acronym>SSL</> library has already been initialized by your
> application.
>

That smells dirty to me. How many PQinitXXX() functions are needed
before we drop the XXX and run with PQinit(...)?

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

Andrew Chernow wrote:
> Bruce Momjian wrote:
> > Andrew Chernow wrote:
> >> Bruce Momjian wrote:
> >>> Ah, OK, so it does its own cleanup on last close, great. I agree a
> >>> connection option for this would be good.
> >>>
> >> What would the option be? "wsainit = [enable | disable]"? Maybe it
> >> should allow setting the version to load: "wsa_version = 2.0". Maybe
> >> the two should be combined: "wsa_version = [default | disable | 2.0]".
> >
> > I assumed it would be like SSL, which is a libpq function call, not a
> > connection option, e.g. PQinitSSL(), and I think true/false is probably
> > enough. PQinitSSL info:
> >
> > If you are using <acronym>SSL</> inside your application (in addition
> > to inside <application>libpq</application>), you can use
> > <function>PQinitSSL(int)</> to tell <application>libpq</application>
> > that the <acronym>SSL</> library has already been initialized by your
> > application.
> >
>
> That smells dirty to me. How many PQinitXXX() functions are needed
> before we drop the XXX and run with PQinit(...)?

Odds are you would still need per-library control over initialization so
I am not sure that helps, i.e. the library initialized WSA already but
needs SSL.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Andrew Chernow wrote:
>> Bruce Momjian wrote:
>>> Andrew Chernow wrote:
>>>> Bruce Momjian wrote:
>>>>> Ah, OK, so it does its own cleanup on last close, great. I agree a
>>>>> connection option for this would be good.
>>>>>
>>>> What would the option be? "wsainit = [enable | disable]"? Maybe it
>>>> should allow setting the version to load: "wsa_version = 2.0". Maybe
>>>> the two should be combined: "wsa_version = [default | disable | 2.0]".
>>> I assumed it would be like SSL, which is a libpq function call, not a
>>> connection option, e.g. PQinitSSL(), and I think true/false is probably
>>> enough. PQinitSSL info:
>>>
>>> If you are using <acronym>SSL</> inside your application (in addition
>>> to inside <application>libpq</application>), you can use
>>> <function>PQinitSSL(int)</> to tell <application>libpq</application>
>>> that the <acronym>SSL</> library has already been initialized by your
>>> application.
>>>
>> That smells dirty to me. How many PQinitXXX() functions are needed
>> before we drop the XXX and run with PQinit(...)?
>
> Odds are you would still need per-library control over initialization so
> I am not sure that helps, i.e. the library initialized WSA already but
> needs SSL.
>

That's fine. I solved that issue here:

http://archives.postgresql.org/pgsql-hackers/2009-01/msg01349.php

One of arguments is an "options" bit mask. PG_OPT_LOADSSL,
PG_OPT_LOADWSA, etc... I also suggested a "int inittype, void
*initdata" arguments that can be used now or for future expansion; that
way PQinit is not limited to a single int argument. This could be used
right away with the PG_OPT_LOADWSA idea, to pass the wsa version you want.

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

Andrew Chernow wrote:
> Bruce Momjian wrote:
> > Andrew Chernow wrote:
> >> Bruce Momjian wrote:
> >>> Andrew Chernow wrote:
> >>>> Bruce Momjian wrote:
> >>>>> Ah, OK, so it does its own cleanup on last close, great. I agree a
> >>>>> connection option for this would be good.
> >>>>>
> >>>> What would the option be? "wsainit = [enable | disable]"? Maybe it
> >>>> should allow setting the version to load: "wsa_version = 2.0". Maybe
> >>>> the two should be combined: "wsa_version = [default | disable | 2.0]".
> >>> I assumed it would be like SSL, which is a libpq function call, not a
> >>> connection option, e.g. PQinitSSL(), and I think true/false is probably
> >>> enough. PQinitSSL info:
> >>>
> >>> If you are using <acronym>SSL</> inside your application (in addition
> >>> to inside <application>libpq</application>), you can use
> >>> <function>PQinitSSL(int)</> to tell <application>libpq</application>
> >>> that the <acronym>SSL</> library has already been initialized by your
> >>> application.
> >>>
> >> That smells dirty to me. How many PQinitXXX() functions are needed
> >> before we drop the XXX and run with PQinit(...)?
> >
> > Odds are you would still need per-library control over initialization so
> > I am not sure that helps, i.e. the library initialized WSA already but
> > needs SSL.
> >
>
> That's fine. I solved that issue here:
>
> http://archives.postgresql.org/pgsql-hackers/2009-01/msg01349.php
>
> One of arguments is an "options" bit mask. PG_OPT_LOADSSL,
> PG_OPT_LOADWSA, etc... I also suggested a "int inittype, void
> *initdata" arguments that can be used now or for future expansion; that
> way PQinit is not limited to a single int argument. This could be used
> right away with the PG_OPT_LOADWSA idea, to pass the wsa version you want.

That seems overly complex to support just two init functions (we only
had one for SSL for years).

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Andrew Chernow wrote:
> The only problem is how to detect the first connection. In a threaded
> environment you'd have to perform locking in connectdb, which is
> probably not going to fly.
Well, if you do an atomic test for a flag being zero, and if so then
enter a critsec, do
the init iff you're the first in, and then set the flag on the way out,
then:
- most times, you'll just have the atomic test
- other times, you have a short critsec

I can't see that being a big deal considering you're about to resolve
the server hostname
and then do a TCP/IP connect.

My understanding is that if you do WSAStartup and WSACleanup scoped to
each connection
then:
- the internal counting means that only the 0 -> 1 and 1 -> 0
transitions are expensive
- libpq will only incur the cost if the application didn't do it already

So it seems that the cost is incurred by an application that:
- makes no other use of winsock (or also does startup/cleanup often)
- does not retain a connection (or pool) but creates and closes
a single connection often

How many applications are there that match this pattern? Isn't it
enough just to tell
the user to do WSAStartup and WSACleanup in main() if they find they
have a performance problem? Surely most Windows programs effectively do
that
anyway, often as a side effect of using a framework.

James

James Mansion wrote:
> Andrew Chernow wrote:
>> The only problem is how to detect the first connection. In a threaded
>> environment you'd have to perform locking in connectdb, which is
>> probably not going to fly.
> Well, if you do an atomic test for a flag being zero, and if so then
> enter a critsec, do

This is not a problem, we do this in other places in libpq already.

> My understanding is that if you do WSAStartup and WSACleanup scoped to
> each connection
> then:
> - the internal counting means that only the 0 -> 1 and 1 -> 0
> transitions are expensive
> - libpq will only incur the cost if the application didn't do it already

Yes.

> So it seems that the cost is incurred by an application that:
> - makes no other use of winsock (or also does startup/cleanup often)
> - does not retain a connection (or pool) but creates and closes
> a single connection often

Correct.

> How many applications are there that match this pattern? Isn't it
> enough just to tell
> the user to do WSAStartup and WSACleanup in main() if they find they
> have a performance problem? Surely most Windows programs effectively do
> that
> anyway, often as a side effect of using a framework.

Yeah, I think an important point here is: If you are willing to call a
special PQinitWinsock() or whatever, then you can just call WSAStartup()
yourself, and the problem goes away...

I guess adding a connection parameter might help a little bit in that
you don't need an extra API call, but I'm unsure if it's worth it given
that the workaround is so simple.

In which case, we should perhaps just document the workaround using
WSAStartup() yourself, and not bother with either API or connection
parameter...

//Magnus

Magnus Hagander wrote:
>
> In which case, we should perhaps just document the workaround using
> WSAStartup() yourself, and not bother with either API or connection
> parameter...
>
>

I didn't originally agree with this but now I do. Any libpq init function for
wsa, would only be replacing an app calling WSAStartup themselves. So, why have
it at all.

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

Andrew Chernow wrote:
> Magnus Hagander wrote:
>>
>> In which case, we should perhaps just document the workaround using
>> WSAStartup() yourself, and not bother with either API or connection
>> parameter...
>>
>>
>
> I didn't originally agree with this but now I do. Any libpq init
> function for wsa, would only be replacing an app calling WSAStartup
> themselves. So, why have it at all.

Ok, I think we're fairly agreed that this is the way to proceed then. Do
you want to propose some wording for the documentation, or should I try
to write something myself?

//Magnus

Magnus Hagander wrote:
> Andrew Chernow wrote:
>> Magnus Hagander wrote:
>>> In which case, we should perhaps just document the workaround using
>>> WSAStartup() yourself, and not bother with either API or connection
>>> parameter...
>>>
>>>
>> I didn't originally agree with this but now I do. Any libpq init
>> function for wsa, would only be replacing an app calling WSAStartup
>> themselves. So, why have it at all.
>
> Ok, I think we're fairly agreed that this is the way to proceed then. Do
> you want to propose some wording for the documentation, or should I try
> to write something myself?
>
> //Magnus
>
>

I can try. Where should this be documented? ISTM that the best place
is at the top of "30.1 Database Connection Control Functions", since the
issue pertains to any connect/disconnect function. Does that sound
correct? Should it be a warning or just regular text?

First attempt:

"On windows, libpq issues a WSAStartup and WSACleanup on a per
connection basis. Each WSAStartup increments an internal reference
count which is decremented by WSACleanup. Calling WSACleanup with a
reference count of zero, forces all resources to be freed and DLLs to be
unloaded. This is an expensive operation that can take as much as
300ms. This overhead can be seen if an application does not call
WSAStartup and it closes its last PGconn. To avoid this, an application
should manually call WSAStartup."

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

Andrew Chernow <ac(at)esilo(dot)com> writes:
> I can try. Where should this be documented? ISTM that the best place
> is at the top of "30.1 Database Connection Control Functions", since the
> issue pertains to any connect/disconnect function. Does that sound
> correct? Should it be a warning or just regular text?

Minor platform-specific performance nits are not that important. Think
"footnote", not "warning at the top of the page".

regards, tom lane

Tom Lane wrote:
> Andrew Chernow <ac(at)esilo(dot)com> writes:
>> I can try. Where should this be documented? ISTM that the best place
>> is at the top of "30.1 Database Connection Control Functions", since the
>> issue pertains to any connect/disconnect function. Does that sound
>> correct? Should it be a warning or just regular text?
>
> Minor platform-specific performance nits are not that important. Think
> "footnote", not "warning at the top of the page".
>
> regards, tom lane
>
>

Its a footnote at the end of the first paragraph in 30.1.

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

Andrew Chernow wrote:
> Tom Lane wrote:
>> Andrew Chernow <ac(at)esilo(dot)com> writes:
>>> I can try. Where should this be documented? ISTM that the best
>>> place is at the top of "30.1 Database Connection Control Functions",
>>> since the issue pertains to any connect/disconnect function. Does
>>> that sound correct? Should it be a warning or just regular text?
>>
>> Minor platform-specific performance nits are not that important. Think
>> "footnote", not "warning at the top of the page".
>>
> Its a footnote at the end of the first paragraph in 30.1.

Fixed a few things.

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

Andrew Chernow wrote:
> Andrew Chernow wrote:
> > Tom Lane wrote:
> >> Andrew Chernow <ac(at)esilo(dot)com> writes:
> >>> I can try. Where should this be documented? ISTM that the best
> >>> place is at the top of "30.1 Database Connection Control Functions",
> >>> since the issue pertains to any connect/disconnect function. Does
> >>> that sound correct? Should it be a warning or just regular text?
> >>
> >> Minor platform-specific performance nits are not that important. Think
> >> "footnote", not "warning at the top of the page".
> >>
> > Its a footnote at the end of the first paragraph in 30.1.
>
> Fixed a few things.

I have applied a modified version of this documentation patch, attached.
Thanks.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Andrew Chernow wrote:
>> Andrew Chernow wrote:
>>> Tom Lane wrote:
>>>> Andrew Chernow <ac(at)esilo(dot)com> writes:
>>>>> I can try. Where should this be documented? ISTM that the best
>>>>> place is at the top of "30.1 Database Connection Control Functions",
>>>>> since the issue pertains to any connect/disconnect function. Does
>>>>> that sound correct? Should it be a warning or just regular text?
>>>> Minor platform-specific performance nits are not that important. Think
>>>> "footnote", not "warning at the top of the page".
>>>>
>>> Its a footnote at the end of the first paragraph in 30.1.
>> Fixed a few things.
>
> I have applied a modified version of this documentation patch, attached.
> Thanks.
>

"connection is repeated started and shutdown."

"repeated" should be "repeatedly".

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

Andrew Chernow wrote:
> Bruce Momjian wrote:
> > Andrew Chernow wrote:
> >> Andrew Chernow wrote:
> >>> Tom Lane wrote:
> >>>> Andrew Chernow <ac(at)esilo(dot)com> writes:
> >>>>> I can try. Where should this be documented? ISTM that the best
> >>>>> place is at the top of "30.1 Database Connection Control Functions",
> >>>>> since the issue pertains to any connect/disconnect function. Does
> >>>>> that sound correct? Should it be a warning or just regular text?
> >>>> Minor platform-specific performance nits are not that important. Think
> >>>> "footnote", not "warning at the top of the page".
> >>>>
> >>> Its a footnote at the end of the first paragraph in 30.1.
> >> Fixed a few things.
> >
> > I have applied a modified version of this documentation patch, attached.
> > Thanks.
> >
>
> "connection is repeated started and shutdown."
>
> "repeated" should be "repeatedly".

Thanks, fixed.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +