Re: Improvements in psql hooks for variables

Hello,

Please consider following comments on the patch.

In function ParseVariableNum,

> if (!val || !val[0])
>       return false;

Check for 'val == NULL' as in above condition is done even in callers of
ParseVariableNum().
There should be only one such check.

>+       psql_error("Invalid value \"%s\" for \"%s\"\nAn integer is
expected\n",
Cant we have this error in ParseVariableNum() similar to
ParseVariableBool() ?

>+       /*
>+        * Switching AUTOCOMMIT from OFF to ON is not allowed when inside a
>+        * transaction, because it cannot be effective until the current
>+        * transaction is ended.
>+        */
>+       if (autocommit && !pset.autocommit &&
>+           pset.db && PQtransactionStatus(pset.db) == PQTRANS_INTRANS)
>+       {
>+           psql_error("cannot set AUTOCOMMIT to %s when inside a
transaction\n", newval);
>+       }
The above change in autocommit behaviour needs to be documented.


Thank you,
Rahila Syed




On Wed, Jan 25, 2017 at 5:48 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:

> Hello,
>
> The patch works fine on applying on latest master branch and testing it
> for various variables as listed in PsqlSettings struct.
> I will post further comments on patch soon.
>
> Thank you,
> Rahila Syed
>
> On Wed, Jan 25, 2017 at 1:33 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>> "Daniel Verite" <daniel@manitou-mail.org> writes:
>> > Here's an update with these changes:
>>
>> I scanned this patch very quickly and think it addresses my previous
>> stylistic objections.  Rahila is the reviewer of record though, so
>> I'll wait for his comments before going further.
>>
>>                         regards, tom lane
>>
>
>