Thread: 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 unloadsthe 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 existingapps 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. Thinkabout 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 notsure 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 blurbyou 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@momjian.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@momjian.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@momjian.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>), youcan use <function>PQinitSSL(int)</> to tell <application>libpq</application> that the <acronym>SSL</> library has alreadybeen initialized by your application. -- Bruce Momjian <bruce@momjian.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@momjian.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@momjian.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@momjian.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/cleanupoften)- 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@esilo.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@esilo.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/ Index: doc/src/sgml/libpq.sgml =================================================================== RCS file: /projects/cvsroot/pgsql/doc/src/sgml/libpq.sgml,v retrieving revision 1.275 diff -C6 -r1.275 libpq.sgml *** doc/src/sgml/libpq.sgml 10 Jan 2009 20:14:30 -0000 1.275 --- doc/src/sgml/libpq.sgml 22 Jan 2009 16:51:31 -0000 *************** *** 59,72 **** is obtained from the function <function>PQconnectdb</> or <function>PQsetdbLogin</>. Note that these functions will always return a non-null object pointer, unless perhaps there is too little memory even to allocate the <structname>PGconn</> object. The <function>PQstatus</> function should be called to check whether a connection was successfully made before queries are sent ! via the connection object. ! <variablelist> <varlistentry> <term><function>PQconnectdb</function><indexterm><primary>PQconnectdb</></></term> <listitem> <para> Makes a new connection to the database server. --- 59,84 ---- is obtained from the function <function>PQconnectdb</> or <function>PQsetdbLogin</>. Note that these functions will always return a non-null object pointer, unless perhaps there is too little memory even to allocate the <structname>PGconn</> object. The <function>PQstatus</> function should be called to check whether a connection was successfully made before queries are sent ! via the connection object. For windows applications, destroying a ! <structname>PGconn</> can be expensive in a few case. ! <footnote> ! <para> ! 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. ! </para> ! </footnote> <variablelist> <varlistentry> <term><function>PQconnectdb</function><indexterm><primary>PQconnectdb</></></term> <listitem> <para> Makes a new connection to the database server.
Andrew Chernow wrote: > Tom Lane wrote: >> Andrew Chernow <ac@esilo.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/ Index: doc/src/sgml/libpq.sgml =================================================================== RCS file: /projects/cvsroot/pgsql/doc/src/sgml/libpq.sgml,v retrieving revision 1.275 diff -C6 -r1.275 libpq.sgml *** doc/src/sgml/libpq.sgml 10 Jan 2009 20:14:30 -0000 1.275 --- doc/src/sgml/libpq.sgml 22 Jan 2009 17:13:09 -0000 *************** *** 59,72 **** is obtained from the function <function>PQconnectdb</> or <function>PQsetdbLogin</>. Note that these functions will always return a non-null object pointer, unless perhaps there is too little memory even to allocate the <structname>PGconn</> object. The <function>PQstatus</> function should be called to check whether a connection was successfully made before queries are sent ! via the connection object. ! <variablelist> <varlistentry> <term><function>PQconnectdb</function><indexterm><primary>PQconnectdb</></></term> <listitem> <para> Makes a new connection to the database server. --- 59,84 ---- is obtained from the function <function>PQconnectdb</> or <function>PQsetdbLogin</>. Note that these functions will always return a non-null object pointer, unless perhaps there is too little memory even to allocate the <structname>PGconn</> object. The <function>PQstatus</> function should be called to check whether a connection was successfully made before queries are sent ! via the connection object. For windows applications, destroying a ! <structname>PGconn</> can be expensive in a few cases. ! <footnote> ! <para> ! 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. When calling WSACleanup ! with a reference count of zero, all resources will be freed and all ! DLLs will be unloaded. This is an expensive operation that can take ! as much as 300ms. The overhead can be seen if an application does ! not call WSAStartup and it closes its last <structname>PGconn</>. To avoid this, ! an application should manually call WSAStartup. ! </para> ! </footnote> <variablelist> <varlistentry> <term><function>PQconnectdb</function><indexterm><primary>PQconnectdb</></></term> <listitem> <para> Makes a new connection to the database server.
Andrew Chernow wrote: > Andrew Chernow wrote: > > Tom Lane wrote: > >> Andrew Chernow <ac@esilo.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + Index: doc/src/sgml/libpq.sgml =================================================================== RCS file: /cvsroot/pgsql/doc/src/sgml/libpq.sgml,v retrieving revision 1.275 diff -c -c -r1.275 libpq.sgml *** doc/src/sgml/libpq.sgml 10 Jan 2009 20:14:30 -0000 1.275 --- doc/src/sgml/libpq.sgml 6 Feb 2009 18:17:55 -0000 *************** *** 63,68 **** --- 63,83 ---- The <function>PQstatus</> function should be called to check whether a connection was successfully made before queries are sent via the connection object. + + <note> + <para> + On Windows, there is a way to improve performance if a single + database connection is repeated started and shutdown. Internally, + libpq calls WSAStartup() and WSACleanup() for connection startup + and shutdown, respectively. WSAStartup() increments an internal + Windows library reference count which is decremented by WSACleanup(). + When the reference count is just one, calling WSACleanup() frees + all resources and all DLLs are unloaded. This is an expensive + operation. To avoid this, an application can manually call + WSAStartup() so resources will not be freed when the last database + connection is closed. + </para> + </note> <variablelist> <varlistentry>
Bruce Momjian wrote: > Andrew Chernow wrote: >> Andrew Chernow wrote: >>> Tom Lane wrote: >>>> Andrew Chernow <ac@esilo.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@esilo.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +