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
insideof <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
checkthe 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
explainwhy 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,
queryand 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
forrecord 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
overlapfunction 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@gmx.net <mailto:peter_e@gmx.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.
>
>
>
>
>