Thread: libpq thread-locking
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... Comments? //Magnus
Attachment
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
Attachment
! 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