Re: poll: CHECK TRIGGER?

Hello

2012/3/28 Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>:
> Ok, seems that the API issue is settled, so I'm now looking at the code
> actually doing the checking. My first impression is that this is a lot of
> code. Can we simplify it?
>

I am afraid so there are not a big space for simplification :(

> Since this is deeply integrated into the PL/pgSQL interpreter, I was
> expecting that this would run through the normal interpreter, in a special
> mode that skips all the actual execution of queries, and shortcuts all loops
> and other control statements so that all code is executed only once. That
> would mean sprinkling some "if (check_only)" code into the normal exec_*
> functions. I'm not sure how invasive that would be, but it's worth
> considering. I think you would be able to more easily catch more errors that
> way, and the check code would stay better in sync with the execution code.
>

This can mess current code - it can works, but some important
fragments can be less readable after. Almost all "eval" routines
should supports fake mode, and it can be little bit slower and less
readable.

> Another thought is that check_stmt() and all its subroutines are very
> similar to the plpgsql_dumptree() code. Would it make sense to merge those?
> You could have an output mode, in addition to the xml and plain-text
> formats, that would just dump the whole tree like plpgsql_dumptree() does.
>

yes, it is possible - first implementation was via walker, and it was
reused for dump. Current code is more readable, but there is not
reuse. I am able to redesign code to this direction.

> 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.

This is not possible, when we would to enable "fatal_errors = false"
checking. I can do subtransaction in prepare_expr, because it is the
most deep level, but I cannot to use it elsewhere, because I cannot
handle exception and continue with other parts of statement.

>
> If you create a function with an invalid body (ie. set
> check_function_bodies=off; create function ... $$ bogus $$;) ,
> plpgsql_check_function() still throws an error. It's understandable that it
> cannot do in-depth analysis if the function cannot be parsed, but I would
> expect the syntax error to be returned as a return value like other errors
> that it complains about, not thrown as a hard ERROR. That would make it more
> useful to bulk-check all functions in a database with something like "select
> plpgsql_check_function(oid) from pg_class". As it is, the checking stops at
> the first invalid function with an error.
>

it is good idea

> PS. I think plpgsql_check_function() belongs in pl_handler.c

can be - why not.

Regards

Pavel

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