Re: plpgsql_check_function - rebase for 9.3 - Mailing list pgsql-hackers

Hello

I redesigned output from plpgsql_check_function. Now, it returns table
everytime.
Litlle bit code simplification.

postgres=# \sf fx
CREATE OR REPLACE FUNCTION public.fx(integer)
 RETURNS integer
 LANGUAGE plpgsql
AS $function$
BEGIN
  RETURN (SELECT a FROM t1 WHERE b < $1);
END;
$function$
postgres=# select * from plpgsql_check_function('fx(int)');
-[ RECORD 1 ]--------------------------------------
functionid | fx
lineno     | 3
statement  | RETURN
sqlstate   | 42703
message    | column "b" does not exist
detail     | [null]
hint       | [null]
level      | error
position   | 32
query      | SELECT (SELECT a FROM t1 WHERE b < $1)
context    | [null]

Regards

Pavel


2013/3/26 Tom Lane <tgl@sss.pgh.pa.us>:
> Josh Berkus <josh@agliodbs.com> writes:
>> Where are we with this patch?  I'm a bit unclear from the discussion on
>> whether it passes muster or not.  Things seem to have petered out.
>
> I took another look at this patch tonight.  I think the thing that is
> bothering everybody (including Pavel) is that as submitted, pl_check.c
> involves a huge amount of duplication of knowledge and code from
> pl_exec.c, and to a lesser extent pl_comp.c.  It certainly looks like a
> maintenance disaster in the making.  It doesn't bother me so much that
> pl_check.c knows about each syntactic structure in plpgsql: there are
> already four or five places you have to touch when adding new syntax.
> Rather, it's that there's so much duplication of knowledge about
> semantic details, which is largely expressed by copied-and-pasted code
> from pl_exec.c.  It seems like a safe bet that we'll sometimes miss the
> need to fix pl_check.c when we fix something in pl_exec.c.  There are
> annoying duplications from pl_comp.c as well, eg knowledge of exactly
> which magic variables are defined in trigger functions.
>
> Having said all that, it's not clear that we can really do any better.
> The only obvious alternative is pushing support for a checking mode
> directly into pl_exec.c, which would obfuscate and slow down that code
> to an unacceptable degree if Pavel's results at
> http://www.postgresql.org/message-id/CAFj8pRAKuJmVjPjzfSryE7+uB8jF8Wtz5rkxK-0ykXme-k81kA@mail.gmail.com
> are any indication.  (In that message, Pavel proposes shoveling the
> problem into the compile code instead, but that seems unlikely to work
> IMO: the main problem in pl_check.c as it stands is duplication of code
> from pl_exec.c not pl_comp.c.  So I think that path could only lead to
> duplicating the same code into pl_comp.c.)
>
> So question 1 is 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.  Are we
> prepared to reject this type of feature entirely because of the
> code-duplication problem?  That doesn't seem attractive either.
>
> But, even granting that this implementation approach is acceptable,
> the patch does not seem close to being committable as-is: there's
> a lot of fit-and-finish work yet to be done.  To make my point I will
> just quote from one of the regression test additions:
>
> create or replace function f1()
> returns void as $$
> declare a1 int; a2 int;
> begin
>   select 10,20 into a1;
> end;
> $$ language plpgsql;
> -- raise warning
> select plpgsql_check_function('f1()');
>                          plpgsql_check_function
> -------------------------------------------------------------------------
>  warning:00000:4:SQL statement:too many attributies for target variables
>  Detail: There are less target variables than output columns in query.
>  Hint: Check target variables in SELECT INTO statement
> (3 rows)
>
> Do we like this output format?  I don't.  The unlabeled, undocumented
> fields crammed into a single line with colon separators are neither
> readable nor useful.  If we actually need these fields, why aren't we
> splitting the output into multiple columns?  (I'm also wondering why
> the patch bothers with an option to emit this same info in XML.  Surely
> there is vanishingly small use-case for mechanical examination of this
> output.)
>
> This example also shows that the user-visible messages have had neither
> editing from a native speaker of English, nor even attention from a
> spell checker.  I don't fault Pavel for that (his English is far better
> than my Czech) but this patch has been passed by at least one reviewer
> who is a native speaker.  Personally I'd also say that neither the
> Detail nor Hint messages in this example are worth the electrons they
> take up, as they add nothing useful to the basic error message.  I'd be
> embarrassed to be presenting output like this to end users of Postgres.
>
> (The code comments are in even worse shape than the user-visible
> messages, by the by, where they exist at all.)
>
> A somewhat related point is that it doesn't look like any thought
> has been taken about localized message output.
>
> This stuff is certainly all fixable if we agree on the basic
> implementation approach; and I can't really fault Pavel for not
> worrying much about such details while the implementation approach
> hasn't been agreed to.  But I'd judge that there's a week or more's
> worth of work here to get to a patch I'd be happy about committing,
> even without any change in the basic approach.  That's not time I'm
> willing to put in personally right now.
>
>                         regards, tom lane