Skip site navigation (1) Skip section navigation (2)


From: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
To: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>
Cc: Alvaro Herrera <alvherre(at)commandprompt(dot)com>, Peter Eisentraut <peter_e(at)gmx(dot)net>, Robert Haas <robertmhaas(at)gmail(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>, Petr Jelinek <pjmodos(at)pjmodos(dot)net>
Subject: Re: poll: CHECK TRIGGER?
Date: 2012-03-30 09:36:49
Message-ID: (view raw, whole thread or download thread mbox)
Lists: pgsql-hackers

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.


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.


postgres=> select plpgsql_check_function('sss'::regproc, 0);
 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



Pavel Stehule

> --
>  Heikki Linnakangas
>  EnterpriseDB

Attachment: plpgsql_check_function2-2012-03-30.diff.gz
Description: application/x-gzip (20.7 KB)

In response to


pgsql-hackers by date

Next:From: Dimitri FontaineDate: 2012-03-30 10:02:26
Subject: Re: Finer Extension dependencies
Previous:From: Dimitri FontaineDate: 2012-03-30 08:32:34
Subject: Re: Command Triggers patch v18

Privacy Policy | About PostgreSQL
Copyright © 1996-2018 The PostgreSQL Global Development Group