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 played with this and It is not be reduced without darkening current
code of pl_exec.c. So I moved code related to checking from files to
new file pl_check.c. This code is relative large - about 50 kb, but it
is relative simple and I hope it is readable. I afraid so this cannot
be reduced by reuse with other pl_func.c (significantly). Recursive
iteration over nodes is relative not big part of this patch (~25%) and
general stmt walker doesn't help too much.

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

-1

there are a few places, that are very difficult: basic block with
exception handling - exception handlers, CASE stmt, .... Other issue
is increasing of complexity some routines like exec_eval*

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

It is difficult now - without changes in plpgsql_stmt_if,
plpgsql_stmt_case and plpgsql_stmt_block is not possible to write
general walker that is usable for checking and dumptree. It needs
redesign of these nodes first.

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

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

done

postgres=> select plpgsql_check_function('sss'::regproc, 0);
plpgsql_check_function
-------------------------------------------------------------
error:42601:syntax error at or near "adasdfsadf"
Query: adasdfsadf
-- ^
Context: compilation of PL/pgSQL function "sss" near line 1
(4 rows)

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

done

Regards

Pavel Stehule

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