Re: plpgsql_check_function - rebase for 9.3

On 08/22/2013 02:02 AM, Pavel Stehule wrote:
> rebased
>
> Regards
>
> Pavel
>
This patch again needs a rebase, it no longer applies cleanly.
plpgsql_estate_setup now takes a shared estate parameter and the call in
pl_check needs that. I passed NULL in and things seem to work.

I think this is another example where are processes aren't working as
we'd like.

The last time this patch got a review was in March when Tom said
http://www.postgresql.org/message-id/27661.1364267665@sss.pgh.pa.us

Shortly after Pavel responded with a new patch that changes the output
format.
http://www.postgresql.org/message-id/CAFj8pRAqpAEWqL465xkTmwe-DaZ8+7N-oEQ=uv_JjoP21FJmrQ@mail.gmail.com

I don't much has progress in the following 9 months on this patch.

In Tom's review the issue of code duplication and asks:
"do we want to accept that this is the implementation pathway we're
going to settle for, or are we going to hold out for a better one? I'd
be the first in line to hold out if I had a clue how to move towards a
better implementation, but I have none."

This question is still outstanding.

Here are my comments on this version of the patch:

This version of the patch gives output as a table with the structure:

functionid | lineno | statement | sqlstate | message | detail | hint | level | position | query | context
------------+--------+-----------+----------+---------+--------+------+-------+----------+-------+---------

I like this much better than the previous format.

I think the patch needs more documentation.

The extent of the documentation is

<title>Checking of embedded SQL</title>
+ <para>
+ The SQL statements inside <application>PL/pgSQL</> functions are
+ checked by validator for semantic errors. These errors
+ can be found by <function>plpgsql_check_function</function>:

I a don't think that this adequately describes the function. It isn't clear to me what we mean by 'embedded' SQL.
and 'semantic errors'.

I think this would read better as

<sect2 id="plpgsql-check-function">
<title>Checking SQL Inside Functions</title>
<para>
The SQL Statements inside of <application>PL/pgsql</> functions can be
checked by a validation function for semantic errors. You can check
<application>PL/pgsl</application> functions by passing the name of the
function to <function>plpgsql_check_function</function>.
</para>
<para>
The <function>plpgsql_check_function</function> will check the SQL
inside of a <application>PL/pgsql</application> function for certain
types of errors and warnings.
</para>

but that needs to be expanded on.

I was expecting something like:

create function x() returns void as $$ declare a int4; begin a='f'; end $$ language plpgsql;

to return an error but it doesn't

but

create temp table x(a int4);
create or replace function x() returns void as $$ declare a int4; begin insert into x values('h'); end $$ language plpgsql;

does give an error when I pass it to the validator. Is this the intended behavior of the patch? If so we need to explain why the first function doesn't generate an error.

I feel we need to document what each of the columns in the output means. What is the difference between statement, query and context?

Some random comments on the messages you return:

Line 816:

if (is_expression && tupdesc->natts != 1)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("qw",
query->query,
tupdesc->natts)));

Should this be "query \"%s\" returned %d columns but only 1 is allowed" or something similar?

Line 837

else
/* XXX: should be a warning? */
ereport(ERROR,
(errmsg("cannot to identify real type for record type variable")));

In what situation does this come up? Should it be a warning or error?
Do we mean "the real type for record variable" ?

Line 1000
ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("function does not return
composite type, is not possible to identify composite type")));

Should this be "function does not return a composite type, it is not
possible to identify the composite type" ?

Line 1109:

appendStringInfo(&str, "parameter \"%s\" is overlapped",
+ refname);

gives warnings like:

select message,detail FROM plpgsql_check_function('b(int)');
message | detail
-----------------------------+--------------------------------------------
parameter "a" is overlapped | Local variable overlap function parameter.

How about instead
"parameter "a" is duplicated" | Local variable is named the same as the
function parameter

Line 1278:

+ checker_error(cstate,
+ 0, 0,
+ "cannot determinate a result of
dynamic SQL",
+ "Cannot to contine in check.",
+ "Don't use dynamic SQL and record type together,
when you would check function.",
+ "warning",
+ 0, NULL, NULL);

How about
"cannot determine the result of dynamic SQL" , "Cannot continue
validating the function", "Do not use plpgsql_check_function on
functions with dynamic SQL"
Also this limitation should be explained in the documentation.

I also thing we need to distinguish between warnings generated because
of problems in the function versus warnings generated because of
limitations in the validator. This implies that there is maybe
something wrong with my function but there is nothing wrong with using
dynamic SQL in functions this is just telling users about a runtime
warning of the validator itself.

Same thing around line 1427

I have not done an in-depth read of the code.

I'm sending this out this patch at least gets some review. I don't
think that I will have a lot more time in the next week to do a more
thorough review or follow-up review

If we aren't happy with the overal approach of this patch then we need
to tell Pavel.

My vote would be to try to get the patch (documentation, error messages,
'XXX' items, etc) into a better state so it can eventually be committed

Steve

>
> 2013/8/22 Peter Eisentraut <peter_e(at)gmx(dot)net <mailto:peter_e(at)gmx(dot)net>>
>
> On Wed, 2013-03-27 at 23:25 +0100, Pavel Stehule wrote:
> > I redesigned output from plpgsql_check_function. Now, it returns
> table
> > everytime.
> > Litlle bit code simplification.
>
> This patch is in the 2013-09 commitfest but needs a rebase.
>
>
>
>
>