Thread: Redefine default result from PQhost()?
Currently, libpq's PQhost() function will return NULL if the connection is
using the default Unix-socket connection address. This seems like a
pretty dubious definition to me. psql works around it in several places
with
host = PQhost(pset.db); if (host == NULL) host = DEFAULT_PGSOCKET_DIR;
That seems rather duplicative, and it also means that both libpq and psql
have the default socket directory hard-wired into them, which does not
seem like a good thing. It's conceivable that psql could get used with
a libpq built with a different default, in which case these places would
be outright lying about which socket is being used.
I think we should do this:
char *PQhost(const PGconn *conn){ if (!conn) return NULL; if (conn->pghost != NULL && conn->pghost[0] !=
'\0') return conn->pghost; else {#ifdef HAVE_UNIX_SOCKETS
- return conn->pgunixsocket;
+ if (conn->pgunixsocket != NULL && conn->pgunixsocket[0] != '\0')
+ return conn->pgunixsocket;
+ else
+ return DEFAULT_PGSOCKET_DIR;#else return DefaultHost;#endif }}
As a definitional change, this would be for HEAD only.
Comments?
regards, tom lane
On Wed, Nov 25, 2015 at 11:59 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Currently, libpq's PQhost() function will return NULL if the connection is
using the default Unix-socket connection address. This seems like a
pretty dubious definition to me. psql works around it in several places
with
host = PQhost(pset.db);
if (host == NULL)
host = DEFAULT_PGSOCKET_DIR;
That seems rather duplicative, and it also means that both libpq and psql
have the default socket directory hard-wired into them, which does not
seem like a good thing. It's conceivable that psql could get used with
a libpq built with a different default, in which case these places would
be outright lying about which socket is being used.
I think we should do this:
char *
PQhost(const PGconn *conn)
{
if (!conn)
return NULL;
if (conn->pghost != NULL && conn->pghost[0] != '\0')
return conn->pghost;
else
{
#ifdef HAVE_UNIX_SOCKETS
- return conn->pgunixsocket;
+ if (conn->pgunixsocket != NULL && conn->pgunixsocket[0] != '\0')
+ return conn->pgunixsocket;
+ else
+ return DEFAULT_PGSOCKET_DIR;
#else
return DefaultHost;
#endif
}
}
As a definitional change, this would be for HEAD only.
Comments?
I agree with this change in genera. But I wonder if there's a risk here that we break some applications isnt' it? It's clearly a backwards incompatible change, so wouldn't it require a bump of libpq version? And I'm not sure it's worth that on it's own...
Magnus Hagander <magnus@hagander.net> writes:
> On Wed, Nov 25, 2015 at 11:59 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think we should [ return DEFAULT_PGSOCKET_DIR not NULL ]
> I agree with this change in genera. But I wonder if there's a risk here
> that we break some applications isnt' it? It's clearly a backwards
> incompatible change, so wouldn't it require a bump of libpq version?
I don't see why it would need a libpq version bump, as it's not an ABI
breakage. As for breaking application logic, it's true that any app code
that is explicitly checking for NULL would become dead code, but that
seems pretty harmless. It's already the case that apps should be prepared
to get back an explicit socket directory path spec; that would merely
become a more common case than before.
Also, given the precedent of yesterday's psql fix, we should not discount
the idea that we will be *removing* not adding corner-case bugs in some
applications. Failure to check for NULL is probably even more common in
other apps than in psql.
On the other side of the coin, it's worth noting that there is no
well-defined way for libpq-using apps to discover the value of
DEFAULT_PGSOCKET_DIR. (psql is getting it by #include'ing an internal
header file; there is no way to get it from PQconndefaults.) So whatever
an app might have been doing if it did check for NULL is arguably
inherently buggy, and bypassing such code will be a good thing.
regards, tom lane
On Thu, Nov 26, 2015 at 4:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
> On Wed, Nov 25, 2015 at 11:59 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think we should [ return DEFAULT_PGSOCKET_DIR not NULL ]
> I agree with this change in genera. But I wonder if there's a risk here
> that we break some applications isnt' it? It's clearly a backwards
> incompatible change, so wouldn't it require a bump of libpq version?
I don't see why it would need a libpq version bump, as it's not an ABI
breakage. As for breaking application logic, it's true that any app code
that is explicitly checking for NULL would become dead code, but that
seems pretty harmless. It's already the case that apps should be prepared
to get back an explicit socket directory path spec; that would merely
become a more common case than before.
Hmm. Good point. I didn't realize they already had to be ready to get a non-default path back.
Of course, the documentation just says it'll return a hostname. It should probably mention that it can return the unix socket path as well - but that's something that's missing already.
--
Magnus Hagander <magnus@hagander.net> writes:
> Hmm. Good point. I didn't realize they already had to be ready to get a
> non-default path back.
Yeah, if it weren't for that, this would definitely be a hazardous change.
But AFAICS, an app that has a problem with this proposal was broken
already, it just didn't know it.
> Of course, the documentation just says it'll return a hostname. It should
> probably mention that it can return the unix socket path as well - but
> that's something that's missing already.
Agreed, will fix that.
regards, tom lane
On Thu, Nov 26, 2015 at 10:48:50AM -0500, Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: > > On Wed, Nov 25, 2015 at 11:59 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> I think we should [ return DEFAULT_PGSOCKET_DIR not NULL ] > > > I agree with this change in genera. But I wonder if there's a risk here > > that we break some applications isnt' it? It's clearly a backwards > > incompatible change, so wouldn't it require a bump of libpq version? > > I don't see why it would need a libpq version bump, as it's not an ABI > breakage. As for breaking application logic, it's true that any app code > that is explicitly checking for NULL would become dead code, but that > seems pretty harmless. It's already the case that apps should be prepared > to get back an explicit socket directory path spec; that would merely > become a more common case than before. > > Also, given the precedent of yesterday's psql fix, we should not discount > the idea that we will be *removing* not adding corner-case bugs in some > applications. Failure to check for NULL is probably even more common in > other apps than in psql. > > On the other side of the coin, it's worth noting that there is no > well-defined way for libpq-using apps to discover the value of > DEFAULT_PGSOCKET_DIR. (psql is getting it by #include'ing an internal > header file; there is no way to get it from PQconndefaults.) So whatever > an app might have been doing if it did check for NULL is arguably > inherently buggy, and bypassing such code will be a good thing. I agree with each of those points. I see little value in distinguishing between, in a default build, explicit PGHOST=/tmp and unspecified PGHOST. The lack of a supported way to discover the default compounds that conclusion.