Re: plpgsql_check_function - rebase for 9.3

There is still valid a variant to move check function to contrib for fist
moment.

Regards

Pavel

2013/12/7 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>

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