On Wed, Nov 2, 2011 at 5:36 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
> Updated patch is attached - adding to Nov commitfest.
Review of the v2 patch:
* Submission Review
Patch applies with some fuzz and builds without warnings. I noticed
some tab characters being used in psql-ref.sgml where they shouldn't
be.
* Usability Review
I sort of doubt I'll use the feature myself, but there was at least
one +1 for the feature idea already, so it seems useful enough. That
said, the stated motivation for the patch:
> First, it would be useful to be able to set pager options and possibly other
> settings, so my suggestion is for a \setenv command that could be put in a
> .psqlrc file, something like:
seems like it would be more suited to being set in the user's
~/.profile (perhaps via an 'alias' if you didn't want the changes to
apply globally). So unless I miss something, the real niche for the
patch seems to be for people to interactively change environment
settings while within psql. Again, not something I'm keen on for my
own use, but if others think it's useful, that's good enough for me.
* Coding Review / Nitpicks
The patch implements \setenv via calls to unsetenv() and putenv(). As
the comment notes,
| That means we
| leak a bit of memory here, but not enough to worry about.
which seems true; the man page basically says there's nothing to be
done about the situation.
The calls to putenv() and unsetenv() are done without any real input
checking. So this admittedly-pathological case produces surprising
results:
test=# \setenv foo=bar baz
test=# \! echo $foo
bar=baz
Perhaps 'envvar' arguments with a '=' in the argument should just be
disallowed.
Also, should the malloc() of newval just use pg_malloc() instead?
* Reviewer Review
I haven't tested on Windows.
Josh