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 <t...@sss.pgh.pa.us>: > Josh Berkus <j...@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-k8...@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
plpgsql_check_function_20130327.patch.gz
Description: GNU Zip compressed data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers