Patch: psql \whoami option
in [PgSql]
|
Prev: Small FSM is too large
Next: deprecating =>, take two
From: Robert Haas on 21 Jun 2010 00:07 On Sun, Jun 20, 2010 at 10:51 PM, Steve Singer <ssinger_pg(a)sympatico.ca> wrote: > One comment I have on the output format is that values (ie the database > name) are enclosed in double quotes but the values being quoted can contain > double quotes that are not being escaped. � For example > > Connected to database: "testing"er", user: "ssinger", port: "5432" via local > domain socket > > (where my database name is testing"er ). �Programs will have a hard time > parsing this. �I'm not sure if this is a valid concern but I'm mentioning > it. It seems like for user and database it might be sensible to apply PQescapeIdentifier to the value before printing it. This will double-quote it and escape any internal double-quotes appropriately. The port is, I guess, being stored as a string, but doesn't it have to be an integer? In which case, why quote it at all? Is there really a point to the non-DSN format or should we just use the DSN format always? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Steve Singer on 20 Jun 2010 22:51 This is a review for the \whoami patch (changed to \conninfo). This review was done on the Feb 2 2010 version of the patch (rebased to head) that reflects some of the feedback from -hackers on the initial submission. The commitfest entry should be updated to reflect the most recent version of this patch that David emailed to me. Content & Purpose ======================== The patch adds a \conninfo command to psql to print connection information for the current connection. The patch includes documentation updates but no regression test changes. I don't see regression tests for other psql '\' commands so I don't think they are required in this case either. Usability Review ========================== The initial discussion on -hackers recommened renaming the command to \conninfo which was done. One comment I have on the output format is that values (ie the database name) are enclosed in double quotes but the values being quoted can contain double quotes that are not being escaped. For example Connected to database: "testing"er", user: "ssinger", port: "5432" via local domain socket (where my database name is testing"er ). Programs will have a hard time parsing this. I'm not sure if this is a valid concern but I'm mentioning it. Initial Run ============== Connecting both through tcp/ip and unix domain sockets produces valid \conninfo output. The regression tests pass when the patch is applied. Performance ============= I see no performance implications of this patch. Code & Nitpicking ================ in command.c you have the opening brace on the same line as the if. See "if (host) {" and the associated "else {" The block " else if (strcmp(cmd, "conninfo") == 0)" is in between the commands "\c" and "\cd" it looks like the commands are ordered alphabetically. Wouldn't conninfo fit in after "\cd" but before "\copy" In help.c you don't update the row count at the top of slashUsage() per the comment you should increment it. Other than those issues the patch looks fine. Steve -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Tom Lane on 21 Jun 2010 10:00 Robert Haas <robertmhaas(a)gmail.com> writes: > On Sun, Jun 20, 2010 at 10:51 PM, Steve Singer <ssinger_pg(a)sympatico.ca> wrote: >> One comment I have on the output format is that values (ie the database >> name) are enclosed in double quotes but the values being quoted can contain >> double quotes that are not being escaped. This is the same as standard practice in just about every other message... > It seems like for user and database it might be sensible to apply > PQescapeIdentifier to the value before printing it. I think this would actually be a remarkably bad idea in this particular instance, because in the majority of cases psql does not apply identifier dequoting rules to user and database names. What is printed should be the same as what you'd need to give to \connect, for example. > The port is, I guess, being stored as a string, but doesn't it have to > be an integer? In which case, why quote it at all? Agreed, no need for quotes there. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Tom Lane on 21 Jun 2010 10:51 Robert Haas <robertmhaas(a)gmail.com> writes: > Is there really a point to the non-DSN format or should we just use > the DSN format always? BTW, didn't have an opinion on that to start with, but after thinking about it I'd turn it around. psql doesn't deal in DSN format anywhere else, so why should it do so here? To make the point more obvious, what's the justification for printing DSN format and not, say, JDBC URL format? I'd vote for removing the DSN printout option, not the other way round. If there was some mechanically readable format to offer to print, it would be conninfo string format, which you can actually use with psql if you have a mind to. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: David Christensen on 18 Jul 2010 13:17
On Jun 21, 2010, at 9:00 AM, Tom Lane wrote: > Robert Haas <robertmhaas(a)gmail.com> writes: >> On Sun, Jun 20, 2010 at 10:51 PM, Steve Singer <ssinger_pg(a)sympatico.ca> wrote: >>> One comment I have on the output format is that values (ie the database >>> name) are enclosed in double quotes but the values being quoted can contain >>> double quotes that are not being escaped. > > This is the same as standard practice in just about every other > message... > >> It seems like for user and database it might be sensible to apply >> PQescapeIdentifier to the value before printing it. > > I think this would actually be a remarkably bad idea in this particular > instance, because in the majority of cases psql does not apply > identifier dequoting rules to user and database names. What is printed > should be the same as what you'd need to give to \connect, for example. So I'm not quite sure how the above two paragraphs resolve? Should the user/database names be quoted or not? I have a new version of this patch available which has incorporated the feedback to this point? As an example of the current behavior, consider: machack:machack:5432=# create database "foo""bar" machack-# ; CREATE DATABASE [Sun Jul 18 12:14:49 CDT 2010] machack:machack:5432=# \c foo"bar unterminated quoted string You are now connected to database "machack". [Sun Jul 18 12:14:53 CDT 2010] machack:machack:5432=# \c "foo"bar" unterminated quoted string You are now connected to database "machack". [Sun Jul 18 12:14:59 CDT 2010] machack:machack:5432=# \c "foo""bar" You are now connected to database "foo"bar". As you can see, the value passed to connect differs from the output in the "connected to database" string. Regards, David -- David Christensen End Point Corporation david(a)endpoint.com -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |