Thread: BUG #1956: Plpgsql top-level DECLARE does not share scope with CREATE FUNCTION
BUG #1956: Plpgsql top-level DECLARE does not share scope with CREATE FUNCTION
From
"Karl O. Pinc"
Date:
The following bug has been logged online: Bug reference: 1956 Logged by: Karl O. Pinc Email address: kop@meme.com PostgreSQL version: 8.0.3 Operating system: Linux Description: Plpgsql top-level DECLARE does not share scope with CREATE FUNCTION Details: Depending on how you want to look at it, this is not really a bug. It does produce odd results though. The ALIASes for function parameters that get created as part of the plpgsql CREATE FUNCTION do not share scope (namespace?) with the top-level DECLARE of the function. As a result, you inadvertently make your function arguments 'disappear' by "re-declaring" them in the top-level DECLARE. I haven't put any deep thought into this, but offhand this 'feature' seems to have no utility but does have the ability to cause problems. Hence, this report. It would be nice if somebody who has deep thoughts on this would think them. I would expect the below to produce an error when creating foo(int). The error would complain about trying to declare the same thing with two different types. Instead we see the result below. create function foo (arg int) returns int language plpgsql as $$ declare arg text; begin return bar(arg); end; $$; create function bar (arg int) returns int language plpgsql as $$ begin return arg + 1; end; $$; babase=# select foo(1); ERROR: function bar(text) does not exist HINT: No function matches the given name and argument types. You may need to add explicit type casts. CONTEXT: SQL statement "SELECT bar( $1 )" PL/pgSQL function "foo" line 4 at return Regards, Karl O. Pinc
Interesting. If I define this in C:
int x(int y)
{
char *y;
}
I get a warning:
x.c: In function `x':
x.c:3: warning: declaration of `y' shadows a parameter
but no error. We tend to follow the C conventions, so perhaps we should
throw a warning, but I can't think of any cases where we throw a warning
in plpgsql because we compile it once on first call.
I am thinking this falls in the "don't do that" category.
---------------------------------------------------------------------------
Karl O. Pinc wrote:
>
> The following bug has been logged online:
>
> Bug reference: 1956
> Logged by: Karl O. Pinc
> Email address: kop@meme.com
> PostgreSQL version: 8.0.3
> Operating system: Linux
> Description: Plpgsql top-level DECLARE does not share scope with
> CREATE FUNCTION
> Details:
>
> Depending on how you want to look at it, this is not really a bug. It does
> produce odd results though.
>
> The ALIASes for function parameters that get created as part of the plpgsql
> CREATE FUNCTION do not share scope (namespace?) with the top-level DECLARE
> of the function. As a result, you inadvertently make your function
> arguments 'disappear' by "re-declaring" them in the top-level DECLARE. I
> haven't put any deep thought into this, but offhand this 'feature' seems to
> have no utility but does have the ability to cause problems. Hence, this
> report.
>
> It would be nice if somebody who has deep thoughts on this would think
> them.
>
> I would expect the below to produce an error when creating foo(int). The
> error would complain about trying to declare the same thing with two
> different types. Instead we see the result below.
>
> create function foo (arg int)
> returns int
> language plpgsql
> as $$
> declare
> arg text;
> begin
> return bar(arg);
> end;
> $$;
>
> create function bar (arg int)
> returns int
> language plpgsql
> as $$
> begin
> return arg + 1;
> end;
> $$;
>
> babase=# select foo(1);
> ERROR: function bar(text) does not exist
> HINT: No function matches the given name and argument types. You may need
> to add explicit type casts.
> CONTEXT: SQL statement "SELECT bar( $1 )"
> PL/pgSQL function "foo" line 4 at return
>
> Regards,
> Karl O. Pinc
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo@postgresql.org so that your
> message can get through to the mailing list cleanly
>
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
On 10/12/2005 10:32:20 PM, Bruce Momjian wrote:
We tend to follow the C conventions, so perhaps we
> should
> throw a warning, but I can't think of any cases where we throw a
> warning
> in plpgsql because we compile it once on first call.
>
> I am thinking this falls in the "don't do that" category.
Fair enough. At the same time it sure would be nice if
plpgsql actually compiled (and parsed SQL) at
function definition time, even when the result is thrown away.
I'm building a big system and it's quite annoying
to get syntax errors, IIRC,
in code months after writing it, just because it took
me that long to get around to exercising a particular
IF statement.
Karl <kop@meme.com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein
Karl O. Pinc wrote:
>
> On 10/12/2005 10:32:20 PM, Bruce Momjian wrote:
> We tend to follow the C conventions, so perhaps we
> > should
> > throw a warning, but I can't think of any cases where we throw a
> > warning
> > in plpgsql because we compile it once on first call.
> >
> > I am thinking this falls in the "don't do that" category.
>
> Fair enough. At the same time it sure would be nice if
> plpgsql actually compiled (and parsed SQL) at
> function definition time, even when the result is thrown away.
> I'm building a big system and it's quite annoying
> to get syntax errors, IIRC,
> in code months after writing it, just because it took
> me that long to get around to exercising a particular
> IF statement.
8.0 has this improvement:
* Do minimal syntax checking of PL/pgSQL functions at creation time (Tom)
This allows us to catch simple syntax errors sooner.
I assume you are running an earlier version of PostgreSQL.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Karl O. Pinc wrote:
>> Fair enough. At the same time it sure would be nice if
>> plpgsql actually compiled (and parsed SQL) at
>> function definition time, even when the result is thrown away.
> 8.0 has this improvement:
> * Do minimal syntax checking of PL/pgSQL functions at creation time (Tom)
> This allows us to catch simple syntax errors sooner.
8.1 does more in that line (thanks to Neil).
regards, tom lane
On Thu, Oct 13, 2005 at 10:38:36AM -0400, Bruce Momjian wrote: > Karl O. Pinc wrote: > > > > On 10/12/2005 10:32:20 PM, Bruce Momjian wrote: > > We tend to follow the C conventions, so perhaps we > > > should > > > throw a warning, but I can't think of any cases where we throw a > > > warning > > > in plpgsql because we compile it once on first call. > > > > > > I am thinking this falls in the "don't do that" category. > > > > Fair enough. At the same time it sure would be nice if > > plpgsql actually compiled (and parsed SQL) at > > function definition time, even when the result is thrown away. > > I'm building a big system and it's quite annoying > > to get syntax errors, IIRC, > > in code months after writing it, just because it took > > me that long to get around to exercising a particular > > IF statement. > > 8.0 has this improvement: > > * Do minimal syntax checking of PL/pgSQL functions at creation time (Tom) > This allows us to catch simple syntax errors sooner. Based on all that, I would certainly be in favor of throwing a warning if you over-define something, since 99% of the time it's a mistake. Is that possible with the current checking we do at compile time? -- Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com Pervasive Software http://pervasive.com work: 512-231-6117 vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
"Jim C. Nasby" <jnasby@pervasive.com> writes:
> Based on all that, I would certainly be in favor of throwing a warning
> if you over-define something, since 99% of the time it's a mistake. Is
> that possible with the current checking we do at compile time?
Without having looked at the code, I imagine the problem is that we
can't tell this situation from an ordinary nested DECLARE block,
that is
declare x int;
begin
...
declare x float;
begin
...
The above is legal code and I don't think we should throw a warning for
it.
Basically, DECLARE introduces a new name scope that wouldn't be there
if you didn't say DECLARE. Without some bizarre reinterpretation of the
meaning of a DECLARE at the start of a function, variables automatically
created by plpgsql are going to be in an outer scope surrounding that of
the first DECLARE.
regards, tom lane
On Thu, Oct 13, 2005 at 01:30:56PM -0400, Tom Lane wrote: > "Jim C. Nasby" <jnasby@pervasive.com> writes: > > Based on all that, I would certainly be in favor of throwing a warning > > if you over-define something, since 99% of the time it's a mistake. Is > > that possible with the current checking we do at compile time? > > Without having looked at the code, I imagine the problem is that we > can't tell this situation from an ordinary nested DECLARE block, > that is > > declare x int; > begin > ... > declare x float; > begin > ... > > The above is legal code and I don't think we should throw a warning for > it. > > Basically, DECLARE introduces a new name scope that wouldn't be there > if you didn't say DECLARE. Without some bizarre reinterpretation of the > meaning of a DECLARE at the start of a function, variables automatically > created by plpgsql are going to be in an outer scope surrounding that of > the first DECLARE. Yeah, I agree that in the legitimate case it makes much less sense to throw an error. Are blocks that aren't explicitely labled assigned a machine-generated label? If so then it should be possible to tell if something is in the outer-most block or if it's part of the function declaration itself. But I have no idea how difficult it would be to do that. Another possibility is tracking what level sub-block something is in, and using that to determine if the top-most declare in a function is over-writing something. -- Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com Pervasive Software http://pervasive.com work: 512-231-6117 vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
"Jim C. Nasby" <jnasby@pervasive.com> writes:
> On Thu, Oct 13, 2005 at 01:30:56PM -0400, Tom Lane wrote:
>> Basically, DECLARE introduces a new name scope that wouldn't be there
>> if you didn't say DECLARE. Without some bizarre reinterpretation of the
>> meaning of a DECLARE at the start of a function, variables automatically
>> created by plpgsql are going to be in an outer scope surrounding that of
>> the first DECLARE.
> Another possibility is tracking what level sub-block something is in,
> and using that to determine if the top-most declare in a function is
> over-writing something.
BTW, another issue here is that if we did merge the first DECLARE with
the scope of auto-declared variables, it would be a non backwards
compatible change. Right now you can do, say,
declare found int;
and it'll override the standard FOUND variable. If we change this then
you'd get an error. (Of course, it could be argued that that would be
a Good Thing. But it would inhibit us from adding new auto-declared
variables that are less central to the language than FOUND, because of
the risk of breaking existing code.)
regards, tom lane
On 10/13/2005 03:24:23 PM, Tom Lane wrote:
> "Jim C. Nasby" <jnasby@pervasive.com> writes:
> > On Thu, Oct 13, 2005 at 01:30:56PM -0400, Tom Lane wrote:
> >> Basically, DECLARE introduces a new name scope that wouldn't be
> there
> >> if you didn't say DECLARE. Without some bizarre reinterpretation
> of the
> >> meaning of a DECLARE at the start of a function, variables
> automatically
> >> created by plpgsql are going to be in an outer scope surrounding
> that of
> >> the first DECLARE.
>
> BTW, another issue here is that if we did merge the first DECLARE with
> the scope of auto-declared variables, it would be a non backwards
> compatible change.
How about merging only the auto-declared function arguments into the
first DECLARE? Down side would be that I can see that causing
an actual error instead of a warning if somebody wants to shadow
one of the function arguments with their own declaration.
Karl <kop@meme.com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein
P.S.
Seems that as of today I can no longer write Bruce Momjian
<pgman@candle.pha.pa.us>. :-( This showed up in a bounce:
--------<snip>---------
Hi. This is the qmail-send program at mail27.sea5.speakeasy.net.
<snip>
<pgman@candle.pha.pa.us>:
64.139.89.126 does not like recipient.
Remote host said: 550 5.0.0 <pgman@candle.pha.pa.us>... Delivery
blocked ---
Previous SPAM received from your mail server
Giving up on 64.139.89.126.
On 10/13/2005 09:38:36 AM, Bruce Momjian wrote:
> > Fair enough. At the same time it sure would be nice if
> > plpgsql actually compiled (and parsed SQL) at
> > function definition time, even when the result is thrown away.
> > I'm building a big system and it's quite annoying
> > to get syntax errors, IIRC,
> > in code months after writing it, just because it took
> > me that long to get around to exercising a particular
> > IF statement.
>
> 8.0 has this improvement:
>
> * Do minimal syntax checking of PL/pgSQL functions at creation
> time (Tom)
> This allows us to catch simple syntax errors sooner.
>
> I assume you are running an earlier version of PostgreSQL.
I was, but switched to 8.0.2 and then 8.0.3 as soon as it came out.
Perhaps my problems are more to do with not-simple syntax errors,
or I could be recalling my experience before 8.0.x. I definately
do not recall catching any additional errors at compile time as
part of the switch to 8. Call it paranoia, I'm still not
confident. More compile time checks are better!
Gimmie!
Mine!
*Ahem*...
Ok. Thanks for the reply. I'm done with the bulk of the code
at this point but I'll keep a lookout for cases where
syntax/datatype errors crop up out of the blue.
Karl <kop@meme.com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein
On Thu, Oct 13, 2005 at 03:51:15PM +0000, Karl O. Pinc wrote: > I definately do not recall catching any additional errors at > compile time as part of the switch to 8. 8.0's syntax checking is minimal; 8.1's will be better. Also, you might not even have plpgsql's lanvalidator function if you restored from an earlier version. What's the result of the following query? SELECT * FROM pg_language WHERE lanname = 'plpgsql'; If lanvalidator is 0 then you won't get even the minimal syntax checks. 8.1 will avoid this problem by creating languages based on entries in a template table (pg_pltemplate). -- Michael Fuhr
On Thu, Oct 13, 2005 at 04:24:23PM -0400, Tom Lane wrote: > "Jim C. Nasby" <jnasby@pervasive.com> writes: > > On Thu, Oct 13, 2005 at 01:30:56PM -0400, Tom Lane wrote: > >> Basically, DECLARE introduces a new name scope that wouldn't be there > >> if you didn't say DECLARE. Without some bizarre reinterpretation of the > >> meaning of a DECLARE at the start of a function, variables automatically > >> created by plpgsql are going to be in an outer scope surrounding that of > >> the first DECLARE. > > > Another possibility is tracking what level sub-block something is in, > > and using that to determine if the top-most declare in a function is > > over-writing something. > > BTW, another issue here is that if we did merge the first DECLARE with > the scope of auto-declared variables, it would be a non backwards > compatible change. Right now you can do, say, I wasn't suggesting that we actually merge the scopes; is it possible to detect over-writing a variable without merging them? > declare found int; > > and it'll override the standard FOUND variable. If we change this then > you'd get an error. (Of course, it could be argued that that would be > a Good Thing. But it would inhibit us from adding new auto-declared > variables that are less central to the language than FOUND, because of > the risk of breaking existing code.) I thought we were only talking about throwing a warning, not an error. I don't think it makes sense to throw an error for any of these cases, because it could well be what the user wants, but it would be nice to warn them since there's a good chance it's a mistake (especially in the top-level). -- Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com Pervasive Software http://pervasive.com work: 512-231-6117 vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
On 10/14/2005 09:30:51 AM, Michael Fuhr wrote:
>> On Thu, Oct 13, 2005 at 03:51:15PM +0000, Karl O. Pinc wrote:
> > I definately do not recall catching any additional errors at
> > compile time as part of the switch to 8.
> 8.0's syntax checking is minimal; 8.1's will be better.
Looking forward to it!
Also, you
> might not even have plpgsql's lanvalidator function if you restored
> from an earlier version. What's the result of the following query?
>
> SELECT * FROM pg_language WHERE lanname = 'plpgsql';
>
> If lanvalidator is 0 then you won't get even the minimal syntax
> checks. 8.1 will avoid this problem by creating languages based
> on entries in a template table (pg_pltemplate).
babase=# SELECT * FROM pg_language WHERE lanname = 'plpgsql';
lanname | lanispl | lanpltrusted | lanplcallfoid | lanvalidator |
lanacl
---------+---------+--------------+---------------+--------------+--------
plpgsql | t | t | 17239 | 17240 |
(1 row)
So I've got it.
IIRC I didn't have anything but test data at the time I upgraded
to 8.0 and so re-created all my databases.
Karl <kop@meme.com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein
On Thu, 13 Oct 2005 16:24:23 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote: > BTW, another issue here is that if we did merge the first DECLARE with > the scope of auto-declared variables, it would be a non backwards > compatible change. Right now you can do, say, > > declare found int; > > and it'll override the standard FOUND variable. If we change this then > you'd get an error. (Of course, it could be argued that that would be > a Good Thing. But it would inhibit us from adding new auto-declared > variables that are less central to the language than FOUND, because of > the risk of breaking existing code.) Could something be done using alias? eg declare x int; ... declare x alias for outer x klint.