thread-safe libpq and DBD::Pg

Goran Thyni <goran@bildbasen.se> writes:

Has anyone any experience of the thread-safeness of libpq?

PQconnectdb() is not thread-safe, because it scribbles temporary
data in the static PQconninfoOptions array.  You can get around
that easily enough by not using PQconnectdb (or PQconndefaults)
--- use PQsetdbLogin() instead.  In any case the problem exists
only if different threads try to open database connections
at the same time.

As far as I can think at the moment, ordinary operations while
the database connection is open should be thread safe. That's
assuming that your libc (esp. malloc/free) is thread safe of course.
It also assumes that only one thread is using any one PGconn
object (at a time, anyway). Multiple threads using the same
PGconn to issue concurrent requests definitely will not work.

There is also some thread risk during connection shutdown because
libpq momentarily commandeers the SIGPIPE handler. I am thinking
it'd be nice to get rid of that code if possible --- see discussion
from a couple weeks ago on the hackers (or was it interfaces?) list.

I had thought a little bit about ripping out PQconnectdb's use of
changeable fields in PQconninfoOptions, but it looks like this
could break applications that access PQconninfoOptions.
Does anyone have any feelings about that, pro or con?

regards, tom lane

I wrote:

Goran Thyni <goran@bildbasen.se> writes:

Has anyone any experience of the thread-safeness of libpq?

There is also some thread risk during connection shutdown because
libpq momentarily commandeers the SIGPIPE handler. I am thinking
it'd be nice to get rid of that code if possible --- see discussion
from a couple weeks ago on the hackers (or was it interfaces?) list.

I have removed that SIGPIPE hacking from closePGconn() in a patch
just submitted to pgsql-patches; that's one less thread safety violation
in libpq. However, whilst I was poking about in the libpq innards I
noticed a couple of other ones :-(.

PQprint() also manipulates the SIGPIPE handler in order to prevent
application termination if the pager subprocess it starts up decides
to quit early. This is a good thing, I think, but it does create
a problem if the app is multi-threaded and other threads would prefer
a different SIGPIPE setting. (I suppose if signal handlers are
thread-local in your environment then it's a non-issue anyway.)

I also noticed that PQoidStatus() returns a pointer to a static
char array, which is a classic thread-safety booboo. I am thinking
that the cleanest solution is for it to copy the OID number into
conn->errorMessage and return a pointer to that. This would mean
that the value would not be good after the next operation on the
PGconn object, but I wouldn't imagine that any apps hold onto the
pointer very long anyway --- they probably always feed it to atoi
immediately. (Actually, why the dickens doesn't this routine
return an Oid value directly? It could return InvalidOid in
case of trouble, just like PQftype does... but I suppose we don't
want to break application code just to make the API a tad cleaner.)

regards, tom lane

I also noticed that PQoidStatus() returns a pointer to a static
char array, which is a classic thread-safety booboo. I am thinking
that the cleanest solution is for it to copy the OID number into
conn->errorMessage and return a pointer to that. This would mean
that the value would not be good after the next operation on the
PGconn object, but I wouldn't imagine that any apps hold onto the
pointer very long anyway --- they probably always feed it to atoi
immediately. (Actually, why the dickens doesn't this routine
return an Oid value directly? It could return InvalidOid in
case of trouble, just like PQftype does... but I suppose we don't
want to break application code just to make the API a tad cleaner.)

Yes, it is very silly that PQoidStatus() returns a char*. I believe it
was only done that way because it was pulling the oid out of the
cmdStatus field. We really should return an int. Perhaps we don't
because we would be hardcoding the oid type on the client side. By
default, we return all types as char *. I don't think we do this
anywhere else in the protocol.

This is a complete guess. I believe it was added by Vadim in 6.2 or
6.3.

Perhaps we could double-use the cmdStatus field, perhaps when they ask
for the oid, we put a null after the oid value, and return a pointer to
the inside of the cmdStatus field. If the call for PQcmdStatus(), we
remove the null and return the string. Strange, I admit.

-- 
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)

Tom Lane wrote:

Goran Thyni <goran@bildbasen.se> writes:

Has anyone any experience of the thread-safeness of libpq?

PQconnectdb() is not thread-safe, because it scribbles temporary
data in the static PQconninfoOptions array.  You can get around
that easily enough by not using PQconnectdb (or PQconndefaults)
--- use PQsetdbLogin() instead.  In any case the problem exists
only if different threads try to open database connections
at the same time.
[...}

Tom,
thanks for the reply.
DBD::Pg dos indeed use PQconnectdb(),
but at least in my light test I get SEGV in $sth->fetch.
The problems are probably somewhere in DBDI or DBD::Pg or
somewhere in the interaction of the parts.

I am looking into how much work a "pure Perl" interface
should amount to. Using native perl IO and pgsql-fe/be-protocol
should make "pgsql-clienting" even more portable over platforms.

regards,
G�ran

Bruce Momjian <maillist@candle.pha.pa.us> writes:

Yes, it is very silly that PQoidStatus() returns a char*. I believe it
was only done that way because it was pulling the oid out of the
cmdStatus field. We really should return an int. Perhaps we don't
because we would be hardcoding the oid type on the client side.

Type Oid is already visible to libpq clients: it's returned by PQftype()
as the type code for attributes. I think encouraging clients to store
oids as type Oid rather than some-int-type-chosen-at-random would be a
good thing, not a bad thing.

However, improving the cleanliness of the API might not be worth the
cost of breaking application code, even though this routine is probably
not very widely used. Does anyone else have an opinion pro or con?

Perhaps we could double-use the cmdStatus field, perhaps when they ask
for the oid, we put a null after the oid value, and return a pointer to
the inside of the cmdStatus field. If the call for PQcmdStatus(), we
remove the null and return the string. Strange, I admit.

I had a variant of that idea: what's stored in cmdStatus looks like
INSERT oidvalue count
but there's a fair amount of room left over (cmdStatus is 40 bytes ...
and we could make it bigger if we wanted). We can make PQoidStatus
copy the oidvalue into the free space:
cmdStatus = "INSERT oidvalue count\0oidvalue\0"
and return a pointer to here ...............^
That way PQcmdStatus and PQoidStatus don't interfere with each other.
The result of PQoidStatus will still get zapped by the next interaction
with the backend, but at least it's not as transient as it would be in
the errorMessage field.

But, still, converting to an integer return value would be a *lot*
cleaner.

If we were going to do that, I'd be strongly inclined to change
PQcmdTuples to return an int (or more likely a long) as well.
I can't envision any situation where people aren't writing
atoi(PQcmdTuples(conn)) so why not save them the trouble...

regards, tom lane

Bruce Momjian <maillist@candle.pha.pa.us> writes:

Yes, it is very silly that PQoidStatus() returns a char*. I believe it
was only done that way because it was pulling the oid out of the
cmdStatus field. We really should return an int. Perhaps we don't
because we would be hardcoding the oid type on the client side.

Type Oid is already visible to libpq clients: it's returned by PQftype()
as the type code for attributes. I think encouraging clients to store
oids as type Oid rather than some-int-type-chosen-at-random would be a
good thing, not a bad thing.

However, improving the cleanliness of the API might not be worth the
cost of breaking application code, even though this routine is probably
not very widely used. Does anyone else have an opinion pro or con?

Perhaps we could double-use the cmdStatus field, perhaps when they ask
for the oid, we put a null after the oid value, and return a pointer to
the inside of the cmdStatus field. If the call for PQcmdStatus(), we
remove the null and return the string. Strange, I admit.

I had a variant of that idea: what's stored in cmdStatus looks like
INSERT oidvalue count
but there's a fair amount of room left over (cmdStatus is 40 bytes ...
and we could make it bigger if we wanted). We can make PQoidStatus
copy the oidvalue into the free space:
cmdStatus = "INSERT oidvalue count\0oidvalue\0"
and return a pointer to here ...............^
That way PQcmdStatus and PQoidStatus don't interfere with each other.
The result of PQoidStatus will still get zapped by the next interaction
with the backend, but at least it's not as transient as it would be in
the errorMessage field.

But, still, converting to an integer return value would be a *lot*
cleaner.

If we were going to do that, I'd be strongly inclined to change
PQcmdTuples to return an int (or more likely a long) as well.
I can't envision any situation where people aren't writing
atoi(PQcmdTuples(conn)) so why not save them the trouble...

Another idea I had was to a char* field to the result structure.
Initialize it to NULL, then when they want oidStatus, we malloc() some
memory, assign it to our char*, and copy the string into there, and
return that to the user. If we get called later, we can re-use the
memory, and free() it when we free the PQresult.

It was added in 6.2.

If we change it, we will have to require people using those calls to
modify their apps, rather than just relink with the new libpq. However,
old binaries will run fine because they would be linked to the old
libpq.

php3 uses PQoidStatus, so I can imagine great problems if we change the
libpq interface. I am afraid we are stuck with char*.

-- 
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)

Bruce Momjian <maillist@candle.pha.pa.us> writes:

php3 uses PQoidStatus, so I can imagine great problems if we change the
libpq interface. I am afraid we are stuck with char*.

Yeah, I can't really see breaking applications just for a marginal
improvement in cleanliness.

There is a possible compromise however: we could leave PQcmdTuples and
PQoidStatus defined as-is (but do something to get rid of PQoidStatus'
use of a static return area), and add two more functions that have more
reasonable return conventions. The documentation could describe the
older functions as deprecated.

Perhaps the int-returning forms could be named "PQCmdTuples" and
"PQOidStatus" (note difference in capitalization) ... unless someone
has a better idea.

Does anyone think this is worth the trouble, or shall we leave bad
enough alone?

I do intend to get rid of the static return area for PQoidStatus in any
case. I'd also like to fix the problem with PQconninfoOptions not being
treated as a constant (specifically, the "val" fields are being used as
working storage). Is anyone aware of any applications that would be
broken by removing "val" from the PQconninfoOption struct?

regards, tom lane

Bruce Momjian <maillist@candle.pha.pa.us> writes:

php3 uses PQoidStatus, so I can imagine great problems if we change the
libpq interface. I am afraid we are stuck with char*.

Yeah, I can't really see breaking applications just for a marginal
improvement in cleanliness.

There is a possible compromise however: we could leave PQcmdTuples and
PQoidStatus defined as-is (but do something to get rid of PQoidStatus'
use of a static return area), and add two more functions that have more
reasonable return conventions. The documentation could describe the
older functions as deprecated.

Perhaps the int-returning forms could be named "PQCmdTuples" and
"PQOidStatus" (note difference in capitalization) ... unless someone
has a better idea.

Does anyone think this is worth the trouble, or shall we leave bad
enough alone?

Perhaps we can leave the change for a time when we want to change the
libpq interface in a more significant way. Having two functions just
seems like a waste for such a rarely-called fuction.

I do intend to get rid of the static return area for PQoidStatus in any
case. I'd also like to fix the problem with PQconninfoOptions not being
treated as a constant (specifically, the "val" fields are being used as
working storage). Is anyone aware of any applications that would be
broken by removing "val" from the PQconninfoOption struct?

regards, tom lane

-- 
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)