Skip site navigation (1) Skip section navigation (2)

Re: Patch: psql \whoami option

From: Steve Singer <ssinger_pg(at)sympatico(dot)ca>
To: David Christensen <david(at)endpoint(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Patch: psql \whoami option
Date: 2010-06-21 02:51:01
Message-ID: BLU0-SMTP95FDB39843F3B8FCA1E560ACC30@phx.gbl (view raw or flat)
Thread:
Lists: pgsql-hackers

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


In response to

Responses

pgsql-hackers by date

Next:From: Robert HaasDate: 2010-06-21 03:54:21
Subject: Re: beta3 & the open items list
Previous:From: Tom LaneDate: 2010-06-21 02:25:12
Subject: Re: About tapes

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group