libpq thread-locking

Magnus Hagander wrote:

Attached patch adds some error checking to the thread locking stuff in
libpq. Previously, if thread locking failed for some reason, we would
just fall through and do things without locking. This patch makes us
abort() instead. It's not the greatest thing probably, but our API
doesn't let us pass back return values...

I have looked over the patch and it seems fine, though I am concerned
about the abort() case with no output. I realize stderr might be going
nowhere, but in fe-print.c we do an fprintf(stderr) for memory failures
so for consistency I think we should do the same here. If there is
concern about code bloat, I suggest a macro at the top of the file for
thread failure exits:

#define THEAD_FAILURE(str) \
do { \
fprintf(stderr, libpq_gettext("Thread failure: %s\n")); \
exit(1); \
} while(0)

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

Magnus Hagander wrote:

Attached patch adds some error checking to the thread locking stuff in
libpq. Previously, if thread locking failed for some reason, we would
just fall through and do things without locking. This patch makes us
abort() instead. It's not the greatest thing probably, but our API
doesn't let us pass back return values...

I have looked over the patch and it seems fine, though I am concerned
about the abort() case with no output. I realize stderr might be going
nowhere, but in fe-print.c we do an fprintf(stderr) for memory failures
so for consistency I think we should do the same here. If there is
concern about code bloat, I suggest a macro at the top of the file for
thread failure exits:

#define THEAD_FAILURE(str) \
do { \
fprintf(stderr, libpq_gettext("Thread failure: %s\n")); \
exit(1); \
} while(0)

Oh, this is Tom saying he doesn't like stderr and the added code lines
for failure:

http://archives.postgresql.org/pgsql-patches/2008-04/msg00254.php

I think the macro and consistency suggest doing as I outlined.

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

Bruce Momjian wrote:

Magnus Hagander wrote:

Attached patch adds some error checking to the thread locking
stuff in libpq. Previously, if thread locking failed for some
reason, we would just fall through and do things without locking.
This patch makes us abort() instead. It's not the greatest thing
probably, but our API doesn't let us pass back return values...

I have looked over the patch and it seems fine, though I am
concerned about the abort() case with no output. I realize stderr
might be going nowhere, but in fe-print.c we do an fprintf(stderr)
for memory failures so for consistency I think we should do the
same here. If there is concern about code bloat, I suggest a macro
at the top of the file for thread failure exits:

#define THEAD_FAILURE(str) \
do { \
fprintf(stderr, libpq_gettext("Thread failure:
%s\n")); \ exit(1); \
} while(0)

Oh, this is Tom saying he doesn't like stderr and the added code lines
for failure:

http://archives.postgresql.org/pgsql-patches/2008-04/msg00254.php

I think the macro and consistency suggest doing as I outlined.

Does this one look like what you're suggesting?

//Magnus

! int
pthread_mutex_init(pthread_mutex_t *mp, void *attr)
{
*mp = CreateMutex(0, 0, 0);
+ if (*mp == NULL)
+ return 1;
+ return 0;
}

Maybe it would be better to emulate what pthreads does. Instead of
returing 1 to indicate an error, return an errno. In the above case,
ENOMEM seems like a good fit.

Also, maybe you should check the passed in mutex pointer. If its NULL,
you could return EINVAL.

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

Andrew Chernow wrote:

! int
pthread_mutex_init(pthread_mutex_t *mp, void *attr)
{
*mp = CreateMutex(0, 0, 0);
+ if (*mp == NULL)
+ return 1;
+ return 0;
}

Maybe it would be better to emulate what pthreads does. Instead of
returing 1 to indicate an error, return an errno. In the above case,
ENOMEM seems like a good fit.

Also, maybe you should check the passed in mutex pointer. If its
NULL, you could return EINVAL.

Given that we only call this stuff internally, I don't think it's a big
issue. But either way - that part of the code will be replaced with the
critical_section code later anyway - I just want to get the "generic"
changes through first.

//Magnus

Magnus Hagander wrote:

Bruce Momjian wrote:

Bruce Momjian wrote:

Magnus Hagander wrote:

Attached patch adds some error checking to the thread locking
stuff in libpq. Previously, if thread locking failed for some
reason, we would just fall through and do things without locking.
This patch makes us abort() instead. It's not the greatest thing
probably, but our API doesn't let us pass back return values...

I have looked over the patch and it seems fine, though I am
concerned about the abort() case with no output. I realize stderr
might be going nowhere, but in fe-print.c we do an fprintf(stderr)
for memory failures so for consistency I think we should do the
same here. If there is concern about code bloat, I suggest a macro
at the top of the file for thread failure exits:

#define THEAD_FAILURE(str) \
do { \
fprintf(stderr, libpq_gettext("Thread failure:
%s\n")); \ exit(1); \
} while(0)

Oh, this is Tom saying he doesn't like stderr and the added code lines
for failure:

http://archives.postgresql.org/pgsql-patches/2008-04/msg00254.php

I think the macro and consistency suggest doing as I outlined.

Does this one look like what you're suggesting?

Yep.

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

Magnus Hagander wrote:

Bruce Momjian wrote:

Bruce Momjian wrote:

Magnus Hagander wrote:

Attached patch adds some error checking to the thread locking
stuff in libpq. Previously, if thread locking failed for some
reason, we would just fall through and do things without
locking. This patch makes us abort() instead. It's not the
greatest thing probably, but our API doesn't let us pass back
return values...

I have looked over the patch and it seems fine, though I am
concerned about the abort() case with no output. I realize
stderr might be going nowhere, but in fe-print.c we do an
fprintf(stderr) for memory failures so for consistency I think
we should do the same here. If there is concern about code
bloat, I suggest a macro at the top of the file for thread
failure exits:

#define THEAD_FAILURE(str) \
do { \
fprintf(stderr, libpq_gettext("Thread failure:
%s\n")); \ exit(1); \
} while(0)

Oh, this is Tom saying he doesn't like stderr and the added code
lines for failure:

http://archives.postgresql.org/pgsql-patches/2008-04/msg00254.php

I think the macro and consistency suggest doing as I outlined.

Does this one look like what you're suggesting?

Yep.

Ok, applied.

//Magnus