SEO
vBulletin Search Engine Optimization
| |||||||
| Register | FAQ | Members List | Calendar | Search | Today's Posts | Mark Forums Read |
04-19-2008, 06:28 AM
| ||||
| ||||
 Re: libpq Win32 Mutex performance patch Andrew Chernow <ac@esilo.com> writes: > The attached patch replaces the win32 mutex calls with critical section > calls. The change will not affect the behavior of the windows > pthread_xxx functions. Why have you defined the lock/unlock functions as willing to fall through silently if handed a null pointer? I think a crash in such a case is what we *want*. Silently not locking is surely not very safe. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches      |
|
04-19-2008, 06:28 AM
| |||
| |||
| Re: libpq Win32 Mutex performance patch Tom Lane wrote: > Andrew Chernow <ac@esilo.com> writes: >> The attached patch replaces the win32 mutex calls with critical section >> calls. The change will not affect the behavior of the windows >> pthread_xxx functions. > > Why have you defined the lock/unlock functions as willing to fall > through silently if handed a null pointer? I think a crash in > such a case is what we *want*. Silently not locking is surely > not very safe. > > regards, tom lane > Yeah, both naughty. These functions were not implemented to spec. These pthread functions are all supposed to return an int (which is an errno value). I was trying not to change the existing prototypes ... should I? I can return EINVAL if something is NULL and ENOMEM if the malloc fails ... or just dump core. If you like the return value idea, I can make all occurances of pthread_xxx check the return value. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches |
|
04-19-2008, 06:28 AM
| |||
| |||
| Re: libpq Win32 Mutex performance patch Andrew Chernow wrote: > Tom Lane wrote: >> Silently not locking is surely >> not very safe. >> > > Here is the dump code version of the patch. If anyone wants the return > value idea, let me know. > > > ------------------------------------------------------------------------ > > A more graceful solution would be to print something to stderr and then exit. This allows an app's atexit funcs to run. I don't think libpq should core dump an app by choice. I'd be pretty upset if a library I was using core dumped in some cases rather than exiting. andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches |
|
04-19-2008, 06:28 AM
| |||
| |||
| Re: libpq Win32 Mutex performance patch Andrew Chernow <ac@esilo.com> writes: > A more graceful solution would be to print something to stderr and then > exit. stderr doesn't exist, or point to a useful place, in many environments. And a forced exit() is no better than a crash for most purposes. > I don't think libpq should core dump an app by choice. The case that we are talking about is a bug, or would be a bug if it could happen (the fact that we've gotten along happily with no similar test in the existing code shows that it can't). Many forms of bug can result in core dumps; it's foolish to try to prevent them all. And bloating one line of code into five or more lines to defend against can't-happen cases is a good way to end up with unreadable, unmaintainable software. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches |
|
04-19-2008, 06:28 AM
| |||
| |||
| Re: libpq Win32 Mutex performance patch On Fri, Apr 11, 2008 at 06:25:53PM -0400, Tom Lane wrote: > Andrew Chernow <ac@esilo.com> writes: > > A more graceful solution would be to print something to stderr and then > > exit. > > stderr doesn't exist, or point to a useful place, in many environments. > And a forced exit() is no better than a crash for most purposes. > > > I don't think libpq should core dump an app by choice. > > The case that we are talking about is a bug, or would be a bug if it > could happen (the fact that we've gotten along happily with no similar > test in the existing code shows that it can't). Many forms of bug can > result in core dumps; it's foolish to try to prevent them all. And > bloating one line of code into five or more lines to defend against > can't-happen cases is a good way to end up with unreadable, > unmaintainable software. > > regards, tom lane +1 -dg -- David Gould daveg@sonic.net 510 536 1443 510 282 0869 If simplicity worked, the world would be overrun with insects. -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches |
|
04-19-2008, 06:28 AM
| |||
| |||
| Re: libpq Win32 Mutex performance patch daveg wrote: > On Fri, Apr 11, 2008 at 06:25:53PM -0400, Tom Lane wrote: >> Andrew Chernow <ac@esilo.com> writes: >>> A more graceful solution would be to print something to stderr and then >>> exit. >> stderr doesn't exist, or point to a useful place, in many environments. >> And a forced exit() is no better than a crash for most purposes. >> >>> I don't think libpq should core dump an app by choice. >> The case that we are talking about is a bug, or would be a bug if it >> could happen (the fact that we've gotten along happily with no similar >> test in the existing code shows that it can't). Many forms of bug can >> result in core dumps; it's foolish to try to prevent them all. And >> bloating one line of code into five or more lines to defend against >> can't-happen cases is a good way to end up with unreadable, >> unmaintainable software. >> >> regards, tom lane > > +1 > > -dg okay. BTW, my real interest here is the libpq hooks patch requires a lock/unlock for every conn-create, conn-destroy, result-create, result-destroy. Currently, it looks like only the ssl stuff uses mutexes. Adding more mutex use is a good case for a more optimized approach on windows. andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches |
|
04-19-2008, 06:28 AM
| |||
| |||
| Re: libpq Win32 Mutex performance patch Andrew Chernow wrote: > Tom Lane wrote: > > Andrew Chernow <ac@esilo.com> writes: > >> The attached patch replaces the win32 mutex calls with critical > >> section calls. The change will not affect the behavior of the > >> windows pthread_xxx functions. > > > > Why have you defined the lock/unlock functions as willing to fall > > through silently if handed a null pointer? I think a crash in > > such a case is what we *want*. Silently not locking is surely > > not very safe. > > > > regards, tom lane > > > > Yeah, both naughty. > > These functions were not implemented to spec. These pthread > functions are all supposed to return an int (which is an errno > value). I was trying not to change the existing prototypes ... > should I? I can return EINVAL if something is NULL and ENOMEM if the > malloc fails ... or just dump core. > > If you like the return value idea, I can make all occurances of > pthread_xxx check the return value. Getting these emails in the wrong order for some reason :-( Yes, actually checking the return values from these functions in libpq seems like a good idea to me. ISTM that we already have the case where we can fall through silently when failing to lock, and that should be fixed. It looks like the internal API of pgthreadlock_t needs to be changed as well in this case, because it can currently only reaturn void. But - it also seems we are not actually *using* it anywhere. Perhaps that part of the API should simply be removed? //Magnus -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches |
|
04-24-2008, 06:15 PM
| |||
| |||
| Re: libpq Win32 Mutex performance patch Andrew Chernow wrote: > Tom Lane wrote: > >Silently not locking is surely > > not very safe. > > > > Here is the dump code version of the patch. If anyone wants the > return value idea, let me know. Here's a version of this patch that doesn't use malloc - instead, I had to change the callers to it a bit. This makes a difference only on Vista+ really, because prior to Vista the function InitializeCriticalSection() *can* fail - it will throw an exception and not return any error code. Which really isn't that different from just crashing like this latest patch from Andrew would have us do (on out of memory). I'm leaning towards going with the simpler one, which is the patch from Andrew that crashes on out of memory. Comments/preferences? //Magnus -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches |
|
Today, 01:43 PM
| |||
| |||
| Re: libpq Win32 Mutex performance patch Andrew Chernow wrote: > Tom Lane wrote: >> Silently not locking is surely >> not very safe. >> > > Here is the dump code version of the patch. If anyone wants the return > value idea, let me know. So is this a patch we want applied? -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches |
|
Today, 01:43 PM
| ||||
| ||||
| Re: libpq Win32 Mutex performance patch Alvaro Herrera wrote: > Andrew Chernow wrote: > > Tom Lane wrote: > >> Silently not locking is surely > >> not very safe. > >> > > > > Here is the dump code version of the patch. If anyone wants the > > return value idea, let me know. > > So is this a patch we want applied? Please see my other thread about libpq thread-locking which should be finished before this one, after which this patch will change. So no, this is not the version to be applied. //Magnus -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches |
Linear Mode
