pl/pgsql feature request: shorthand for argument and local variable references

Hi

út 17. 11. 2020 v 19:04 odesílatel Jack Christensen <jack@jncsoftware.com>
napsal:

When arguments and other local variables in pl/pgsql functions have the
same name as columns referenced in queries it is necessary to disambiguate
the names. This can be done by prefixing the function name (e.g.
my_func.name), using the argument number is the case of an argument (e.g.
$1), or renaming the variable (e.g. _name). It is also possible to use a
GUC to always use the variable or the column but that seems dangerous to me.

Prefixing names with an underscore works well enough for local variables,
but when using named arguments I prefer the external name not require an
underscore. I would like to suggest a standard prefix such as $ to
reference a local variable or argument. $ followed by an integer already
references an argument by ordinal. What if $ followed by a name meant a
local reference (essentially it would expand to "my_func.")?

For example, currently I have to do something like this:

create function update_item(id int, foo int, bar text) returns void
language plpgsql as $$
begin
update items
set foo = update_item.foo,
bar = update_item.bar
where items.id = update_item.id;
end;
$$;

I would like to be able to do something like:

create function update_item(id int, foo int, bar text) returns void
language plpgsql as $$
begin
update items
set foo = $foo,
bar = $bar
where items.id = $id;
end;
$$;

Any opinions on the desirability of this feature? My C skills are rather
atrophied, but from the outside it seems like a small enough change I might
be able to tackle it...

I don't like this proposal too much. Introducing the next different syntax
for writing local variables doesn't look like a good idea for me. More this
syntax is used by very different languages than is PL/pgSQL, and then it
can be messy. The behaviour of local variables in PHP or Perl or shell is
really very different.

Personally in your example I very much like notation "update_item.id",
because there is a clean signal so "id" is the function's argument. When
you use "$id", then it is not clean if "id" is a local variable or
function's argument. So your proposal decreases safety :-/. Plus this
syntax reduces collision only on one side, you should use aliases for sql
identifiers and again it is not balanced - In MS SQL I can write predicate
id = @id. But it is not possible in your proposal (and it is not possible
from compatibility reasons ever).

More we already has a possibility to do ALIAS of any variable
https://www.postgresql.org/docs/current/plpgsql-declarations.html#PLPGSQL-DECLARATION-ALIAS

I understand that there can be problems with functions with very long
names.

We already can do

CREATE OR REPLACE FUNCTION public.fx(par1 integer, par2 integer)
RETURNS void
LANGUAGE plpgsql
AS $function$
<<b>>
declare
p1 alias for par1;
p2 alias for par2;
begin
raise notice '% % % %', par1, par2, b.p1, b.p2;
end;
$function$

or safer

CREATE OR REPLACE FUNCTION public.fx(par1 integer, par2 integer)
RETURNS void
LANGUAGE plpgsql
AS $function$
<<b>>
declare
p1 alias for fx.par1;
p2 alias for fx.par2;
begin
raise notice '% % % %', par1, par2, b.p1, b.p2;
end;
$function$

So I think introducing new syntax is not necessary. The open question is a
possibility to do aliasing more comfortably. ADA language has a possibility
to rename function or procedure. But it is much more stronger, than can be
implemented in plpgsql. Probably the most easy implementation can be a
possibility to specify a new argument's label with already supported
#option syntax.

CREATE OR REPLACE FUNCTION very_long_name(par1 int)
RETURNS int AS $$
#routine_label lnm
BEGIN
RAISE NOTICE '%', lnm.par1;

Regards

Pavel

On Tue, Nov 17, 2020 at 1:36 PM Pavel Stehule <pavel.stehule@gmail.com>
wrote:

Personally in your example I very much like notation "update_item.id",
because there is a clean signal so "id" is the function's argument. When
you use "$id", then it is not clean if "id" is a local variable or
function's argument. So your proposal decreases safety :-/. Plus this
syntax reduces collision only on one side, you should use aliases for sql
identifiers and again it is not balanced - In MS SQL I can write predicate
id = @id. But it is not possible in your proposal (and it is not possible
from compatibility reasons ever).

More we already has a possibility to do ALIAS of any variable
https://www.postgresql.org/docs/current/plpgsql-declarations.html#PLPGSQL-DECLARATION-ALIAS

I understand that there can be problems with functions with very long
names.

Right. The problem is most pronounced when the function name is long. And
in particular when there are a lot of optional arguments. In this case the
caller will be using named arguments so I want to avoid prefixing the
parameter names. And while alias is an option, that can also get quite
verbose when there are a large number of parameters.

So I think introducing new syntax is not necessary. The open question is a
possibility to do aliasing more comfortably. ADA language has a possibility
to rename function or procedure. But it is much more stronger, than can be
implemented in plpgsql. Probably the most easy implementation can be a
possibility to specify a new argument's label with already supported
#option syntax.

CREATE OR REPLACE FUNCTION very_long_name(par1 int)
RETURNS int AS $$
#routine_label lnm
BEGIN
RAISE NOTICE '%', lnm.par1;

I concur. This would be an improvement.

Jack

On 11/17/20 15:18, Jack Christensen wrote:

CREATE OR REPLACE FUNCTION very_long_name(par1 int)
RETURNS int AS $$
#routine_label lnm
BEGIN
RAISE NOTICE '%', lnm.par1;

Could that be somehow shoehorned into the existing ALIAS syntax, maybe as

DECLARE
lnm ALIAS FOR ALL very_long_name.*;

or something?

(And would it be cool if Table C.1 [1]https://www.postgresql.org/docs/13/sql-keywords-appendix.html#KEYWORDS-TABLE had some sort of javascript-y
filtering on reservedness categories, for just such kinds of bikeshedding?)

Regards,
-Chap

[1]: https://www.postgresql.org/docs/13/sql-keywords-appendix.html#KEYWORDS-TABLE

út 17. 11. 2020 v 21:45 odesílatel Chapman Flack <chap@anastigmatix.net>
napsal:

On 11/17/20 15:18, Jack Christensen wrote:

CREATE OR REPLACE FUNCTION very_long_name(par1 int)
RETURNS int AS $$
#routine_label lnm
BEGIN
RAISE NOTICE '%', lnm.par1;

Could that be somehow shoehorned into the existing ALIAS syntax, maybe as

DECLARE
lnm ALIAS FOR ALL very_long_name.*;

or something?

I thought about it - but I don't think so this syntax is correct - in your
case it should be

lnm.* ALIAS FOR ALL very_long_name.*;

but it looks a little bit scary in ADA based language.

Maybe

DECLARE lnm LABEL ALIAS FOR very_long_name;

or

DECLARE lnm ALIAS FOR LABEL very_long_name;

I think so implementation should not be hard. But there are advantages,
disadvantages - 1. label aliasing increases the complexity of variable
searching (instead comparing string with name of namespace, you should
compare list of names). Probably not too much. 2. The syntax is a little
bit harder than #option. Implementation based on #option can just rename
top namespace, so there is not any overhead, and parsing #option syntax is
pretty simple (and the syntax is shorter). So from implementation reasons I
prefer #option based syntax. There is clear zero impact on performance.

Regards

Pavel

st 18. 11. 2020 v 6:58 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:

út 17. 11. 2020 v 21:45 odesílatel Chapman Flack <chap@anastigmatix.net>
napsal:

On 11/17/20 15:18, Jack Christensen wrote:

CREATE OR REPLACE FUNCTION very_long_name(par1 int)
RETURNS int AS $$
#routine_label lnm
BEGIN
RAISE NOTICE '%', lnm.par1;

Could that be somehow shoehorned into the existing ALIAS syntax, maybe as

DECLARE
lnm ALIAS FOR ALL very_long_name.*;

or something?

I thought about it - but I don't think so this syntax is correct - in your
case it should be

lnm.* ALIAS FOR ALL very_long_name.*;

but it looks a little bit scary in ADA based language.

Maybe

DECLARE lnm LABEL ALIAS FOR very_long_name;

or

DECLARE lnm ALIAS FOR LABEL very_long_name;

I think so implementation should not be hard. But there are advantages,
disadvantages - 1. label aliasing increases the complexity of variable
searching (instead comparing string with name of namespace, you should
compare list of names). Probably not too much. 2. The syntax is a little
bit harder than #option. Implementation based on #option can just rename
top namespace, so there is not any overhead, and parsing #option syntax is
pretty simple (and the syntax is shorter). So from implementation reasons I
prefer #option based syntax. There is clear zero impact on performance.

Regards

I checked code - and I have to change my opinion - the current
implementation of namespaces in plpgsql doesn't allow renaming or realising
labels elegantly. The design has low memory requirements but it is not
flexible. I wrote a proof concept patch, and I had to hack the nsitem
little bit.

postgres=# create or replace function bubu(a int, b int)
returns void as $$
#routine_label b
begin
raise notice '% %', b.a, b.b;
end;
$$ language plpgsql;
CREATE FUNCTION
postgres=# select bubu(10,20);
NOTICE: 10 20
┌──────┐
│ bubu │
╞══════╡
│ │
└──────┘
(1 row)

st 18. 11. 2020 v 21:21 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:

st 18. 11. 2020 v 6:58 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:

út 17. 11. 2020 v 21:45 odesílatel Chapman Flack <chap@anastigmatix.net>
napsal:

On 11/17/20 15:18, Jack Christensen wrote:

CREATE OR REPLACE FUNCTION very_long_name(par1 int)
RETURNS int AS $$
#routine_label lnm
BEGIN
RAISE NOTICE '%', lnm.par1;

Could that be somehow shoehorned into the existing ALIAS syntax, maybe as

DECLARE
lnm ALIAS FOR ALL very_long_name.*;

or something?

I thought about it - but I don't think so this syntax is correct - in
your case it should be

lnm.* ALIAS FOR ALL very_long_name.*;

but it looks a little bit scary in ADA based language.

Maybe

DECLARE lnm LABEL ALIAS FOR very_long_name;

or

DECLARE lnm ALIAS FOR LABEL very_long_name;

I think so implementation should not be hard. But there are advantages,
disadvantages - 1. label aliasing increases the complexity of variable
searching (instead comparing string with name of namespace, you should
compare list of names). Probably not too much. 2. The syntax is a little
bit harder than #option. Implementation based on #option can just rename
top namespace, so there is not any overhead, and parsing #option syntax is
pretty simple (and the syntax is shorter). So from implementation reasons I
prefer #option based syntax. There is clear zero impact on performance.

Regards

I checked code - and I have to change my opinion - the current
implementation of namespaces in plpgsql doesn't allow renaming or realising
labels elegantly. The design has low memory requirements but it is not
flexible. I wrote a proof concept patch, and I had to hack the nsitem
little bit.

postgres=# create or replace function bubu(a int, b int)
returns void as $$
#routine_label b

maybe a better name for this option can be "routine_alias".

On 11/18/20 9:21 PM, Pavel Stehule wrote:

postgres=# create or replace function bubu(a int, b int)
returns void as $$
#routine_label b
begin
raise notice '% %', b.a, b.b;
end;
$$ language plpgsql;

Why not use the block labeling syntax we already have?

create or replace function bubu(a int, b int)
returns void as $$
<< b >>
begin
raise notice '% %', b.a, b.b;
end;
$$ language plpgsql;

That doesn't currently work, but it could be made to.
--
Vik Fearing

Hi

this is less invasive, and probably more correct work with ns items patch.

čt 19. 11. 2020 v 1:54 odesílatel Vik Fearing <vik@postgresfriends.org>
napsal:

On 11/18/20 9:21 PM, Pavel Stehule wrote:

postgres=# create or replace function bubu(a int, b int)
returns void as $$
#routine_label b
begin
raise notice '% %', b.a, b.b;
end;
$$ language plpgsql;

Why not use the block labeling syntax we already have?

create or replace function bubu(a int, b int)
returns void as $$
<< b >>
begin
raise notice '% %', b.a, b.b;
end;
$$ language plpgsql;

That doesn't currently work, but it could be made to.

I don't think it is correct - in your example you are trying to merge two
different namespaces - so minimally it should allow duplicate names in one
namespace, or it breaks compatibility.

And I don't think so we want to lose information and separate access to
function's arguments.

Regards

Pavel

Hi

fix broken regress test

Regards

Pavel

I think that this can be a useful feature in many occasions, especially since
the overhead is almost inexistent.

The implementation is sensible and there are regression tests to cover the new
feature.

There's a problem however if you try to add multiple #routine_label commands in
the same function. At minimum the Assert on "nse->itemno == (int)
PLPGSQL_LABEL_BLOCK" will fail.

I don't think that it makes sense to have multiple occurences of this command,
and we should simply error out if plpgsql_curr_compile->root_ns->itemno is
PLPGSQL_LABEL_REPLACED. If any case this should also be covered in the
regression tests.

Hi

so 13. 3. 2021 v 9:53 odesílatel Julien Rouhaud <rjuju123@gmail.com> napsal:

I think that this can be a useful feature in many occasions, especially
since
the overhead is almost inexistent.

The implementation is sensible and there are regression tests to cover the
new
feature.

There's a problem however if you try to add multiple #routine_label
commands in
the same function. At minimum the Assert on "nse->itemno == (int)
PLPGSQL_LABEL_BLOCK" will fail.

I don't think that it makes sense to have multiple occurences of this
command,
and we should simply error out if plpgsql_curr_compile->root_ns->itemno is
PLPGSQL_LABEL_REPLACED. If any case this should also be covered in the
regression tests.

I did it. Thank you for check

I wrote few sentences to documentation

Regards

Pavel

On Sat, Mar 13, 2021 at 12:10:29PM +0100, Pavel Stehule wrote:

so 13. 3. 2021 v 9:53 odes�latel Julien Rouhaud <rjuju123@gmail.com> napsal:

I don't think that it makes sense to have multiple occurences of this
command,
and we should simply error out if plpgsql_curr_compile->root_ns->itemno is
PLPGSQL_LABEL_REPLACED. If any case this should also be covered in the
regression tests.

I did it. Thank you for check

Thanks, LGTM.

I wrote few sentences to documentation

Great!

I just had a few cosmetic comments:

+      block can be changed by inserting special command at the start of the function
[...]
+     arguments) is possible with option <literal>routine_label</literal>:
[...]
+				 errhint("The option \"routine_label\" can be used only once in rutine."),
[...]
+-- Check root namespace renaming (routine_label option)

You're sometimes using "command" and "sometimes" option. Should we always use
the same term, maybe "command" as it's already used for #variable_conflict
documentation?

Also

+      variables can be qualified with the function's name. The name of this outer
+      block can be changed by inserting special command at the start of the function
+      <literal>#routine_label new_name</literal>.

It's missing a preposition before "special command". Maybe

+      variables can be qualified with the function's name. The name of this outer
+      block can be changed by inserting a special command
+      <literal>#routine_label new_name</literal> at the start of the function.

+ The function's argument can be qualified by function name:

Should be "qualified with the function name"

+     Sometimes the function name is too long and can be more practical to use
+     some shorter label. An change of label of top namespace (with functions's
+     arguments) is possible with option <literal>routine_label</literal>:

Few similar issues. How about:

Sometimes the function name is too long and *it* can be more practical to use
some shorter label. *The top namespace label can be changed* (*along* with
*the* functions' arguments) *using the option* <literal>routine_label</literal>:

I'm not a native English speaker so the proposes changed may be wrong or not
enough.

Hi

so 13. 3. 2021 v 12:48 odesílatel Julien Rouhaud <rjuju123@gmail.com>
napsal:

On Sat, Mar 13, 2021 at 12:10:29PM +0100, Pavel Stehule wrote:

so 13. 3. 2021 v 9:53 odesílatel Julien Rouhaud <rjuju123@gmail.com>

napsal:

I don't think that it makes sense to have multiple occurences of this
command,
and we should simply error out if

plpgsql_curr_compile->root_ns->itemno is

PLPGSQL_LABEL_REPLACED. If any case this should also be covered in the
regression tests.

I did it. Thank you for check

Thanks, LGTM.

I wrote few sentences to documentation

Great!

I just had a few cosmetic comments:

+      block can be changed by inserting special command at the start of
the function
[...]
+     arguments) is possible with option <literal>routine_label</literal>:
[...]
+                                errhint("The option \"routine_label\" can
be used only once in rutine."),
[...]
+-- Check root namespace renaming (routine_label option)

You're sometimes using "command" and "sometimes" option. Should we always
use
the same term, maybe "command" as it's already used for #variable_conflict
documentation?

Also

+      variables can be qualified with the function's name. The name of
this outer
+      block can be changed by inserting special command at the start of
the function
+      <literal>#routine_label new_name</literal>.

It's missing a preposition before "special command". Maybe

+      variables can be qualified with the function's name. The name of
this outer
+      block can be changed by inserting a special command
+      <literal>#routine_label new_name</literal> at the start of the
function.

+ The function's argument can be qualified by function name:

Should be "qualified with the function name"

+     Sometimes the function name is too long and can be more practical to
use
+     some shorter label. An change of label of top namespace (with
functions's
+     arguments) is possible with option <literal>routine_label</literal>:

Few similar issues. How about:

Sometimes the function name is too long and *it* can be more practical to
use
some shorter label. *The top namespace label can be changed* (*along* with
*the* functions' arguments) *using the option*
<literal>routine_label</literal>:

I'm not a native English speaker so the proposes changed may be wrong or
not
enough.

My English is good enough for taking beer everywhere in the world :) . Ti
is not good, but a lot of people who don't understand to English understand
my simplified fork of English language.

Thank you for check

updated patch attached

Pavel

On Sun, Mar 14, 2021 at 08:41:15PM +0100, Pavel Stehule wrote:

My English is good enough for taking beer everywhere in the world :) . Ti
is not good, but a lot of people who don't understand to English understand
my simplified fork of English language.

I have the same problem. We can talk about it and other stuff while having a
beer sometimes :)

updated patch attached

Thanks, it looks good to me, I'm marking the patch as RFC!

po 15. 3. 2021 v 4:54 odesílatel Julien Rouhaud <rjuju123@gmail.com> napsal:

On Sun, Mar 14, 2021 at 08:41:15PM +0100, Pavel Stehule wrote:

My English is good enough for taking beer everywhere in the world :) . Ti
is not good, but a lot of people who don't understand to English

understand

my simplified fork of English language.

I have the same problem. We can talk about it and other stuff while
having a
beer sometimes :)

:)

updated patch attached

Thanks, it looks good to me, I'm marking the patch as RFC!

Thank you very much

Regards

Pavel

On Mon, Mar 15, 2021 at 05:31:18AM +0100, Pavel Stehule wrote:

Thank you very much

I am not much a fan of the implementation done here. Based on my
understanding of the PL code, the namespace lookup is organized as a
list of items, with the namespace associated to the routine name being
the first one pushed to the list after plpgsql_ns_init() initializes
the top of it. Then, the proposed patch hijacks the namespace lookup
list by doing a replacement of the root label while parsing the
routine and then tweaks the lookup logic when a variable is qualified
(aka name2 != NULL) to bump the namespace list one level higher so as
it does not look after the namespace with the routine name but instead
fetches the one defined by ROUTINE_NAME. That seems rather bug-prone
to me, to say the least.

No changes to plpgsql_compile_inline()?

+       ereport(ERROR,
+               (errcode(ERRCODE_SYNTAX_ERROR),
+                errmsg("redundant option"),
+                errhint("The command \"routine_label\" can be used
only once in rutine."),
+                plpgsql_scanner_errposition(location)));
s/rutine/routine/

+ /* Don't allow repeated redefination of routine label */
redefination?

I am wondering whether it would be better to allow multiple aliases
though, and if it would bring more readability to the routines written
if these are treated equal to the top-most namespace which is the
routine name now, meaning that we would maintain not one, but N top
namespace labels that could be used as aliases of the root one.
--
Michael

st 17. 3. 2021 v 4:52 odesílatel Michael Paquier <michael@paquier.xyz>
napsal:

On Mon, Mar 15, 2021 at 05:31:18AM +0100, Pavel Stehule wrote:

Thank you very much

I am not much a fan of the implementation done here. Based on my
understanding of the PL code, the namespace lookup is organized as a
list of items, with the namespace associated to the routine name being
the first one pushed to the list after plpgsql_ns_init() initializes
the top of it. Then, the proposed patch hijacks the namespace lookup
list by doing a replacement of the root label while parsing the
routine and then tweaks the lookup logic when a variable is qualified
(aka name2 != NULL) to bump the namespace list one level higher so as
it does not look after the namespace with the routine name but instead
fetches the one defined by ROUTINE_NAME. That seems rather bug-prone
to me, to say the least.

I'll check what can be done. I am not sure if it is possible to push a new
label to the same level as the top label.

No changes to plpgsql_compile_inline()?

+       ereport(ERROR,
+               (errcode(ERRCODE_SYNTAX_ERROR),
+                errmsg("redundant option"),
+                errhint("The command \"routine_label\" can be used
only once in rutine."),
+                plpgsql_scanner_errposition(location)));
s/rutine/routine/

+ /* Don't allow repeated redefination of routine label */
redefination?

I'll check it. But inline block has not top label

I am wondering whether it would be better to allow multiple aliases
though, and if it would bring more readability to the routines written
if these are treated equal to the top-most namespace which is the
routine name now, meaning that we would maintain not one, but N top
namespace labels that could be used as aliases of the root one.

I do not have a strong opinion, but I am inclined to disallow this. I am
afraid so code can be less readable.

There is a precedent - SQL doesn't allow you to use table names as
qualifiers when you have an alias.

But it is a very important question. The selected behavior strongly impacts
an implementation.

What is the more common opinion about it? 1. Should we allow the original
top label or not? 2. Should we allow to define more top labels?

Regards

Pavel

On Wed, Mar 17, 2021 at 05:30:30AM +0100, Pavel Stehule wrote:

st 17. 3. 2021 v 4:52 odes�latel Michael Paquier <michael@paquier.xyz>

I am wondering whether it would be better to allow multiple aliases
though, and if it would bring more readability to the routines written
if these are treated equal to the top-most namespace which is the
routine name now, meaning that we would maintain not one, but N top
namespace labels that could be used as aliases of the root one.

I do not have a strong opinion, but I am inclined to disallow this. I am
afraid so code can be less readable.

There is a precedent - SQL doesn't allow you to use table names as
qualifiers when you have an alias.

+1

But it is a very important question. The selected behavior strongly impacts
an implementation.

What is the more common opinion about it? 1. Should we allow the original
top label or not? 2. Should we allow to define more top labels?

I also think that there should be a single usable top label, otherwise it will
lead to confusing code and it can be a source of bug.

On Wed, Mar 17, 2021 at 02:06:57PM +0800, Julien Rouhaud wrote:

I also think that there should be a single usable top label, otherwise it will
lead to confusing code and it can be a source of bug.

Okay, that's fine by me. Could it be possible to come up with an
approach that does not hijack the namespace list contruction and the
lookup logic as much as it does now? I get it that the patch is done
this way because of the ordering of operations done for the initial ns
list creation and the grammar parsing that adds the routine label on
top of the root one, but I'd like to believe that there are more solid
ways to achieve that, like for example something that decouples the
root label and its alias, or something that associates an alias
directly with its PLpgSQL_nsitem entry?
--
Michael