Thread: pgsql: psql: fix \connect with URIs and conninfo strings
psql: fix \connect with URIs and conninfo strings psql was already accepting conninfo strings as the first parameter in \connect, but the way it worked wasn't sane; some of the other parameters would get the previous connection's values, causing it to connect to a completely unexpected server or, more likely, not finding any server at all because of completely wrong combinations of parameters. Fix by explicitely checking for a conninfo-looking parameter in the dbname position; if one is found, use its complete specification rather than mix with the other arguments. Also, change tab-completion to not try to complete conninfo/URI-looking "dbnames" and document that conninfos are accepted as first argument. There was a weak consensus to backpatch this, because while the behavior of using the dbname as a conninfo is nowhere documented for \connect, it is reasonable to expect that it works because it does work in many other contexts. Therefore this is backpatched all the way back to 9.0. To implement this, routines previously private to libpq have been duplicated so that psql can decide what looks like a conninfo/URI string. In back branches, just duplicate the same code all the way back to 9.2, where URIs where introduced; 9.0 and 9.1 have a simpler version. In master, the routines are moved to src/common and renamed. Author: David Fetter, Andrew Dunstan. Some editorialization by me (probably earning a Gierth's "Sloppy" badge in the process.) Reviewers: Andrew Gierth, Erik Rijkers, Pavel Stěhule, Stephen Frost, Robert Haas, Andrew Dunstan. Branch ------ master Details ------- http://git.postgresql.org/pg/commitdiff/fcef1617295c074f2684c887627184d2fc26ac04 Modified Files -------------- doc/src/sgml/ref/psql-ref.sgml | 40 ++++++++++++++----- src/bin/psql/command.c | 80 ++++++++++++++++++++++++++----------- src/bin/psql/help.c | 4 +- src/bin/psql/tab-complete.c | 13 +++++- src/common/Makefile | 2 +- src/common/connstrings.c | 53 ++++++++++++++++++++++++ src/include/common/connstrings.h | 16 ++++++++ src/interfaces/libpq/fe-connect.c | 46 +++------------------ 8 files changed, 175 insertions(+), 79 deletions(-)
On Thu, Apr 2, 2015 at 8:10 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
psql: fix \connect with URIs and conninfo strings
psql was already accepting conninfo strings as the first parameter in
\connect, but the way it worked wasn't sane; some of the other
parameters would get the previous connection's values, causing it to
connect to a completely unexpected server or, more likely, not finding
any server at all because of completely wrong combinations of
parameters.
This patch has broken the build on OSX:
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dromedary&dt=2015-04-02%2000%3A10%3A54
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dromedary&dt=2015-04-02%2000%3A10%3A54
My box complains as well.
I haven't looked at that in details, but it looks that there are linking problems with -lpgcommon, as it is the first time that libpq has a dependency with it.
I am seeing as well that this is at least missing at the top of connstring.c:
#ifndef FRONTEND
#error "This file is not expected to be compiled for backend code"
#endif
And that src/tools/msvc has not been updated to have connstring.c show up in the list of frontend-only objects for libpgcommon.
--
Michael
On Thu, Apr 2, 2015 at 10:29 AM, Michael Paquier <michael.paquier@gmail.com> wrote:
On Thu, Apr 2, 2015 at 8:10 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:psql: fix \connect with URIs and conninfo strings
psql was already accepting conninfo strings as the first parameter in
\connect, but the way it worked wasn't sane; some of the other
parameters would get the previous connection's values, causing it to
connect to a completely unexpected server or, more likely, not finding
any server at all because of completely wrong combinations of
parameters.This patch has broken the build on OSX:
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dromedary&dt=2015-04-02%2000%3A10%3A54My box complains as well.I haven't looked at that in details, but it looks that there are linking problems with -lpgcommon, as it is the first time that libpq has a dependency with it.
I am seeing as well that this is at least missing at the top of connstring.c:
#ifndef FRONTEND
#error "This file is not expected to be compiled for backend code"
#endifAnd that src/tools/msvc has not been updated to have connstring.c show up in the list of frontend-only objects for libpgcommon.
On other Linux machines, tests for dblink are failing:
+ ERROR: could not load library "/usr/src/pgfarm/build/HEAD/inst/lib/postgresql/dblink.so": /usr/src/pgfarm/build/HEAD/inst/lib/libpq.so.5: undefined symbol: libpq_connstring_is_recognized
--
+ ERROR: could not load library "/usr/src/pgfarm/build/HEAD/inst/lib/postgresql/dblink.so": /usr/src/pgfarm/build/HEAD/inst/lib/libpq.so.5: undefined symbol: libpq_connstring_is_recognized
--
Michael
On Thu, Apr 2, 2015 at 10:58 AM, Michael Paquier wrote:
On other Linux machines, tests for dblink are failing:
+ ERROR: could not load library "/usr/src/pgfarm/build/HEAD/inst/lib/postgresql/dblink.so": /usr/src/pgfarm/build/HEAD/inst/lib/libpq.so.5: undefined symbol: libpq_connstring_is_recognized
The patch attached fixes all those inconsistencies (tested build on OSX and Windows).
--
--
Michael
Attachment
Michael Paquier <michael.paquier@gmail.com> writes:
> The patch attached fixes all those inconsistencies (tested build on OSX and
> Windows).
I think this is going in the wrong direction entirely, ie doubling down
on Alvaro's original mistake. libpq *must not* depend on libpgcommon,
because the latter is not compiled to be position-independent code
(on machines where that matters). Furthermore, proposing to add this:
+#ifndef FRONTEND
+#error "This file is not expected to be compiled for backend code"
+#endif
seems to me to prove that connstrings.c didn't belong in src/common
in the first place.
I'm too tired to think through exactly what this should be like instead,
but we do have rules about what goes where, and the response to violating
those rules should not be to break down the divisions even more.
regards, tom lane
On Thu, Apr 2, 2015 at 11:49 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Michael Paquier <michael.paquier@gmail.com> writes:
> The patch attached fixes all those inconsistencies (tested build on OSX and
> Windows).
I think this is going in the wrong direction entirely, ie doubling down
on Alvaro's original mistake. libpq *must not* depend on libpgcommon,
because the latter is not compiled to be position-independent code
(on machines where that matters).
Hm, OK. I was not aware of that.
Furthermore, proposing to add this:
+#ifndef FRONTEND
+#error "This file is not expected to be compiled for backend code"
+#endif
seems to me to prove that connstrings.c didn't belong in src/common
in the first place.
I'm too tired to think through exactly what this should be like instead,
but we do have rules about what goes where, and the response to violating
those rules should not be to break down the divisions even more.
--
Michael
On Wed, Apr 1, 2015 at 9:58 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On other Linux machines, tests for dblink are failing:
> + ERROR: could not load library
> "/usr/src/pgfarm/build/HEAD/inst/lib/postgresql/dblink.so":
> /usr/src/pgfarm/build/HEAD/inst/lib/libpq.so.5: undefined symbol:
> libpq_connstring_is_recognized
I can't even build the server.
ar crs libpq.a fe-auth.o fe-connect.o fe-exec.o fe-misc.o fe-print.o
fe-lobj.o fe-protocol2.o fe-protocol3.o pqexpbuffer.o fe-secure.o
libpq-events.o chklocale.o inet_net_ntop.o noblock.o pgstrcasecmp.o
pqsignal.o thread.o ip.o md5.o encnames.o wchar.o fe-secure-openssl.o
Undefined symbols for architecture x86_64:
"_libpq_connstring_is_recognized", referenced from:
_PQconnectStartParams in fe-connect.o
_PQsetdbLogin in fe-connect.o
"_libpq_connstring_uri_prefix_length", referenced from:
_parse_connection_string in fe-connect.o
ld: symbol(s) not found for architecture x86_64
collect2: ld returned 1 exit status
This is really inconvenient.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> This is really inconvenient.
I'm going to revert that commit in HEAD shortly, unless Alvaro pops
up and promises a fix PDQ. Or you could do the same.
regards, tom lane
On Thu, Apr 2, 2015 at 10:00 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> This is really inconvenient. > > I'm going to revert that commit in HEAD shortly, unless Alvaro pops > up and promises a fix PDQ. Or you could do the same. I was thinking of changing master to look like the 9.4 version. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Apr 2, 2015 at 10:00 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm going to revert that commit in HEAD shortly, unless Alvaro pops
>> up and promises a fix PDQ. Or you could do the same.
> I was thinking of changing master to look like the 9.4 version.
[ shrug... ] IMO, a quick "git revert" is less work and leaves a cleaner
state for Alvaro to apply whatever final solution he settles on.
But do what you wish.
regards, tom lane
On Thu, Apr 2, 2015 at 10:09 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Thu, Apr 2, 2015 at 10:00 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> I'm going to revert that commit in HEAD shortly, unless Alvaro pops >>> up and promises a fix PDQ. Or you could do the same. > >> I was thinking of changing master to look like the 9.4 version. > > [ shrug... ] IMO, a quick "git revert" is less work and leaves a cleaner > state for Alvaro to apply whatever final solution he settles on. > But do what you wish. OK, I've just reverted it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Apr 2, 2015 at 10:19:51AM -0400, Robert Haas wrote: > On Thu, Apr 2, 2015 at 10:09 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Robert Haas <robertmhaas@gmail.com> writes: > >> On Thu, Apr 2, 2015 at 10:00 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >>> I'm going to revert that commit in HEAD shortly, unless Alvaro pops > >>> up and promises a fix PDQ. Or you could do the same. > > > >> I was thinking of changing master to look like the 9.4 version. > > > > [ shrug... ] IMO, a quick "git revert" is less work and leaves a cleaner > > state for Alvaro to apply whatever final solution he settles on. > > But do what you wish. > > OK, I've just reverted it. Can I ask about the logic of why this bug fix was backpatched, or is that clear to everyone but me. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Thu, Apr 2, 2015 at 10:38 AM, Bruce Momjian <bruce@momjian.us> wrote: > On Thu, Apr 2, 2015 at 10:19:51AM -0400, Robert Haas wrote: >> On Thu, Apr 2, 2015 at 10:09 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> > Robert Haas <robertmhaas@gmail.com> writes: >> >> On Thu, Apr 2, 2015 at 10:00 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> >>> I'm going to revert that commit in HEAD shortly, unless Alvaro pops >> >>> up and promises a fix PDQ. Or you could do the same. >> > >> >> I was thinking of changing master to look like the 9.4 version. >> > >> > [ shrug... ] IMO, a quick "git revert" is less work and leaves a cleaner >> > state for Alvaro to apply whatever final solution he settles on. >> > But do what you wish. >> >> OK, I've just reverted it. > > Can I ask about the logic of why this bug fix was backpatched, or is > that clear to everyone but me. What is your question, exactly? There was a fair amount of discussion of whether and how to back-patch this on pgsql-hackers. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
eOn Thu, Apr 2, 2015 at 10:41:55AM -0400, Robert Haas wrote: > On Thu, Apr 2, 2015 at 10:38 AM, Bruce Momjian <bruce@momjian.us> wrote: > > On Thu, Apr 2, 2015 at 10:19:51AM -0400, Robert Haas wrote: > >> On Thu, Apr 2, 2015 at 10:09 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> > Robert Haas <robertmhaas@gmail.com> writes: > >> >> On Thu, Apr 2, 2015 at 10:00 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> >>> I'm going to revert that commit in HEAD shortly, unless Alvaro pops > >> >>> up and promises a fix PDQ. Or you could do the same. > >> > > >> >> I was thinking of changing master to look like the 9.4 version. > >> > > >> > [ shrug... ] IMO, a quick "git revert" is less work and leaves a cleaner > >> > state for Alvaro to apply whatever final solution he settles on. > >> > But do what you wish. > >> > >> OK, I've just reverted it. > > > > Can I ask about the logic of why this bug fix was backpatched, or is > > that clear to everyone but me. > > What is your question, exactly? There was a fair amount of discussion > of whether and how to back-patch this on pgsql-hackers. Uh, I didn't see a huge amount of discussion, but I guess it is sufficient: http://www.postgresql.org/message-id/flat/20150109191551.GA32660@fetter.org#20150109191551.GA32660@fetter.org -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Thu, Apr 2, 2015 at 10:57 AM, Bruce Momjian <bruce@momjian.us> wrote: > Uh, I didn't see a huge amount of discussion, but I guess it is > sufficient: > > http://www.postgresql.org/message-id/flat/20150109191551.GA32660@fetter.org#20150109191551.GA32660@fetter.org There are at least a dozen messages on that thread that are entirely dedicated to whether this should be back-patched. How much discussion did you want? Now, mind you, my view did not prevail, so I can't be completely happy with the outcome, but I was in the minority. At least the version that broke essentially the entire build-farm only went into master, so that's something. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas wrote: > On Thu, Apr 2, 2015 at 10:57 AM, Bruce Momjian <bruce@momjian.us> wrote: > > Uh, I didn't see a huge amount of discussion, but I guess it is > > sufficient: > > > > http://www.postgresql.org/message-id/flat/20150109191551.GA32660@fetter.org#20150109191551.GA32660@fetter.org > > There are at least a dozen messages on that thread that are entirely > dedicated to whether this should be back-patched. How much discussion > did you want? Yeah, this patch did get more discussion than many other patches do. Sometimes you just have to go against what looks like apathy, and move forward. That's what commitfests are supposed to be good for, after all: personally, I care not one bit about this functionality, but somebody went huge lengths to get it to work, which must mean that there definitely is value in it. > Now, mind you, my view did not prevail, so I can't be completely happy > with the outcome, but I was in the minority. At least the version > that broke essentially the entire build-farm only went into master, so > that's something. Yeah, I realized that backpatching the refactoring was the wrong approach, though I didn't notice the breakage in master until it was much too late. I pushed the 9.4 version to master now. Apologies for not fixing it sooner -- had a funeral to attend, a drive 5 hours away. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services