Re: ON_ERROR_ROLLBACK

Adrian Klaver <adrian(dot)klaver(at)aklaver(dot)com> writes:
> So it seems you can turn ON_ERROR_ROLLBACK on with either 1 or 'on', but you can only turn it off with 'off'.
> With ON_ERROR_STOP 1/on and 0/off both seem to work.

> Is this expected?

on_error_stop_hook() uses ParseVariableBool, while
on_error_rollback_hook() uses some hard-coded logic that checks for
"interactive" and "off" and otherwise assumes "on". This is inconsistent
first in not accepting as many spellings as ParseVariableBool, and second
in not complaining about invalid input --- ParseVariableBool does, so
why not here?

echo_hook, echo_hidden_hook, histcontrol_hook, and verbosity_hook are
equally cavalierly written.

For on_error_rollback_hook and echo_hidden_hook, where "on" and "off"
are documented values, I think it would make sense to allow all spellings
accepted by ParseVariableBool, say like this:

if (newval == NULL)
pset.on_error_rollback = PSQL_ERROR_ROLLBACK_OFF;
else if (pg_strcasecmp(newval, "interactive") == 0)
pset.on_error_rollback = PSQL_ERROR_ROLLBACK_INTERACTIVE;
- else if (pg_strcasecmp(newval, "off") == 0)
- pset.on_error_rollback = PSQL_ERROR_ROLLBACK_OFF;
- else
- pset.on_error_rollback = PSQL_ERROR_ROLLBACK_ON;
+ else if (ParseVariableBool(newval))
+ pset.on_error_rollback = PSQL_ERROR_ROLLBACK_ON;
+ else
+ pset.on_error_rollback = PSQL_ERROR_ROLLBACK_OFF;

The other 3 functions don't need to accept on/off I think, but they should
print a warning for unrecognized input like ParseVariableBool does.

And while we're at it, we should consistently allow case-insensitive
input; right now it looks like somebody rolled dice to decide whether
to use "strcmp" or "pg_strcasecmp" in these functions. Per line, not
even per function.

Given the lack of previous complaints, this probably isn't backpatching
material, but it sure seems like a bit of attention to consistency
would be warranted here.

regards, tom lane