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

Re: BUG #1956: Plpgsql top-level DECLARE does not share scope

From
Bruce Momjian
Date:
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

Re: BUG #1956: Plpgsql top-level DECLARE does not share

From
"Karl O. Pinc"
Date:
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

Re: BUG #1956: Plpgsql top-level DECLARE does not share scope

From
Bruce Momjian
Date:
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

Re: BUG #1956: Plpgsql top-level DECLARE does not share scope

From
Tom Lane
Date:
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

Re: BUG #1956: Plpgsql top-level DECLARE does not share scope

From
"Jim C. Nasby"
Date:
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

Re: BUG #1956: Plpgsql top-level DECLARE does not share scope

From
Tom Lane
Date:
"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

Re: BUG #1956: Plpgsql top-level DECLARE does not share scope

From
"Jim C. Nasby"
Date:
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

Re: BUG #1956: Plpgsql top-level DECLARE does not share scope

From
Tom Lane
Date:
"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

Re: BUG #1956: Plpgsql top-level DECLARE does not share

From
"Karl O. Pinc"
Date:
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.

Re: BUG #1956: Plpgsql top-level DECLARE does not share

From
"Karl O. Pinc"
Date:
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

Re: BUG #1956: Plpgsql top-level DECLARE does not share

From
Michael Fuhr
Date:
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

Re: BUG #1956: Plpgsql top-level DECLARE does not share scope

From
"Jim C. Nasby"
Date:
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

Re: BUG #1956: Plpgsql top-level DECLARE does not share

From
"Karl O. Pinc"
Date:
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

Re: BUG #1956: Plpgsql top-level DECLARE does not share scope

From
Klint Gore
Date:
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.