Re: poll: CHECK TRIGGER?

2012/4/4 Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>:
> On 30.03.2012 12:36, Pavel Stehule wrote:
>>
>> 2012/3/28 Heikki Linnakangas<heikki(dot)linnakangas(at)enterprisedb(dot)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.

I checked your idea and it should to work.

What other cleanup (without mentioned in previous mails) do you think?

Regards

Pavel

>
>
> --
>  Heikki Linnakangas
>  EnterpriseDB   http://www.enterprisedb.com