Re: plpgsql_check_function - rebase for 9.3

Hello

thank you for review

2013/12/7 Steve Singer <steve@ssinger.info>

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

yes, I am waiting still to a) have some better idea, or b) have significantly more free time for larger plpgsql refactoring. Nothing of it happens :(

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

This is expected. PL/pgSQL use a late casting - so a := '10'; is acceptable. And in this moment plpgsql check doesn't evaluate constant and doesn't try to cast to target type. But this check can be implemented sometime. In this moment, we have no simple way how to identify constant expression in plpgsql. So any constants are expressions from plpgsql perspective.

 

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;

it is expected too. SQL doesn't use late casting - casting is controlled by available casts and constants are evaluated early - due possible optimization.

 

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" ?

a most difficult part of plpgsql check function is about transformation dynamic types to know row types. It is necessary for checking usable functions and operators.

typical pattern is:

declare r record;

begin

  for r in select a, b from some_tab

  loop

    raise notice '%', extract(day from r.a);

  end loop;

and we should to detect type of r.a. Sometimes we cannot to do it due using dynamic SQL - plpgsql check doesn't try to evaluate expressions (as protection against side efects).

 

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

I have no idea about good sentence, but "duplicate" probably is not best. Any variable can be locally overlapped. Probably any overlapping should be signalized - it is a bad design always.

 

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

Thank you

Regards

Pavel

 

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers