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(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
>> "Daniel Verite" <daniel(at)manitou-mail(dot)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
>>
>
>