Re: plpgsql_check_function - rebase for 9.3

Hello

thank you for review

2013/12/7 Steve Singer <steve(at)ssinger(dot)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(at)mail(dot)gmail(dot)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(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.
>>
>>
>>
>>
>>
>>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>