Thread: Clobbered parameter names via DECLARE in PL/PgSQL
Hello hackers, It turns out that in a PL/PgSQL function, you can DECLARE a variable using the same name as one of the function parameters. This has the effect of clobbering the parameter, for example: CREATE OR REPLACE FUNCTION declare_clobber(foo int) RETURNS int LANGUAGE plpgsql AS $$DECLARE foo text;BEGIN RETURN foo;END; $$; SELECT declare_clobber(1); ==> NULL On the other hand, PL/PgSQL does protect against duplicate definitions within DECLARE: CREATE OR REPLACE FUNCTION declare_clobber(foo int) RETURNS int LANGUAGE plpgsql AS $$DECLARE foo int; foo text;BEGIN RETURN foo;END; $$; ==> ERROR: duplicate declaration at or near "foo" And it also protects against using a DECLAREd name as a parameter alias: CREATE OR REPLACE FUNCTION declare_clobber(foo int) RETURNS int LANGUAGE plpgsql AS $$DECLARE bar int; bar ALIAS FOR $1;BEGIN RETURN bar;END; $$; ==> ERROR: duplicate declaration at or near "bar" I would suggest that if the user DECLAREs a variable with the same name as a parameter, it is very evidently a programming error, and we should raise the same "duplicate declaration" error. I haven't yet looked at how difficult this would be to fix, but if there are no objections I would like to attempt a patch. Cheers, BJ
2012/4/15 Brendan Jurd <direvus@gmail.com>: > Hello hackers, > > It turns out that in a PL/PgSQL function, you can DECLARE a variable > using the same name as one of the function parameters. This has the > effect of clobbering the parameter, for example: > > CREATE OR REPLACE FUNCTION declare_clobber(foo int) > RETURNS int LANGUAGE plpgsql AS $$ > DECLARE > foo text; > BEGIN > RETURN foo; > END; > $$; > > SELECT declare_clobber(1); > ==> NULL > > On the other hand, PL/PgSQL does protect against duplicate definitions > within DECLARE: > > CREATE OR REPLACE FUNCTION declare_clobber(foo int) > RETURNS int LANGUAGE plpgsql AS $$ > DECLARE > foo int; > foo text; > BEGIN > RETURN foo; > END; > $$; > ==> ERROR: duplicate declaration at or near "foo" > > And it also protects against using a DECLAREd name as a parameter alias: > > CREATE OR REPLACE FUNCTION declare_clobber(foo int) > RETURNS int LANGUAGE plpgsql AS $$ > DECLARE > bar int; > bar ALIAS FOR $1; > BEGIN > RETURN bar; > END; > $$; > ==> ERROR: duplicate declaration at or near "bar" > > I would suggest that if the user DECLAREs a variable with the same > name as a parameter, it is very evidently a programming error, and we > should raise the same "duplicate declaration" error. I haven't yet > looked at how difficult this would be to fix, but if there are no > objections I would like to attempt a patch. > I disagree - variables and parameters are in different namespace so you can exactly identify variable and parameter. More - it is compatibility break. If plpgsql_check_function exists, then this check can be implemented as warning. Regards Pavel > Cheers, > BJ > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers
On 15 April 2012 17:55, Pavel Stehule <pavel.stehule@gmail.com> wrote: > 2012/4/15 Brendan Jurd <direvus@gmail.com>: >> It turns out that in a PL/PgSQL function, you can DECLARE a variable >> using the same name as one of the function parameters. This has the >> effect of clobbering the parameter, for example: >> ... >> >> I would suggest that if the user DECLAREs a variable with the same >> name as a parameter, it is very evidently a programming error, and we >> should raise the same "duplicate declaration" error. I haven't yet >> looked at how difficult this would be to fix, but if there are no >> objections I would like to attempt a patch. >> > > I disagree - variables and parameters are in different namespace so > you can exactly identify variable and parameter. More - it is > compatibility break. > They may technically be in different namespaces, but the fact that the declared variable quietly goes ahead and masks the parameter locally, seems like a recipe for unexpected consequences. It certainly was in my case, and I doubt I'm the first or the last to make that mistake. Under these conditions, you now have "foo" which refers to the variable, and "declare_clobber.foo" which refers to the parameter. Not exactly a model of clarity, and it's also quite easy to miss the part of the PL/PgSQL docs mentioning this notation. Perhaps it's a failure of imagination on my part, but I can't think of a legitimate reason for a programmer to deliberately use the same name to refer to a declared variable and a function parameter. What would be the benefit? Cheers, BJ
2012/4/15 Brendan Jurd <direvus@gmail.com>: > On 15 April 2012 17:55, Pavel Stehule <pavel.stehule@gmail.com> wrote: >> 2012/4/15 Brendan Jurd <direvus@gmail.com>: >>> It turns out that in a PL/PgSQL function, you can DECLARE a variable >>> using the same name as one of the function parameters. This has the >>> effect of clobbering the parameter, for example: >>> > ... >>> >>> I would suggest that if the user DECLAREs a variable with the same >>> name as a parameter, it is very evidently a programming error, and we >>> should raise the same "duplicate declaration" error. I haven't yet >>> looked at how difficult this would be to fix, but if there are no >>> objections I would like to attempt a patch. >>> >> >> I disagree - variables and parameters are in different namespace so >> you can exactly identify variable and parameter. More - it is >> compatibility break. >> > > They may technically be in different namespaces, but the fact that the > declared variable quietly goes ahead and masks the parameter locally, > seems like a recipe for unexpected consequences. It certainly was in > my case, and I doubt I'm the first or the last to make that mistake. > I agree so this issue is relative usual. But I don't think so we can use too hard solution like exclusion of parameter names, because it just has not support in standard or PL/SQL or SQL/PSM. And we introduce a few years ago different solution - function's namespace. > Under these conditions, you now have "foo" which refers to the > variable, and "declare_clobber.foo" which refers to the parameter. > Not exactly a model of clarity, and it's also quite easy to miss the > part of the PL/PgSQL docs mentioning this notation. it is not well documented and should be documented better: http://www.postgresql.org/docs/9.1/static/plpgsql-structure.html 39.2. Structure of PL/pgSQL Note: There is actually a hidden "outer block" surrounding the body of any PL/pgSQL function. This block provides the declarations of the function's parameters (if any), as well as some special variables such as FOUND (see Section 39.5.5). The outer block is labeled with the function's name, meaning that parameters and special variables can be qualified with the function's name. "39.3.1. Declaring Function Parameters Note: These two examples are not perfectly equivalent. In the first case, subtotal could be referenced as sales_tax.subtotal, but in the second case it could not. (Had we attached a label to the inner block, subtotal could be qualified with that label, instead. It should be documented better." > > Perhaps it's a failure of imagination on my part, but I can't think of > a legitimate reason for a programmer to deliberately use the same name > to refer to a declared variable and a function parameter. What would > be the benefit? it depends on level of nesting blocks. For simple functions there parameter redeclaration is clean bug, but for more nested blocks and complex procedures, there should be interesting using some local variables with same identifier like some parameters and blocking parameter's identifier can be same unfriendly feature like RO parameters in previous pg versions. I understand your motivation well, but solution should be warning, not blocking. I think. Regards Pavel > > Cheers, > BJ
On 15 April 2012 18:54, Pavel Stehule <pavel.stehule@gmail.com> wrote: > 2012/4/15 Brendan Jurd <direvus@gmail.com>: >> Perhaps it's a failure of imagination on my part, but I can't think of >> a legitimate reason for a programmer to deliberately use the same name >> to refer to a declared variable and a function parameter. What would >> be the benefit? > > it depends on level of nesting blocks. For simple functions there > parameter redeclaration is clean bug, but for more nested blocks and > complex procedures, there should be interesting using some local > variables with same identifier like some parameters and blocking > parameter's identifier can be same unfriendly feature like RO > parameters in previous pg versions. > > I understand your motivation well, but solution should be warning, not > blocking. I think. I can accept that ... but I wonder about the implementation of such a warning. Can we raise a WARNING message on CREATE [OR REPLACE] FUNCTION? If so, should there be a way to switch it off? If so, would this be implemented globally, or per-function? Would it be a postgres run-time setting, or an extension to CREATE FUNCTION syntax, or something within the PL/pgSQL code (like Perl's 'use strict')? Cheers, BJ
2012/4/15 Brendan Jurd <direvus@gmail.com>: > On 15 April 2012 18:54, Pavel Stehule <pavel.stehule@gmail.com> wrote: >> 2012/4/15 Brendan Jurd <direvus@gmail.com>: >>> Perhaps it's a failure of imagination on my part, but I can't think of >>> a legitimate reason for a programmer to deliberately use the same name >>> to refer to a declared variable and a function parameter. What would >>> be the benefit? >> >> it depends on level of nesting blocks. For simple functions there >> parameter redeclaration is clean bug, but for more nested blocks and >> complex procedures, there should be interesting using some local >> variables with same identifier like some parameters and blocking >> parameter's identifier can be same unfriendly feature like RO >> parameters in previous pg versions. >> >> I understand your motivation well, but solution should be warning, not >> blocking. I think. > > I can accept that ... but I wonder about the implementation of such a > warning. Can we raise a WARNING message on CREATE [OR REPLACE] > FUNCTION? If so, should there be a way to switch it off? If so, > would this be implemented globally, or per-function? Would it be a > postgres run-time setting, or an extension to CREATE FUNCTION syntax, > or something within the PL/pgSQL code (like Perl's 'use strict')? We can raise warning from CREATE OR REPLACE FUNCTION - but I would to like have plpgsql_check_function inside core - and it is better place for this and similar issues. Now we talk about features in 9.3, and there check_function should be. Regards Pavel > > Cheers, > BJ
On 04/15/2012 03:49 AM, Brendan Jurd wrote: > Hello hackers, > > It turns out that in a PL/PgSQL function, you can DECLARE a variable > using the same name as one of the function parameters. This has the > effect of clobbering the parameter, for example: > ... > > I would suggest that if the user DECLAREs a variable with the same > name as a parameter, it is very evidently a programming error, and we > should raise the same "duplicate declaration" error. I haven't yet > looked at how difficult this would be to fix, but if there are no > objections I would like to attempt a patch. This doesn't need fixing, IMNSHO. The name isn't "clobbered" and the partameter is accessible. The docs state: Any statement in the statement section of a block can be a subblock. Subblocks can be used for logical grouping or tolocalize variables to a small group of statements. Variables declared in a subblock mask any similarly-named variablesof outer blocks for the duration of the subblock; but you can access the outer variables anyway if you qualifytheir names with their block's label. ... There is actually a hidden "outer block" surrounding the body of any PL/pgSQL function. This block provides the declarations of the function's parameters (if any), as well as some specialvariables such as FOUND (see Section 39.5.5). The outer block is labeled with the function's name, meaning thatparameters and special variables can be qualified with the function's name. Note that you can label the outermost block of the function body, as the example in the docs shows. cheers andrew
Pavel Stehule <pavel.stehule@gmail.com> writes:
> We can raise warning from CREATE OR REPLACE FUNCTION - but I would to
> like have plpgsql_check_function inside core - and it is better place
> for this and similar issues.
I agree. This is a perfectly legal use of nested declaration scopes,
so it would be totally inappropriate to complain about it in normal
use of a plpgsql function. On the other hand, it would probably be
sane and useful for CHECK FUNCTION to flag any case where an inner
declaration shadows an outer-scope name (not only the specific case
of topmost block vs function parameter).
regards, tom lane
2012/4/15 Tom Lane <tgl@sss.pgh.pa.us>: > Pavel Stehule <pavel.stehule@gmail.com> writes: >> We can raise warning from CREATE OR REPLACE FUNCTION - but I would to >> like have plpgsql_check_function inside core - and it is better place >> for this and similar issues. > > I agree. This is a perfectly legal use of nested declaration scopes, > so it would be totally inappropriate to complain about it in normal > use of a plpgsql function. On the other hand, it would probably be > sane and useful for CHECK FUNCTION to flag any case where an inner > declaration shadows an outer-scope name (not only the specific case > of topmost block vs function parameter). yes, it is very simple check there. There should be "levels" of warnings in future and performance or semantic warnings. But, we don't need to increase complexity of CHECK FUNCTION now. A design of CHECK FUNCTION was rich for this purposes. And we need to find way to push plpgsql_check_function to core first. Regards Pavel > > regards, tom lane
Hello there is VIP patch of plpgsql_check_function that supports this warning Regards Pavel 2012/4/15 Pavel Stehule <pavel.stehule@gmail.com>: > 2012/4/15 Tom Lane <tgl@sss.pgh.pa.us>: >> Pavel Stehule <pavel.stehule@gmail.com> writes: >>> We can raise warning from CREATE OR REPLACE FUNCTION - but I would to >>> like have plpgsql_check_function inside core - and it is better place >>> for this and similar issues. >> >> I agree. This is a perfectly legal use of nested declaration scopes, >> so it would be totally inappropriate to complain about it in normal >> use of a plpgsql function. On the other hand, it would probably be >> sane and useful for CHECK FUNCTION to flag any case where an inner >> declaration shadows an outer-scope name (not only the specific case >> of topmost block vs function parameter). > > yes, it is very simple check there. There should be "levels" of > warnings in future and performance or semantic warnings. > > But, we don't need to increase complexity of CHECK FUNCTION now. A > design of CHECK FUNCTION was rich for this purposes. And we need to > find way to push plpgsql_check_function to core first. > > Regards > > Pavel > > > > > >> >> regards, tom lane