On Wed, Nov 2, 2011 at 5:36 PM, Andrew Dunstan <and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
