Re: [HACKERS] -d option for pg_isready is broken
On Thu, Dec 12, 2013 at 4:48 AM, Tom Lane wrote: > Robert Haas writes: >> On Wed, Dec 11, 2013 at 2:29 PM, Tom Lane wrote: >>> More generally, if we do go over in 9.4 to the position that PQhost >>> reports the host parameter and nothing but, I'm not sure that introducing >>> a third behavior into the back branches is something that anybody will >>> thank us for. > >> It doesn't seem very plausible to say that we're just going to >> redefine it that way, unless we're planning to bump the soversion. > > Well, we didn't bump the soversion (nor touch the documentation) > in commit f6a756e4, which is basically what I'm suggesting we ought > to revert. It was nothing but a quick hack at the time, and hindsight > is saying it was a bad idea. Admittedly, it was long enough ago that > there might be some grandfather status attached to the current behavior; > but that argument can't be made for changing its behavior still further. > >> But maybe we should decide what we *are* going to do in master first, >> before deciding what to back-patch. > > Right. I'm thinking to implement PQhostaddr() libpq function which returns the host address of the connection. Also I'd like to fix the following two bugs of PQhost(), which I reported upthread. > (1) PQhost() can return Unix-domain socket directory path even in the > platform that > doesn't support Unix-domain socket. > > (2) In the platform that doesn't support Unix-domain socket, when > neither host nor hostaddr > are specified, the default host 'localhost' is used to connect to > the server and > PQhost() must return that, but it doesn't. Then, we can change \conninfo so that it calls both PQhostaddr() and PQhost(). If PQhostaddr() returns non-NULL, \conninfo should display the IP address. Otherwise, \conninfo should display the return value of PQhost(). Regards, -- Fujii Masao -- 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] -d option for pg_isready is broken
Robert Haas writes: > On Wed, Dec 11, 2013 at 2:29 PM, Tom Lane wrote: >> More generally, if we do go over in 9.4 to the position that PQhost >> reports the host parameter and nothing but, I'm not sure that introducing >> a third behavior into the back branches is something that anybody will >> thank us for. > It doesn't seem very plausible to say that we're just going to > redefine it that way, unless we're planning to bump the soversion. Well, we didn't bump the soversion (nor touch the documentation) in commit f6a756e4, which is basically what I'm suggesting we ought to revert. It was nothing but a quick hack at the time, and hindsight is saying it was a bad idea. Admittedly, it was long enough ago that there might be some grandfather status attached to the current behavior; but that argument can't be made for changing its behavior still further. > But maybe we should decide what we *are* going to do in master first, > before deciding what to back-patch. Right. 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] -d option for pg_isready is broken
On Wed, Dec 11, 2013 at 2:29 PM, Tom Lane wrote: > Robert Haas writes: >> On Wed, Dec 11, 2013 at 2:10 PM, Tom Lane wrote: >>> Robert Haas writes: Well, returning /tmp on Windows is just stupid. I don't see why we should feel bad about changing that. A bug is a bug. > >>> What I was suggesting was we should take out the pgunixsocket fallback, >>> not make it even more complicated. That probably implies that we need >>> still another accessor function to get the socket path. > >> Well, I guess. I have a hard time seeing whatever rejiggering we want >> to do in master as a reason not to back-patch that fix, though. > > I guess as long as the pgunixsocket thing is in there, it makes sense > to substitute DefaultHost for it on Windows, but are we sure that's > something to back-patch? Well, it seems like a clear case of returning a ridiculous value, but I'm willing to be talked out of it if someone can explain how it would break things. I guess it's possible someone could have code out that that tests for the exact value /tmp and does something based on that, but that seems a stretch - and if they did have such code, it would probably just handle it by substituting localhost anyway. > Right now, as I was saying, PQhost is in some gray area where it's not too > clear what its charter is. It's not "what was the host parameter", for > sure, but we haven't tried to make it an accurate description of the > connection either. It's a bit less accurate on Windows than elsewhere, > but do we want to risk breaking anything to only partially resolve that? I guess it depends on how risky we think it is. > More generally, if we do go over in 9.4 to the position that PQhost > reports the host parameter and nothing but, I'm not sure that introducing > a third behavior into the back branches is something that anybody will > thank us for. It doesn't seem very plausible to say that we're just going to redefine it that way, unless we're planning to bump the soversion. But maybe we should decide what we *are* going to do in master first, before deciding what to back-patch. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] -d option for pg_isready is broken
Robert Haas writes: > On Wed, Dec 11, 2013 at 2:10 PM, Tom Lane wrote: >> Robert Haas writes: >>> Well, returning /tmp on Windows is just stupid. I don't see why we >>> should feel bad about changing that. A bug is a bug. >> What I was suggesting was we should take out the pgunixsocket fallback, >> not make it even more complicated. That probably implies that we need >> still another accessor function to get the socket path. > Well, I guess. I have a hard time seeing whatever rejiggering we want > to do in master as a reason not to back-patch that fix, though. I guess as long as the pgunixsocket thing is in there, it makes sense to substitute DefaultHost for it on Windows, but are we sure that's something to back-patch? Right now, as I was saying, PQhost is in some gray area where it's not too clear what its charter is. It's not "what was the host parameter", for sure, but we haven't tried to make it an accurate description of the connection either. It's a bit less accurate on Windows than elsewhere, but do we want to risk breaking anything to only partially resolve that? More generally, if we do go over in 9.4 to the position that PQhost reports the host parameter and nothing but, I'm not sure that introducing a third behavior into the back branches is something that anybody will thank us for. 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] -d option for pg_isready is broken
On Wed, Dec 11, 2013 at 2:10 PM, Tom Lane wrote: > Robert Haas writes: >> On Wed, Dec 11, 2013 at 11:24 AM, Tom Lane wrote: >>> In general, I think the definition of these query functions ought to >>> be "what was the value of this parameter when the connection was made". >>> As such, I'm not even sure that the pgunixsocket behavior that's in >>> PQhost now is a good idea, much less that we should extend that hack >>> to cover DefaultHost. > >> Well, returning /tmp on Windows is just stupid. I don't see why we >> should feel bad about changing that. A bug is a bug. > > What I was suggesting was we should take out the pgunixsocket fallback, > not make it even more complicated. That probably implies that we need > still another accessor function to get the socket path. Well, I guess. I have a hard time seeing whatever rejiggering we want to do in master as a reason not to back-patch that fix, though. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] -d option for pg_isready is broken
Robert Haas writes: > On Wed, Dec 11, 2013 at 11:24 AM, Tom Lane wrote: >> In general, I think the definition of these query functions ought to >> be "what was the value of this parameter when the connection was made". >> As such, I'm not even sure that the pgunixsocket behavior that's in >> PQhost now is a good idea, much less that we should extend that hack >> to cover DefaultHost. > Well, returning /tmp on Windows is just stupid. I don't see why we > should feel bad about changing that. A bug is a bug. What I was suggesting was we should take out the pgunixsocket fallback, not make it even more complicated. That probably implies that we need still another accessor function to get the socket path. 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] -d option for pg_isready is broken
On Wed, Dec 11, 2013 at 11:24 AM, Tom Lane wrote: > Robert Haas writes: >> On Wed, Dec 11, 2013 at 9:35 AM, Andres Freund >> wrote: >>> One use case is accessing a particular host when using DNS round robin >>> to standbys in combination with SSL. > >> Ah, interesting point. And it's not inconceivable that some >> application out there could be using PQhost() to retrieve the host >> from an existing connection object and reusing that value for a new >> connection, in which case redefining it to sometimes return hostaddr >> would break things. So I think we shouldn't do that. > > I think the only reasonable way to fix this is to improve the logic > in psql, not turn PQhost() into a mess with no understandable definition. > If that means we need to add a separate PQhostaddr() query function, > so be it. We won't be able to fix the complained-of bug in back branches, > but I'd rather live with that (it's basically just cosmetic anyway) > than risk introducing perhaps-not-so-cosmetic bugs into other existing > applications. I can't argue with that. > In general, I think the definition of these query functions ought to > be "what was the value of this parameter when the connection was made". > As such, I'm not even sure that the pgunixsocket behavior that's in > PQhost now is a good idea, much less that we should extend that hack > to cover DefaultHost. Well, returning /tmp on Windows is just stupid. I don't see why we should feel bad about changing that. A bug is a bug. > There is room also for a function defined as "give me a textual > description of what I'm connected to", which is not meant to reflect any > single connection parameter but rather the total behavior. Right now > I think PQhost is on the borderline of doing this instead of just > reporting the "host" parameter, but I think rather than pushing it > across that border we'd be better off to invent a function explicitly > charged with doing that. That would give us room to do something > actually meaningful with host+hostaddr cases, for instance. I think > really what ought to be printed in such cases is something like > "host-name (address IP-address)"; leaving out the former would be > unhelpful but leaving out the latter is outright misleading. > > On the other hand, I'm not sure how much of a translatability problem > it'd be to wedge such a description into a larger message. Might be > better to just put the logic into psql. libpq needs to expose enough functionality to make this simple for psql, but psql should be the final arbiter of the output format. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] -d option for pg_isready is broken
Robert Haas writes: > On Wed, Dec 11, 2013 at 9:35 AM, Andres Freund wrote: >> One use case is accessing a particular host when using DNS round robin >> to standbys in combination with SSL. > Ah, interesting point. And it's not inconceivable that some > application out there could be using PQhost() to retrieve the host > from an existing connection object and reusing that value for a new > connection, in which case redefining it to sometimes return hostaddr > would break things. So I think we shouldn't do that. I think the only reasonable way to fix this is to improve the logic in psql, not turn PQhost() into a mess with no understandable definition. If that means we need to add a separate PQhostaddr() query function, so be it. We won't be able to fix the complained-of bug in back branches, but I'd rather live with that (it's basically just cosmetic anyway) than risk introducing perhaps-not-so-cosmetic bugs into other existing applications. In general, I think the definition of these query functions ought to be "what was the value of this parameter when the connection was made". As such, I'm not even sure that the pgunixsocket behavior that's in PQhost now is a good idea, much less that we should extend that hack to cover DefaultHost. There is room also for a function defined as "give me a textual description of what I'm connected to", which is not meant to reflect any single connection parameter but rather the total behavior. Right now I think PQhost is on the borderline of doing this instead of just reporting the "host" parameter, but I think rather than pushing it across that border we'd be better off to invent a function explicitly charged with doing that. That would give us room to do something actually meaningful with host+hostaddr cases, for instance. I think really what ought to be printed in such cases is something like "host-name (address IP-address)"; leaving out the former would be unhelpful but leaving out the latter is outright misleading. On the other hand, I'm not sure how much of a translatability problem it'd be to wedge such a description into a larger message. Might be better to just put the logic into psql. 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] -d option for pg_isready is broken
On Wed, Dec 11, 2013 at 9:35 AM, Andres Freund wrote: > On 2013-12-11 08:56:43 -0500, Robert Haas wrote: >> > $ psql -d "hostaddr=127.0.0.1" >> > =# \conninfo >> > You are connected to database "postgres" as user "postgres" via socket >> > in "/tmp" at port "5432". >> >> Yeah, that's true. But the whole point of having both host and >> hostaddr seems to be that you can lie about where you're connecting. >> If you set host=some.pretty.domain.name hostaddr=1.2.3.4, the point is >> to say that you're connecting to the first while, under the covers, >> actually connecting to the second. Now, I am unclear what value this >> has, but someone at some point evidently thought it was a good idea, >> so we need to be careful about changing it. > > One use case is accessing a particular host when using DNS round robin > to standbys in combination with SSL. Ah, interesting point. And it's not inconceivable that some application out there could be using PQhost() to retrieve the host from an existing connection object and reusing that value for a new connection, in which case redefining it to sometimes return hostaddr would break things. So I think we shouldn't do that. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] -d option for pg_isready is broken
On 2013-12-11 08:56:43 -0500, Robert Haas wrote: > > $ psql -d "hostaddr=127.0.0.1" > > =# \conninfo > > You are connected to database "postgres" as user "postgres" via socket > > in "/tmp" at port "5432". > > Yeah, that's true. But the whole point of having both host and > hostaddr seems to be that you can lie about where you're connecting. > If you set host=some.pretty.domain.name hostaddr=1.2.3.4, the point is > to say that you're connecting to the first while, under the covers, > actually connecting to the second. Now, I am unclear what value this > has, but someone at some point evidently thought it was a good idea, > so we need to be careful about changing it. One use case is accessing a particular host when using DNS round robin to standbys in combination with SSL. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- 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] -d option for pg_isready is broken
On Wed, Dec 11, 2013 at 8:45 AM, Fujii Masao wrote: > On Wed, Dec 11, 2013 at 10:26 PM, Robert Haas wrote: >> On Tue, Dec 10, 2013 at 11:40 PM, Fujii Masao wrote: >>> While I was investigaing PQhost() for that approach, I found several >>> problems of PQhost(). >>> >>> (1) PQhost() can return Unix-domain socket directory path even in the >>> platform that >>> doesn't support Unix-domain socket. >>> >>> (2) In the platform that doesn't support Unix-domain socket, when >>> neither host nor hostaddr >>> are specified, the default host 'localhost' is used to connect to >>> the server and >>> PQhost() must return that, but it doesn't. >> >> I think changing PQhost() so that it returns DefaultHost rather than >> conn->pgunixsocket when we don't HAVE_UNIX_SOCKETS is a back-patchable >> bug fix, and I'd say go for it. > > Agreed. > >>> (3) PQhost() cannot return the hostaddr. >> >> However, I'm much less sure whether this is something that we want to >> do at all. It seems like this might be a definition of what the >> function does, and I'm not sure whether the new definition is what >> everyone will want. On the other hand, I'm also not sure that it >> isn't. > > If we don't change PQhost() in that way, we cannot fix the following problem > of wrong output of \conninfo, for example. > > $ psql -d "hostaddr=127.0.0.1" > =# \conninfo > You are connected to database "postgres" as user "postgres" via socket > in "/tmp" at port "5432". Yeah, that's true. But the whole point of having both host and hostaddr seems to be that you can lie about where you're connecting. If you set host=some.pretty.domain.name hostaddr=1.2.3.4, the point is to say that you're connecting to the first while, under the covers, actually connecting to the second. Now, I am unclear what value this has, but someone at some point evidently thought it was a good idea, so we need to be careful about changing it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] -d option for pg_isready is broken
On Wed, Dec 11, 2013 at 10:26 PM, Robert Haas wrote: > On Tue, Dec 10, 2013 at 11:40 PM, Fujii Masao wrote: >> While I was investigaing PQhost() for that approach, I found several >> problems of PQhost(). >> >> (1) PQhost() can return Unix-domain socket directory path even in the >> platform that >> doesn't support Unix-domain socket. >> >> (2) In the platform that doesn't support Unix-domain socket, when >> neither host nor hostaddr >> are specified, the default host 'localhost' is used to connect to >> the server and >> PQhost() must return that, but it doesn't. > > I think changing PQhost() so that it returns DefaultHost rather than > conn->pgunixsocket when we don't HAVE_UNIX_SOCKETS is a back-patchable > bug fix, and I'd say go for it. Agreed. >> (3) PQhost() cannot return the hostaddr. > > However, I'm much less sure whether this is something that we want to > do at all. It seems like this might be a definition of what the > function does, and I'm not sure whether the new definition is what > everyone will want. On the other hand, I'm also not sure that it > isn't. If we don't change PQhost() in that way, we cannot fix the following problem of wrong output of \conninfo, for example. $ psql -d "hostaddr=127.0.0.1" =# \conninfo You are connected to database "postgres" as user "postgres" via socket in "/tmp" at port "5432". Regards. -- Fujii Masao -- 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] -d option for pg_isready is broken
On Tue, Dec 10, 2013 at 11:40 PM, Fujii Masao wrote: > While I was investigaing PQhost() for that approach, I found several > problems of PQhost(). > > (1) PQhost() can return Unix-domain socket directory path even in the > platform that > doesn't support Unix-domain socket. > > (2) In the platform that doesn't support Unix-domain socket, when > neither host nor hostaddr > are specified, the default host 'localhost' is used to connect to > the server and > PQhost() must return that, but it doesn't. I think changing PQhost() so that it returns DefaultHost rather than conn->pgunixsocket when we don't HAVE_UNIX_SOCKETS is a back-patchable bug fix, and I'd say go for it. > (3) PQhost() cannot return the hostaddr. However, I'm much less sure whether this is something that we want to do at all. It seems like this might be a definition of what the function does, and I'm not sure whether the new definition is what everyone will want. On the other hand, I'm also not sure that it isn't. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] -d option for pg_isready is broken
On Thu, Nov 21, 2013 at 9:58 PM, Fujii Masao wrote: > On Thu, Nov 21, 2013 at 6:41 AM, Robert Haas wrote: >> On Wed, Nov 20, 2013 at 4:55 AM, Fujii Masao wrote: Not that I can see, but it's not very future-proof. If libpq changes its idea of what will provoke database-name expansion, this will again be broken. >>> >>> Yeah, I agree that we should make the logic of pg_isready more future-proof >>> in 9.4dev. One idea is to expose internal_ping() as a libpq function. Then, >>> instead of just calling PQpingParams(), we can do PQconnectStartParams(), >>> libpq-function-version-of-internal_ping(), PQhost(), PQport(), and >>> PQfinish(). >>> That is, we get to know the host and port information by calling >>> PQhost() and PQport(), >>> after trying to connect to the server. While I was investigaing PQhost() for that approach, I found several problems of PQhost(). (1) PQhost() can return Unix-domain socket directory path even in the platform that doesn't support Unix-domain socket. (2) In the platform that doesn't support Unix-domain socket, when neither host nor hostaddr are specified, the default host 'localhost' is used to connect to the server and PQhost() must return that, but it doesn't. (3) PQhost() cannot return the hostaddr. As the result of these problems, you can see the incorrect result of \conninfo, for example. $ psql -d "hostaddr=127.0.0.1" =# \conninfo You are connected to database "postgres" as user "postgres" via socket in "/tmp" at port "5432". Obviously "/tmp" should be "127.0.0.1". The attached patch fixes these problems of PQhost(). But I'm concerned about that this change may break the existing application using PQhost(). That is, some existing application might not assume that PQhost() returns hostaddr. If my concern is true, maybe we need to implement something like PQhostaddr(). It's too late to add such new libpq function into the current stable release, though... Thought? If it's okay to change the behavior of PQhost() as I explained, I will commit the patch in all supported versions. Regards, -- Fujii Masao *** a/src/interfaces/libpq/fe-connect.c --- b/src/interfaces/libpq/fe-connect.c *** *** 5188,5194 PQhost(const PGconn *conn) { if (!conn) return NULL; ! return conn->pghost ? conn->pghost : conn->pgunixsocket; } char * --- 5188,5205 { if (!conn) return NULL; ! if (conn->pghostaddr != NULL && conn->pghostaddr[0] != '\0') ! return conn->pghostaddr; ! else if (conn->pghost != NULL && conn->pghost[0] != '\0') ! return conn->pghost; ! else ! { ! #ifdef HAVE_UNIX_SOCKETS ! return conn->pgunixsocket; ! #else ! return DefaultHost; ! #endif ! } } char * -- 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] -d option for pg_isready is broken
On Thu, Nov 21, 2013 at 6:41 AM, Robert Haas wrote: > On Wed, Nov 20, 2013 at 4:55 AM, Fujii Masao wrote: >>> Not that I can see, but it's not very future-proof. If libpq changes >>> its idea of what will provoke database-name expansion, this will again >>> be broken. >> >> Yeah, I agree that we should make the logic of pg_isready more future-proof >> in 9.4dev. One idea is to expose internal_ping() as a libpq function. Then, >> instead of just calling PQpingParams(), we can do PQconnectStartParams(), >> libpq-function-version-of-internal_ping(), PQhost(), PQport(), and >> PQfinish(). >> That is, we get to know the host and port information by calling >> PQhost() and PQport(), >> after trying to connect to the server. > > Hmm, that sounds like a possibly promising approach. > >> But we cannot use this idea in 9.3 because it's too late to add new >> libpq function there. >> Also I don't think that the minor version release would change that >> libpq's logic in 9.3. >> So, barring any objections, I will commit the latest version of the >> patch in 9.3. > > I think you should commit it to both master and REL9_3_STABLE. Committed. > Then, > you can make further changes to master in a subsequent commit. Yeah, I will implement that patch. Regards, -- Fujii Masao -- 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] -d option for pg_isready is broken
On Wed, Nov 20, 2013 at 4:55 AM, Fujii Masao wrote: >> Not that I can see, but it's not very future-proof. If libpq changes >> its idea of what will provoke database-name expansion, this will again >> be broken. > > Yeah, I agree that we should make the logic of pg_isready more future-proof > in 9.4dev. One idea is to expose internal_ping() as a libpq function. Then, > instead of just calling PQpingParams(), we can do PQconnectStartParams(), > libpq-function-version-of-internal_ping(), PQhost(), PQport(), and PQfinish(). > That is, we get to know the host and port information by calling > PQhost() and PQport(), > after trying to connect to the server. Hmm, that sounds like a possibly promising approach. > But we cannot use this idea in 9.3 because it's too late to add new > libpq function there. > Also I don't think that the minor version release would change that > libpq's logic in 9.3. > So, barring any objections, I will commit the latest version of the > patch in 9.3. I think you should commit it to both master and REL9_3_STABLE. Then, you can make further changes to master in a subsequent commit. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] -d option for pg_isready is broken
On 11/20/2013 01:55 AM, Fujii Masao wrote: > Yeah, I agree that we should make the logic of pg_isready more future-proof > in 9.4dev. One idea is to expose internal_ping() as a libpq function. Then, > instead of just calling PQpingParams(), we can do PQconnectStartParams(), > libpq-function-version-of-internal_ping(), PQhost(), PQport(), and PQfinish(). > That is, we get to know the host and port information by calling > PQhost() and PQport(), > after trying to connect to the server. +1 This would allow writers of client drivers to implement their own pg_ping() functions instead of needing to go through the shell (as I currently do with pg_isready). -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- 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] -d option for pg_isready is broken
On Wed, Nov 20, 2013 at 3:12 AM, Robert Haas wrote: > On Tue, Nov 19, 2013 at 1:10 PM, Fujii Masao wrote: >> On Tue, Nov 19, 2013 at 11:51 PM, Robert Haas wrote: >>> On Fri, Nov 15, 2013 at 9:01 PM, Fabrízio de Royes Mello >>> wrote: On Wed, Nov 13, 2013 at 9:37 PM, Josh Berkus wrote: > handyrep@john:~/handyrep$ pg_isready --version > pg_isready (PostgreSQL) 9.3.1 > > handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U postgres -d > postgres -q > pg_isready: missing "=" after "postgres" in connection info string > > handyrep@john:~/handyrep$ pg_isready --host=john --port=5432 > --user=postgres --dbname=postgres > pg_isready: missing "=" after "postgres" in connection info string > > handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U postgres > john:5432 - accepting connections > > so apparently the -d option: > > a) doesn't work, and > b) doesn't do anything > > I suggest simply removing it from the utility. > > I'll note that the -U option doesn't appear to do anything relevant > either, but at least it doesn't error unnecessarily: > > handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U no_such_user > john:5432 - accepting connections > The attached patch fix it. >>> >>> Well, there still seem to be some problems. >>> >>> [rhaas pgsql]$ src/bin/scripts/pg_isready -d host=sgdasasg >>> /tmp:5432 - no attempt >>> >>> I added some debugging instrumentation and it looks like there's a >>> problem with this code: >>> >>> if (strcmp(def->keyword, "hostaddr") == 0 || >>> strcmp(def->keyword, "host") == 0) >>> >>> The problem is that we end up executing the code contained in that >>> block twice, once for host and once for hostaddr. Thus: >>> >>> [rhaas pgsql]$ src/bin/scripts/pg_isready -d host=sgdasasg >>> def->keyword=host pghost_str=sgdasasg >>> def->keyword=hostaddr pghost_str=/tmp >>> def->keyword=port pgport_str=5432 >>> /tmp:5432 - no attempt >>> >>> Oops. >>> >>> Nevertheless, that's kind of a separate problem, so we could address >>> in a separate commit. But even ignoring that, this still isn't right: >>> according to the fine documentation, -d will be expanded not only if >>> it contains an =, but also if it starts with a valid URI prefix. This >>> would break that documented behavior. >>> >>> http://www.postgresql.org/docs/9.3/static/app-pg-isready.html >> >> Attached is the updated version of the patch. Is there any other bug? > > Not that I can see, but it's not very future-proof. If libpq changes > its idea of what will provoke database-name expansion, this will again > be broken. Yeah, I agree that we should make the logic of pg_isready more future-proof in 9.4dev. One idea is to expose internal_ping() as a libpq function. Then, instead of just calling PQpingParams(), we can do PQconnectStartParams(), libpq-function-version-of-internal_ping(), PQhost(), PQport(), and PQfinish(). That is, we get to know the host and port information by calling PQhost() and PQport(), after trying to connect to the server. But we cannot use this idea in 9.3 because it's too late to add new libpq function there. Also I don't think that the minor version release would change that libpq's logic in 9.3. So, barring any objections, I will commit the latest version of the patch in 9.3. Regards, -- Fujii Masao -- 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] -d option for pg_isready is broken
On Tue, Nov 19, 2013 at 1:22 PM, Josh Berkus wrote: > On 11/19/2013 10:12 AM, Robert Haas wrote: >> On Tue, Nov 19, 2013 at 1:10 PM, Fujii Masao wrote: >>> On Tue, Nov 19, 2013 at 11:51 PM, Robert Haas wrote: On Fri, Nov 15, 2013 at 9:01 PM, Fabrízio de Royes Mello http://www.postgresql.org/docs/9.3/static/app-pg-isready.html >>> >>> Attached is the updated version of the patch. Is there any other bug? >> >> Not that I can see, but it's not very future-proof. If libpq changes >> its idea of what will provoke database-name expansion, this will again >> be broken. > > Why aren't we using the exact same code as psql? Why does pg_isready > have its own code for this? Because pg_isready wants to print the host and port we actually tried to connect to, which no other utility does. Turns out, there's no clean API for that. We tried to invent something, but the evidence seems to indicate that what we invented bites. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] -d option for pg_isready is broken
On 11/19/2013 10:12 AM, Robert Haas wrote: > On Tue, Nov 19, 2013 at 1:10 PM, Fujii Masao wrote: >> On Tue, Nov 19, 2013 at 11:51 PM, Robert Haas wrote: >>> On Fri, Nov 15, 2013 at 9:01 PM, Fabrízio de Royes Mello >>> http://www.postgresql.org/docs/9.3/static/app-pg-isready.html >> >> Attached is the updated version of the patch. Is there any other bug? > > Not that I can see, but it's not very future-proof. If libpq changes > its idea of what will provoke database-name expansion, this will again > be broken. Why aren't we using the exact same code as psql? Why does pg_isready have its own code for this? -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- 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] -d option for pg_isready is broken
On Tue, Nov 19, 2013 at 1:10 PM, Fujii Masao wrote: > On Tue, Nov 19, 2013 at 11:51 PM, Robert Haas wrote: >> On Fri, Nov 15, 2013 at 9:01 PM, Fabrízio de Royes Mello >> wrote: >>> On Wed, Nov 13, 2013 at 9:37 PM, Josh Berkus wrote: handyrep@john:~/handyrep$ pg_isready --version pg_isready (PostgreSQL) 9.3.1 handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U postgres -d postgres -q pg_isready: missing "=" after "postgres" in connection info string handyrep@john:~/handyrep$ pg_isready --host=john --port=5432 --user=postgres --dbname=postgres pg_isready: missing "=" after "postgres" in connection info string handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U postgres john:5432 - accepting connections so apparently the -d option: a) doesn't work, and b) doesn't do anything I suggest simply removing it from the utility. I'll note that the -U option doesn't appear to do anything relevant either, but at least it doesn't error unnecessarily: handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U no_such_user john:5432 - accepting connections >>> >>> The attached patch fix it. >> >> Well, there still seem to be some problems. >> >> [rhaas pgsql]$ src/bin/scripts/pg_isready -d host=sgdasasg >> /tmp:5432 - no attempt >> >> I added some debugging instrumentation and it looks like there's a >> problem with this code: >> >> if (strcmp(def->keyword, "hostaddr") == 0 || >> strcmp(def->keyword, "host") == 0) >> >> The problem is that we end up executing the code contained in that >> block twice, once for host and once for hostaddr. Thus: >> >> [rhaas pgsql]$ src/bin/scripts/pg_isready -d host=sgdasasg >> def->keyword=host pghost_str=sgdasasg >> def->keyword=hostaddr pghost_str=/tmp >> def->keyword=port pgport_str=5432 >> /tmp:5432 - no attempt >> >> Oops. >> >> Nevertheless, that's kind of a separate problem, so we could address >> in a separate commit. But even ignoring that, this still isn't right: >> according to the fine documentation, -d will be expanded not only if >> it contains an =, but also if it starts with a valid URI prefix. This >> would break that documented behavior. >> >> http://www.postgresql.org/docs/9.3/static/app-pg-isready.html > > Attached is the updated version of the patch. Is there any other bug? Not that I can see, but it's not very future-proof. If libpq changes its idea of what will provoke database-name expansion, this will again be broken. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] -d option for pg_isready is broken
On Tue, Nov 19, 2013 at 11:51 PM, Robert Haas wrote: > On Fri, Nov 15, 2013 at 9:01 PM, Fabrízio de Royes Mello > wrote: >> On Wed, Nov 13, 2013 at 9:37 PM, Josh Berkus wrote: >>> handyrep@john:~/handyrep$ pg_isready --version >>> pg_isready (PostgreSQL) 9.3.1 >>> >>> handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U postgres -d >>> postgres -q >>> pg_isready: missing "=" after "postgres" in connection info string >>> >>> handyrep@john:~/handyrep$ pg_isready --host=john --port=5432 >>> --user=postgres --dbname=postgres >>> pg_isready: missing "=" after "postgres" in connection info string >>> >>> handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U postgres >>> john:5432 - accepting connections >>> >>> so apparently the -d option: >>> >>> a) doesn't work, and >>> b) doesn't do anything >>> >>> I suggest simply removing it from the utility. >>> >>> I'll note that the -U option doesn't appear to do anything relevant >>> either, but at least it doesn't error unnecessarily: >>> >>> handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U no_such_user >>> john:5432 - accepting connections >>> >> >> The attached patch fix it. > > Well, there still seem to be some problems. > > [rhaas pgsql]$ src/bin/scripts/pg_isready -d host=sgdasasg > /tmp:5432 - no attempt > > I added some debugging instrumentation and it looks like there's a > problem with this code: > > if (strcmp(def->keyword, "hostaddr") == 0 || > strcmp(def->keyword, "host") == 0) > > The problem is that we end up executing the code contained in that > block twice, once for host and once for hostaddr. Thus: > > [rhaas pgsql]$ src/bin/scripts/pg_isready -d host=sgdasasg > def->keyword=host pghost_str=sgdasasg > def->keyword=hostaddr pghost_str=/tmp > def->keyword=port pgport_str=5432 > /tmp:5432 - no attempt > > Oops. > > Nevertheless, that's kind of a separate problem, so we could address > in a separate commit. But even ignoring that, this still isn't right: > according to the fine documentation, -d will be expanded not only if > it contains an =, but also if it starts with a valid URI prefix. This > would break that documented behavior. > > http://www.postgresql.org/docs/9.3/static/app-pg-isready.html Attached is the updated version of the patch. Is there any other bug? Regards, -- Fujii Masao *** a/src/bin/scripts/pg_isready.c --- b/src/bin/scripts/pg_isready.c *** *** 31,36 main(int argc, char **argv) --- 31,37 const char *connect_timeout = DEFAULT_CONNECT_TIMEOUT; const char *pghost_str = NULL; + const char *pghostaddr_str = NULL; const char *pgport_str = NULL; #define PARAMS_ARRAY_SIZE 7 *** *** 130,136 main(int argc, char **argv) /* * Get the host and port so we can display them in our output */ ! if (pgdbname) { opts = PQconninfoParse(pgdbname, &errmsg); if (opts == NULL) --- 131,140 /* * Get the host and port so we can display them in our output */ ! if (pgdbname && ! (strncmp(pgdbname, "postgresql://", 13) == 0 || ! strncmp(pgdbname, "postgres://", 11) == 0 || ! strchr(pgdbname, '=') != NULL)) { opts = PQconninfoParse(pgdbname, &errmsg); if (opts == NULL) *** *** 149,156 main(int argc, char **argv) for (opt = opts, def = defs; def->keyword; def++) { ! if (strcmp(def->keyword, "hostaddr") == 0 || ! strcmp(def->keyword, "host") == 0) { if (opt && opt->val) pghost_str = opt->val; --- 153,159 for (opt = opts, def = defs; def->keyword; def++) { ! if (strcmp(def->keyword, "host") == 0) { if (opt && opt->val) pghost_str = opt->val; *** *** 161,166 main(int argc, char **argv) --- 164,176 else pghost_str = DEFAULT_PGSOCKET_DIR; } + else if (strcmp(def->keyword, "hostaddr") == 0) + { + if (opt && opt->val) + pghostaddr_str = opt->val; + else if (def->val) + pghostaddr_str = def->val; + } else if (strcmp(def->keyword, "port") == 0) { if (opt && opt->val) *** *** 179,185 main(int argc, char **argv) if (!quiet) { ! printf("%s:%s - ", pghost_str, pgport_str); switch (rv) { --- 189,197 if (!quiet) { ! printf("%s:%s - ", ! pghostaddr_str != NULL ? pghostaddr_str : pghost_str, ! pgport_str); switch (rv) { -- 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] -d option for pg_isready is broken
On Fri, Nov 15, 2013 at 9:01 PM, Fabrízio de Royes Mello wrote: > On Wed, Nov 13, 2013 at 9:37 PM, Josh Berkus wrote: >> handyrep@john:~/handyrep$ pg_isready --version >> pg_isready (PostgreSQL) 9.3.1 >> >> handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U postgres -d >> postgres -q >> pg_isready: missing "=" after "postgres" in connection info string >> >> handyrep@john:~/handyrep$ pg_isready --host=john --port=5432 >> --user=postgres --dbname=postgres >> pg_isready: missing "=" after "postgres" in connection info string >> >> handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U postgres >> john:5432 - accepting connections >> >> so apparently the -d option: >> >> a) doesn't work, and >> b) doesn't do anything >> >> I suggest simply removing it from the utility. >> >> I'll note that the -U option doesn't appear to do anything relevant >> either, but at least it doesn't error unnecessarily: >> >> handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U no_such_user >> john:5432 - accepting connections >> > > The attached patch fix it. Well, there still seem to be some problems. [rhaas pgsql]$ src/bin/scripts/pg_isready -d host=sgdasasg /tmp:5432 - no attempt I added some debugging instrumentation and it looks like there's a problem with this code: if (strcmp(def->keyword, "hostaddr") == 0 || strcmp(def->keyword, "host") == 0) The problem is that we end up executing the code contained in that block twice, once for host and once for hostaddr. Thus: [rhaas pgsql]$ src/bin/scripts/pg_isready -d host=sgdasasg def->keyword=host pghost_str=sgdasasg def->keyword=hostaddr pghost_str=/tmp def->keyword=port pgport_str=5432 /tmp:5432 - no attempt Oops. Nevertheless, that's kind of a separate problem, so we could address in a separate commit. But even ignoring that, this still isn't right: according to the fine documentation, -d will be expanded not only if it contains an =, but also if it starts with a valid URI prefix. This would break that documented behavior. http://www.postgresql.org/docs/9.3/static/app-pg-isready.html If you submit an updated patch, please fix the comment just above the code you're changing to more accurately reflect what this does. The number of bugs in pg_isready certainly seems out of proportion to the surface area of the problem it solves. Whee! -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] -d option for pg_isready is broken
On Wed, Nov 13, 2013 at 9:37 PM, Josh Berkus wrote: > > handyrep@john:~/handyrep$ pg_isready --version > pg_isready (PostgreSQL) 9.3.1 > > handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U postgres -d > postgres -q > pg_isready: missing "=" after "postgres" in connection info string > > handyrep@john:~/handyrep$ pg_isready --host=john --port=5432 > --user=postgres --dbname=postgres > pg_isready: missing "=" after "postgres" in connection info string > > handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U postgres > john:5432 - accepting connections > > so apparently the -d option: > > a) doesn't work, and > b) doesn't do anything > > I suggest simply removing it from the utility. > > I'll note that the -U option doesn't appear to do anything relevant > either, but at least it doesn't error unnecessarily: > > handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U no_such_user > john:5432 - accepting connections > > The attached patch fix it. Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL >> Timbira: http://www.timbira.com.br >> Blog sobre TI: http://fabriziomello.blogspot.com >> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello >> Twitter: http://twitter.com/fabriziomello diff --git a/src/bin/scripts/pg_isready.c b/src/bin/scripts/pg_isready.c index d27ccea..7568df5 100644 --- a/src/bin/scripts/pg_isready.c +++ b/src/bin/scripts/pg_isready.c @@ -130,7 +130,7 @@ main(int argc, char **argv) /* * Get the host and port so we can display them in our output */ - if (pgdbname) + if (pgdbname && strchr(pgdbname, '=') != NULL) { opts = PQconninfoParse(pgdbname, &errmsg); if (opts == NULL) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] -d option for pg_isready is broken
handyrep@john:~/handyrep$ pg_isready --version pg_isready (PostgreSQL) 9.3.1 handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U postgres -d postgres -q pg_isready: missing "=" after "postgres" in connection info string handyrep@john:~/handyrep$ pg_isready --host=john --port=5432 --user=postgres --dbname=postgres pg_isready: missing "=" after "postgres" in connection info string handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U postgres john:5432 - accepting connections so apparently the -d option: a) doesn't work, and b) doesn't do anything I suggest simply removing it from the utility. I'll note that the -U option doesn't appear to do anything relevant either, but at least it doesn't error unnecessarily: handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U no_such_user john:5432 - accepting connections -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers