plpgsql_check_function - rebase for 9.3

Hi!

On Tue, Jun 26, 2012 at 1:19 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

I am sending lightly refreshed patch for checking plpgsql functions..

I checked different implementation, but without success: a) enhancing
of SPI to some fake mode can has negative impact on application, and
patch was not clear, b) generic plpgsql walker doesn't save lines too.

I invite any ideas how to improve this patch

I reviewed this and did a clean up for bitrot and a little whitespace.
In particular, it needed to learn a little about event triggers.

This patch is a follow on from an earlier review thread I found:
http://archives.postgresql.org/message-id/D960CB61B694CF459DCFB4B0128514C2072DF447@exadv11.host.magwien.gv.at

I dug through that thread a bit, and I believe issues raised by
Laurenz, Petr and Alvaro were resolved by Pavel over time.

All tests pass, and after a read-through, the code seems fine.

This also represents my inaugural use of pg_bsd_indent. I ran it on
pl_check.c - which made things mostly better. Happy to try and fix it
up more if someone can explain to me what (if anything) I did
incorrectly when using it.

-selena

--
http://chesnok.com

Selena Deckelmann <selena@chesnok.com> writes:

This also represents my inaugural use of pg_bsd_indent. I ran it on
pl_check.c - which made things mostly better. Happy to try and fix it
up more if someone can explain to me what (if anything) I did
incorrectly when using it.

It looks like you didn't give it a typedef list? Fetch the file from
http://buildfarm.postgresql.org/cgi-bin/typedefs.pl
and pass it with --typedefs=filename. If you're dealing with something
that adds new typedef names, you can add them to the list file manually.

regards, tom lane

On 10/07/2012 11:25 AM, Tom Lane wrote:

Selena Deckelmann <selena@chesnok.com> writes:

This also represents my inaugural use of pg_bsd_indent. I ran it on
pl_check.c - which made things mostly better. Happy to try and fix it
up more if someone can explain to me what (if anything) I did
incorrectly when using it.

It looks like you didn't give it a typedef list? Fetch the file from
http://buildfarm.postgresql.org/cgi-bin/typedefs.pl
and pass it with --typedefs=filename. If you're dealing with something
that adds new typedef names, you can add them to the list file manually.

You's not supposed to be calling pg_bsd_indent directly at all. You's
supposed to be calling pgindent - it will call pg_bsd_indent as needed
and adjust the results. See src/tools/pgindent/README.

cheers

andrew

Hello

I am sending a updated version - now it is prepared for event triggers
and it is little bit more robust

I run pgindent, but I have no experience with it, so I am not sure about success

Regards

Pavel Stehule

2012/10/7 Selena Deckelmann <selena@chesnok.com>:

Hello

a some updated version

* possibility to raise (and filter) performance warnings - detects IO castings
* detects assign composite value to scalar variable

Regards

Pavel Stehule

2012/11/27 Pavel Stehule <pavel.stehule@gmail.com>:

-----Original Message-----
From: pgsql-hackers-owner@postgresql.org [mailto:pgsql-hackers-
owner@postgresql.org] On Behalf Of Pavel Stehule
Sent: 28 November 2012 11:07
To: Selena Deckelmann
Cc: PostgreSQL Hackers
Subject: Re: [HACKERS] plpgsql_check_function - rebase for 9.3

Hello

a some updated version

* possibility to raise (and filter) performance warnings - detects IO castings
* detects assign composite value to scalar variable

Hello,

I was wondering if maybe this could be split to 2 separate patches, one would be all the changes to the existing plpgsql code - rename delete_function to plpgsql_delete_function and extern plpgsql_delete_function, copy_plpgsql_datum, plpgsql_estate_setup, plpgsql_destroy_econtext and the other patch would be the actual checker.

Reasoning for this is that the first patch (exporting some of plpgsql internals) should be very safe to commit and would be useful by itself even if the checker does not get in 9.3 since it would enable users to write lints/checkers/analysers for plpgsql as standalone extensions (for my use case this is actually way more useful than having the checker as part of core).

Regards
Petr

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

"Petr Jelinek" <pjmodos@pjmodos.net> writes:

I was wondering if maybe this could be split to 2 separate patches, one would be all the changes to the existing plpgsql code - rename delete_function to plpgsql_delete_function and extern plpgsql_delete_function, copy_plpgsql_datum, plpgsql_estate_setup, plpgsql_destroy_econtext and the other patch would be the actual checker.

Reasoning for this is that the first patch (exporting some of plpgsql internals) should be very safe to commit and would be useful by itself even if the checker does not get in 9.3 since it would enable users to write lints/checkers/analysers for plpgsql as standalone extensions (for my use case this is actually way more useful than having the checker as part of core).

What exactly do you have in mind there? The way we load extensions,
they can't (AFAIK) see each other's defined symbols, so you couldn't
really make an independent extension that would call functions in
plpgsql. This is not really open for debate, either, as changing that
would risk creating symbol collisions between modules that never had to
worry about polluting global namespace before.

I would note also that we absolutely, positively do not guarantee not
to change plpgsql's private data structures in minor releases.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

-----Original Message-----
From: pgsql-hackers-owner@postgresql.org [mailto:pgsql-hackers-
owner@postgresql.org] On Behalf Of Tom Lane
Sent: 26 January 2013 18:16
Subject: Re: [HACKERS] plpgsql_check_function - rebase for 9.3

"Petr Jelinek" <pjmodos@pjmodos.net> writes:

I was wondering if maybe this could be split to 2 separate patches, one

would be all the changes to the existing plpgsql code - rename
delete_function to plpgsql_delete_function and extern
plpgsql_delete_function, copy_plpgsql_datum, plpgsql_estate_setup,
plpgsql_destroy_econtext and the other patch would be the actual checker.

Reasoning for this is that the first patch (exporting some of plpgsql

internals) should be very safe to commit and would be useful by itself

even if

the checker does not get in 9.3 since it would enable users to write
lints/checkers/analysers for plpgsql as standalone extensions (for my use
case this is actually way more useful than having the checker as part of

core).

What exactly do you have in mind there? The way we load extensions, they
can't (AFAIK) see each other's defined symbols, so you couldn't really

make

an independent extension that would call functions in plpgsql. This is

not

really open for debate, either, as changing that would risk creating

symbol

collisions between modules that never had to worry about polluting global
namespace before.

I can call functions that are exported by plpgsql.so just fine from
different extension now, I just have to preload the plpgsql.so (via LOAD or
guc) first, so I don't see what is the problem here.

I would note also that we absolutely, positively do not guarantee not to
change plpgsql's private data structures in minor releases.

That didn't stop people from from writing plpgsql extensions before, don't
see why it would now, the problem is that they have to use hooks now and
those require the function to be executed, making those plpgsql interfaces
external would help with setting up things so that extension can work with
function without function being executed or duplicating a lot of plpgsql
code. As an example of all of this you can see
https://github.com/okbob/plpgsql_lint which is the original module Pavel
wrote before writing this patch.

The other thing is this is something the patch in current form does anyway
so I don't see the harm.

Regards
Petr

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2013/1/26 Petr Jelinek <pjmodos@pjmodos.net>:

-----Original Message-----
From: pgsql-hackers-owner@postgresql.org [mailto:pgsql-hackers-
owner@postgresql.org] On Behalf Of Tom Lane
Sent: 26 January 2013 18:16
Subject: Re: [HACKERS] plpgsql_check_function - rebase for 9.3

"Petr Jelinek" <pjmodos@pjmodos.net> writes:

I was wondering if maybe this could be split to 2 separate patches, one

would be all the changes to the existing plpgsql code - rename
delete_function to plpgsql_delete_function and extern
plpgsql_delete_function, copy_plpgsql_datum, plpgsql_estate_setup,
plpgsql_destroy_econtext and the other patch would be the actual checker.

Reasoning for this is that the first patch (exporting some of plpgsql

internals) should be very safe to commit and would be useful by itself

even if

the checker does not get in 9.3 since it would enable users to write
lints/checkers/analysers for plpgsql as standalone extensions (for my use
case this is actually way more useful than having the checker as part of

core).

A significant improvement in this are can be placing plpgsql.h to
other header files. Now plpgsql extensions have to use private copy of
this file - what is really ugly

Pavel

What exactly do you have in mind there? The way we load extensions, they
can't (AFAIK) see each other's defined symbols, so you couldn't really

make

an independent extension that would call functions in plpgsql. This is

not

really open for debate, either, as changing that would risk creating

symbol

collisions between modules that never had to worry about polluting global
namespace before.

I can call functions that are exported by plpgsql.so just fine from
different extension now, I just have to preload the plpgsql.so (via LOAD or
guc) first, so I don't see what is the problem here.

I would note also that we absolutely, positively do not guarantee not to
change plpgsql's private data structures in minor releases.

That didn't stop people from from writing plpgsql extensions before, don't
see why it would now, the problem is that they have to use hooks now and
those require the function to be executed, making those plpgsql interfaces
external would help with setting up things so that extension can work with
function without function being executed or duplicating a lot of plpgsql
code. As an example of all of this you can see
https://github.com/okbob/plpgsql_lint which is the original module Pavel
wrote before writing this patch.

The other thing is this is something the patch in current form does anyway
so I don't see the harm.

Regards
Petr

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

"Petr Jelinek" <pjmodos@pjmodos.net> writes:

What exactly do you have in mind there? The way we load extensions, they
can't (AFAIK) see each other's defined symbols, so you couldn't really make
an independent extension that would call functions in plpgsql. This is not
really open for debate, either, as changing that would risk creating symbol
collisions between modules that never had to worry about polluting global
namespace before.

I can call functions that are exported by plpgsql.so just fine from
different extension now, I just have to preload the plpgsql.so (via LOAD or
guc) first, so I don't see what is the problem here.

[ pokes around... ] Hm, it appears that that does work on Linux,
because for some reason we're specifying RTLD_GLOBAL to dlopen().
TBH that seems like a truly horrid idea that we should reconsider.
Aside from the danger of unexpected symbol collisions between
independent loadable modules, I seriously doubt that it works like
that on every platform we support --- so I'd be very strongly against
accepting any code that depends on this working.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

I wrote:

[ pokes around... ] Hm, it appears that that does work on Linux,
because for some reason we're specifying RTLD_GLOBAL to dlopen().
TBH that seems like a truly horrid idea that we should reconsider.

A bit of research in the archives revealed that we're using it because
back in 2001, the lame hack that then passed for a shared-library
version of python required it:
/messages/by-id/Pine.LNX.4.30.0105121914200.757-100000@peter.localdomain

There was subsequent discussion of removing it, because reportedly now
(a) that's no longer the case, and (b) we need to get rid of it to allow
plpython2 and plpython3 to coexist in one session. See for instance:
/messages/by-id/1277506674.5356.27.camel@vanquo.pezone.net

Nothing's been done about that yet, but I think that assuming that we'll
be using RTLD_GLOBAL forever would be foolish.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

-----Original Message-----
From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
Sent: 26 January 2013 20:12
Subject: Re: [HACKERS] plpgsql_check_function - rebase for 9.3

I wrote:

[ pokes around... ] Hm, it appears that that does work on Linux,
because for some reason we're specifying RTLD_GLOBAL to dlopen().
TBH that seems like a truly horrid idea that we should reconsider.

A bit of research in the archives revealed that we're using it because

back in

2001, the lame hack that then passed for a shared-library version of

python

required it:
/messages/by-id/Pine.LNX.4.30.0105121914200.757-
100000@peter.localdomain

There was subsequent discussion of removing it, because reportedly now
(a) that's no longer the case, and (b) we need to get rid of it to allow
plpython2 and plpython3 to coexist in one session. See for instance:
http://www.postgresql.org/message-
id/1277506674.5356.27.camel@vanquo.pezone.net

Nothing's been done about that yet, but I think that assuming that we'll

be

using RTLD_GLOBAL forever would be foolish.

Ok then it was a bad idea after all.

Regards
Petr

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 1/26/13 1:53 PM, Tom Lane wrote:

[ pokes around... ] Hm, it appears that that does work on Linux,
because for some reason we're specifying RTLD_GLOBAL to dlopen().
TBH that seems like a truly horrid idea that we should reconsider.
Aside from the danger of unexpected symbol collisions between
independent loadable modules, I seriously doubt that it works like
that on every platform we support --- so I'd be very strongly against
accepting any code that depends on this working.

Well, that would kill a lot of potentially useful features, including
the transforms feature I've been working on and any kind of hook or
debugger or profiler on an existing module. (How do plpgsql plugins
work?) We also couldn't transparently move functionality out of the
postgres binary into a module.

I see the concern about symbol collisions. But you can normally work
around that by prefixing exported symbols.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 01/28/2013 10:11 AM, Peter Eisentraut wrote:

On 1/26/13 1:53 PM, Tom Lane wrote:

[ pokes around... ] Hm, it appears that that does work on Linux,
because for some reason we're specifying RTLD_GLOBAL to dlopen().
TBH that seems like a truly horrid idea that we should reconsider.
Aside from the danger of unexpected symbol collisions between
independent loadable modules, I seriously doubt that it works like
that on every platform we support --- so I'd be very strongly against
accepting any code that depends on this working.

Well, that would kill a lot of potentially useful features, including
the transforms feature I've been working on and any kind of hook or
debugger or profiler on an existing module. (How do plpgsql plugins
work?) We also couldn't transparently move functionality out of the
postgres binary into a module.

I see the concern about symbol collisions. But you can normally work
around that by prefixing exported symbols.

Yeah, I was just writing something a couple of days ago that leveraged
stuff in an extension, so it looks like this is wanted functionality. In
general we want to be able to layer addon modules, ISTM.

cheers

andrew

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hello

I though about any possibility how to reduce a size of this patch.

I see a one solution - if we would not support a detection of multiple
errors - it stops on first error, we can push this code to compilation
stage.

a plpgsql_validator can be overloaded by function

plpgsql_validator(oid, reloid, level)

reloid - is requested by triggers - it is related class oid
level - it is level of checking

0 - same as current implementation - check only syntax errors
1 - stop on first object error - no table, no field, no expected data type
2 - stop on first performance issue - implicit casting identification
(should be removed or moved to next releases)

This proposal reduces functionality proposed for
plpgsql_check_function - but basic functionality is there.

It has not a impact on performance and it allow check all paths in
compile time.

It will not change behave of current CREATE OR REPLACE function -
there is not a problem with back compatibility

It is good for you?

I can have this modification to end of this week.

Regards

Pavel

2012/11/28 Pavel Stehule <pavel.stehule@gmail.com>:

Hello

a some updated version

* possibility to raise (and filter) performance warnings - detects IO castings
* detects assign composite value to scalar variable

Regards

Pavel Stehule

2012/11/27 Pavel Stehule <pavel.stehule@gmail.com>:

Hello

I am sending a updated version - now it is prepared for event triggers
and it is little bit more robust

I run pgindent, but I have no experience with it, so I am not sure about success

Regards

Pavel Stehule

2012/10/7 Selena Deckelmann <selena@chesnok.com>:

Hi!

On Tue, Jun 26, 2012 at 1:19 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

I am sending lightly refreshed patch for checking plpgsql functions..

I checked different implementation, but without success: a) enhancing
of SPI to some fake mode can has negative impact on application, and
patch was not clear, b) generic plpgsql walker doesn't save lines too.

I invite any ideas how to improve this patch

I reviewed this and did a clean up for bitrot and a little whitespace.
In particular, it needed to learn a little about event triggers.

This patch is a follow on from an earlier review thread I found:
http://archives.postgresql.org/message-id/D960CB61B694CF459DCFB4B0128514C2072DF447@exadv11.host.magwien.gv.at

I dug through that thread a bit, and I believe issues raised by
Laurenz, Petr and Alvaro were resolved by Pavel over time.

All tests pass, and after a read-through, the code seems fine.

This also represents my inaugural use of pg_bsd_indent. I ran it on
pl_check.c - which made things mostly better. Happy to try and fix it
up more if someone can explain to me what (if anything) I did
incorrectly when using it.

-selena

--
http://chesnok.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Folks,

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.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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
/messages/by-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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hello all

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
/messages/by-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.

I wrote lot of versions and proposed code is redundant, but most
simple and clean.

I am really against to pushing check to pl_exec, because it
significantly decrease code readability and increase some bottle neck
in CPU extensive tests. More, there are too less place for some future
feature - performance advising, more verbose error messages, etc

In PL/pgPSM I used a little bit different architecture - necessary for
PSM and maybe better for PL/pgSQL too. It is two stage compiler -
parsing to AST, and AST compilation. It simplify gram.y and processing
order depends on AST deep iteration and not on bizon rules. It can
have a impact on speed of very large procedures probably - I don't see
different disadvantages. With this architecture I was able do lot of
controls to compile stage without problems.

Most complexity in current code is related to detecting record types
from expressions without expression evaluation. Maybe this code can be
in core or pl_compile.c. Code for Describe (F) message is not too
reusable. It is necessary for

DECLARE r RECORD;
FOR r IN SELECT ...
LOOP
RAISE NOTICE '>>%<<', r.xx;
END LOOP;

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 format can be reduced, redesigned, changed. It is designed like
gcc output and optimized for using from psql console.

I tested table output - in original CHECK statement implementation,
but it is not too friendly for showing in monitor - it is just too
wide. There are similar arguments like using tabular output for
EXPLAIN, although there are higher complexity and nested structures
(in EXPLAIN). Personally, I can live with tabular output, it is not
user friendly, but it can be used for machine processing simply, and
any advanced user can write some transform functions to any output
format.

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.

Yes. A details should be precised after final decision. I am hope so
this code doesn't contains significant bug - I have not any reported
issue related to this functionality - it is based on plpgsql_lint -
and this code is used in production by some very large companies. Of
course, there are not too large community - it should be manually
compiled.

Regards

Pavel

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/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>: