Re: [HACKERS] Redefine default result from PQhost()?
On Thu, Nov 26, 2015 at 10:48:50AM -0500, Tom Lane wrote: > Magnus Hagander writes: > > On Wed, Nov 25, 2015 at 11:59 PM, Tom Lane 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Redefine default result from PQhost()?
Magnus Hagander 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Redefine default result from PQhost()?
On Thu, Nov 26, 2015 at 4:48 PM, Tom Lane wrote: > Magnus Hagander writes: > > On Wed, Nov 25, 2015 at 11:59 PM, Tom Lane 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 Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Redefine default result from PQhost()?
Magnus Hagander writes: > On Wed, Nov 25, 2015 at 11:59 PM, Tom Lane 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Redefine default result from PQhost()?
On Wed, Nov 25, 2015 at 11:59 PM, Tom Lane 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
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
