There is still valid a variant to move check function to contrib for fist moment.
Regards Pavel 2013/12/7 Pavel Stehule <pavel.steh...@gmail.com> > Hello > > thank you for review > > > 2013/12/7 Steve Singer <st...@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.1364267...@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_jjop21fj...@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 <pete...@gmx.net <mailto:pete...@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 >> > >
