Thread: Re: [HACKERS] psql commandline conninfo
Tom Lane wrote:
> Martijn van Oosterhout <kleptog@svana.org> writes:
>
>> Does that mean that:
>> psql -d "service=myservice"
>> should Just Work(tm)? That would be nice.
>>
>
> Even more to the point,
>
> psql "service=myservice"
>
> which is why we want to overload dbname rather than any of the other
> PQsetdbLogin parameters for this --- dbname has pride of place in the
> command line syntax for several of the client programs.
>
> regards, tom lane
>
>
Right. Here's the patch I just knocked up, which seems to Just Work (tm) ;-)
cheers
andrew
Index: src/interfaces/libpq/fe-connect.c
===================================================================
RCS file: /cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.339
diff -c -r1.339 fe-connect.c
*** src/interfaces/libpq/fe-connect.c 21 Nov 2006 16:28:00 -0000 1.339
--- src/interfaces/libpq/fe-connect.c 12 Dec 2006 22:49:28 -0000
***************
*** 567,572 ****
--- 567,573 ----
const char *pwd)
{
PGconn *conn;
+ bool have_conninfo = false;
/*
* Allocate memory for the conn structure
***************
*** 575,585 ****
if (conn == NULL)
return NULL;
/*
* Parse an empty conninfo string in order to set up the same defaults
! * that PQconnectdb() would use.
*/
! if (!connectOptions1(conn, ""))
return conn;
/*
--- 576,609 ----
if (conn == NULL)
return NULL;
+ /*
+ * Have we got something that might be a conninfo string?
+ * If so, try that first.
+ */
+ if (dbName && strchr(dbName,'='))
+ {
+ if(connectOptions1(conn,dbName))
+ {
+ /* it was a conninfo string */
+ have_conninfo = true;
+ }
+ else
+ {
+ /* put things back the way they were so we can try again */
+ freePGconn(conn);
+ conn = makeEmptyPGconn();
+ if (conn == NULL)
+ return NULL;
+
+ }
+ }
+
/*
* Parse an empty conninfo string in order to set up the same defaults
! * that PQconnectdb() would use. Skip this if we already found a
! * conninfo string.
*/
! if (!have_conninfo && !connectOptions1(conn, ""))
return conn;
/*
***************
*** 613,619 ****
conn->pgtty = strdup(pgtty);
}
! if (dbName && dbName[0] != '\0')
{
if (conn->dbName)
free(conn->dbName);
--- 637,643 ----
conn->pgtty = strdup(pgtty);
}
! if (!have_conninfo && dbName && dbName[0] != '\0')
{
if (conn->dbName)
free(conn->dbName);
Andrew Dunstan <andrew@dunslane.net> writes:
> Right. Here's the patch I just knocked up, which seems to Just Work (tm) ;-)
The main objection I can see to this is that you'd get a fairly
unhelpful message if you intended a conninfo string and there was
anything wrong with your syntax (eg, misspelled keyword). Maybe we
should go with the conn: bit, although really that doesn't seem any
less likely to collide with actual dbnames than the "does it contain
"="" idea. Anyone have other ideas how to disambiguate?
regards, tom lane
On Dec 12, 2006, at 3:37 PM, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> Right. Here's the patch I just knocked up, which seems to Just >> Work (tm) ;-) > > The main objection I can see to this is that you'd get a fairly > unhelpful message if you intended a conninfo string and there was > anything wrong with your syntax (eg, misspelled keyword). Maybe we > should go with the conn: bit, although really that doesn't seem any > less likely to collide with actual dbnames than the "does it contain > "="" idea. Anyone have other ideas how to disambiguate? I would personally prefer a real option over a prefix, i.e. -- dbconn="service=foo" though the inline conninfo string in place of the dbname would be ideal. Perhaps like Tom suggests, if the value matches a conninfo regex (slightly more rigid than just containing an equals character) then we assume it is a conninfo string, but never try it as a dbname. If someone has a database named like a conninfo string (c'mon folks ;^) then they would need to pass it as explicitly an argument to '-d' or '--dbname', not as a bare argument. This is not completely b/w compatible of course, but IMO the added convenience outweighs the incompatibility. -Casey
Andrew Dunstan wrote: > Tom Lane wrote: >> "Andrew Dunstan" <andrew@dunslane.net> writes: >> >>> We change libpq from time to time. Besides, how many DBs are there that >>> match the name pattern /^conn:.*=/ ? My guess is mighty few. So I don't >>> expect lots of surprise. >>> >> >> Um, but how many DB names have an "=" in them at all? >> >> Basically what this proposal is about is migrating from separated >> dbname/user/host/port/etc parameters to a unified conninfo parameter. >> That seems to me like a good long-term objective, and so I'm willing >> to break a few eggs on the way to the omelet, as long as we're not >> breaking any very likely usages. >> >> So: who here has a database with "=" in the name? And hands up if >> you've got a database whose name begins with "conn:"? >> >> I'm betting zero response rate on both of those, so see no reason to >> contort the long-term definition for a very marginal difference in >> the extent of backwards compatibility ... >> >> >> > > I'm not sure -hackers is the most representative group to poll regarding > dbnames in use ... > > Anyway, if I understand your current position, the only change needed to > my current patch would be that if we fail to parse a dbname parameter > that contains an = we simply fail at that point, rather than retrying it > as a straight database name. > > I'm OK with that. > Here's the patch for what I think is the consensus position. If there's no objection I will apply this and document it. cheers andrew
Attachment
"Andrew Dunstan" <andrew@dunslane.net> writes:
> Here's the patch for what I think is the consensus position. If there's no
> objection I will apply this and document it.
Please do something for the comment for the connectOptions1 call.
As you've coded it, that is doing two completely different things
and the comment is almost completely unhelpful at explaining this
complexity. Oh, and casting away const gets no points for style.
I think you could do worse than to split it into two independent code
paths:
/*
* If the dbName parameter contains '=', assume it's a conninfo
* string.
*/
if (dbName && strchr(dbName,'='))
{
if (!connectOptions1(conn, dbName))
return conn;
}
else
{
/*
* Old-style path: first, parse an empty conninfo string in
* order to set up the same defaults that PQconnectdb() would use.
*/
if (!connectOptions1(conn, ""))
return conn;
/* Insert dbName parameter value into struct */
if (dbName && dbName[0] != '\0')
{
if (conn->dbName)
free(conn->dbName);
conn->dbName = strdup(dbName);
}
}
/*
* Insert remaining parameters into struct, overriding defaults
* (as well as any conflicting data from dbName taken as a conninfo).
*/
(This works because there's no reason the dbName parameter can't be
moved up to process before the rest.)
regards, tom lane
Tom Lane wrote: > "Andrew Dunstan" <andrew@dunslane.net> writes: >> Here's the patch for what I think is the consensus position. If there's >> no >> objection I will apply this and document it. > > Please do something for the comment for the connectOptions1 call. > As you've coded it, that is doing two completely different things > and the comment is almost completely unhelpful at explaining this > complexity. Oh, and casting away const gets no points for style. ouch. :-) Ok, I accept the reproof. In fact I got up this morning, had my coffee, and thought "That's a silly way to do it, I could make it much neater by moving the dbName processing up," and lo and behold when I read your email you had done it :-) It shall be done as you wish. BTW, what is the approved way to handle warnings about const? Copy the object? cheers andrew
"Andrew Dunstan" <andrew@dunslane.net> writes:
> BTW, what is the approved way to handle warnings about const? Copy the
> object?
Well, in the revised code there shouldn't be any warning at all, but
I think the mistake in your original was to declare the local variable
as "char *" instead of "const char *".
If "const" is being used as intended then a const-violation warning
would indeed suggest that you needed to make a writable copy.
Sometimes the problem is that you're working in a chunk of inadequately
const-ified code, ie, you're passing a const argument to some other
functions that do indeed treat their inputs as read-only but don't
declare them const. In such cases you can either run around and try to
inject const everywhere it should be, or hold your nose and use a cast,
or give up on marking your own argument const :-(. But you weren't
presented with that problem here, because connectOptions1() is already
const-ified.
regards, tom lane
I assume this patch will still allow a database name with an equals
sign, right?
psql "dbname='a=b'"
The libpq documentation isn't clear that single-quotes also escape
equals, but I assume that is true looking at the code:
http://www.postgresql.org/docs/8.2/static/libpq-connect.html
The passed string can be empty to use all default parameters, or it can
contain one or more parameter settings separated by whitespace. Each
parameter setting is in the form keyword = value. Spaces around the
equal sign are optional. To write an empty value or a value containing
spaces, surround it with single quotes, e.g., keyword = 'a value'.
Single quotes and backslashes within the value must be escaped with a
backslash, i.e., \' and \\.
---------------------------------------------------------------------------
Andrew Dunstan wrote:
> Andrew Dunstan wrote:
> > Tom Lane wrote:
> >> "Andrew Dunstan" <andrew@dunslane.net> writes:
> >>
> >>> We change libpq from time to time. Besides, how many DBs are there that
> >>> match the name pattern /^conn:.*=/ ? My guess is mighty few. So I don't
> >>> expect lots of surprise.
> >>>
> >>
> >> Um, but how many DB names have an "=" in them at all?
> >>
> >> Basically what this proposal is about is migrating from separated
> >> dbname/user/host/port/etc parameters to a unified conninfo parameter.
> >> That seems to me like a good long-term objective, and so I'm willing
> >> to break a few eggs on the way to the omelet, as long as we're not
> >> breaking any very likely usages.
> >>
> >> So: who here has a database with "=" in the name? And hands up if
> >> you've got a database whose name begins with "conn:"?
> >>
> >> I'm betting zero response rate on both of those, so see no reason to
> >> contort the long-term definition for a very marginal difference in
> >> the extent of backwards compatibility ...
> >>
> >>
> >>
> >
> > I'm not sure -hackers is the most representative group to poll regarding
> > dbnames in use ...
> >
> > Anyway, if I understand your current position, the only change needed to
> > my current patch would be that if we fail to parse a dbname parameter
> > that contains an = we simply fail at that point, rather than retrying it
> > as a straight database name.
> >
> > I'm OK with that.
> >
>
>
> Here's the patch for what I think is the consensus position. If there's no
> objection I will apply this and document it.
>
> cheers
>
> andrew
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: In versions below 8.0, the planner will ignore your desire to
> choose an index scan if your joining column's datatypes do not
> match
--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +