On 30.03.2012 12:36, Pavel Stehule wrote:
> 2012/3/28 Heikki Linnakangas<heikki.linnakangas@enterprisedb.com>:
>> In prepare_expr(), you use a subtransaction to catch any ERRORs that happen
>> during parsing the expression. That's a good idea, and I think many of the
>> check_* functions could be greatly simplified by adopting a similar
>> approach. Just ereport() any errors you find, and catch them at the
>> appropriate level, appending the error to the output string. Your current
>> approach of returning true/false depending on whether there was any errors
>> seems tedious.
>
> It cannot be implemented in AST interpret. Without removing some
> requested functionality - fatal_errors.
I don't think I'm getting my point across by explaining, so here's a
modified version of the patch that does what I was trying to say. The
general pattern of the checker functions has been changed. Instead of
returning a boolean indicating error or no error, with checks for
fatal_errors scattered around them, the internal checker functions now
return nothing. Any errors are reported with ereport(), and there is a
PG_TRY/CATCH block in a couple of carefully chosen places: in
check_stmt(), so that if you get an error while checking a statement,
you continue checking on the next statement, and in check_assignment()
which is now used by check_expr() and a few other helper functions to
basically check all expressions and SQL statements.
IMHO this makes the code much more readable, now that the control logic
of when to return and when to continue is largely gone. A lot of other
cleanup still needs to be done, I just wanted to demonstrate this
ereport+try/catch idea with this patch.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
