proposal - using names as primary names of plpgsql function parameters instead $ based names

2017-05-13 18:26 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

Hi

Now, I when I working on plpgsql_check, I have to check function
parameters. I can use fn_vargargnos and out_param_varno for list of
arguments and related varno(s). when I detect some issue, I am using
refname. It is not too nice now, because these refnames are $ based. Long
names are alias only. There are not a possibility to find related alias.

So, my proposal. Now, we can use names as refname of parameter variable. $
based name can be used as alias. From user perspective there are not any
change.

Comments, notes?

here is a patch

Regards

Pavel

On 5/15/17 14:34, Pavel Stehule wrote:

Now, I when I working on plpgsql_check, I have to check function
parameters. I can use fn_vargargnos and out_param_varno for list of
arguments and related varno(s). when I detect some issue, I am using
refname. It is not too nice now, because these refnames are $ based.
Long names are alias only. There are not a possibility to find
related alias.

So, my proposal. Now, we can use names as refname of parameter
variable. $ based name can be used as alias. From user perspective
there are not any change.

Comments, notes?

here is a patch

I don't understand what this is changing. There are not documentation
or test changes.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

2017-05-19 3:14 GMT+02:00 Peter Eisentraut <peter.eisentraut@2ndquadrant.com

:

On 5/15/17 14:34, Pavel Stehule wrote:

Now, I when I working on plpgsql_check, I have to check function
parameters. I can use fn_vargargnos and out_param_varno for list of
arguments and related varno(s). when I detect some issue, I am using
refname. It is not too nice now, because these refnames are $ based.
Long names are alias only. There are not a possibility to find
related alias.

So, my proposal. Now, we can use names as refname of parameter
variable. $ based name can be used as alias. From user perspective
there are not any change.

Comments, notes?

here is a patch

I don't understand what this is changing. There are not documentation
or test changes.

This change is visible only for tools like plpgsql_check probably and
similar tools. Now, this info is not available from user space (maybe only
from some error message, I have to recheck it)

What is changed.

PLpgSQL variables has field refname - it can be used if you iterate over
variables or it is used for some error messages. When some variables is
searching, then namespace aliases are used. Now for any function argument
is created variable with refname "$x" and namespace aliases "$x" and "name"
if name exists. There are not any way, how to get a aliases related to
variable. When I raise a warning in plpgsql about function arguments I have
to print $x based messages, what is not too readable if function has lot of
parameters.

The proposal is the change of refname "$x" to "name" for all variables
created for function arguments.

There are another possibilities - maintain list of all aliases for
variables or dynamically search all related aliases in namespace tree. Both
little bit more code.

Regards

Pavel

2017-05-19 5:48 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

2017-05-19 3:14 GMT+02:00 Peter Eisentraut <peter.eisentraut@2ndquadrant.
com>:

On 5/15/17 14:34, Pavel Stehule wrote:

Now, I when I working on plpgsql_check, I have to check function
parameters. I can use fn_vargargnos and out_param_varno for list of
arguments and related varno(s). when I detect some issue, I am using
refname. It is not too nice now, because these refnames are $ based.
Long names are alias only. There are not a possibility to find
related alias.

So, my proposal. Now, we can use names as refname of parameter
variable. $ based name can be used as alias. From user perspective
there are not any change.

Comments, notes?

here is a patch

I don't understand what this is changing. There are not documentation
or test changes.

This change is visible only for tools like plpgsql_check probably and
similar tools. Now, this info is not available from user space (maybe only
from some error message, I have to recheck it)

What is changed.

PLpgSQL variables has field refname - it can be used if you iterate over
variables or it is used for some error messages. When some variables is
searching, then namespace aliases are used. Now for any function argument
is created variable with refname "$x" and namespace aliases "$x" and "name"
if name exists. There are not any way, how to get a aliases related to
variable. When I raise a warning in plpgsql about function arguments I have
to print $x based messages, what is not too readable if function has lot of
parameters.

The proposal is the change of refname "$x" to "name" for all variables
created for function arguments.

There are another possibilities - maintain list of all aliases for
variables or dynamically search all related aliases in namespace tree. Both
little bit more code.

I wrote small regression test, where expected behave is visible

master:

postgres=# create or replace function fx(x ct)
returns void as $$
begin
perform 1;
get diagnostics x = row_count;
end;
$$ language plpgsql;
ERROR: "$1" is not a scalar variable
LINE 5: get diagnostics x = row_count;

patched:

postgres=# create or replace function fx(x ct)
returns void as $$
begin
perform 1;
get diagnostics x = row_count;
end;
$$ language plpgsql;
ERROR: "x" is not a scalar variable
LINE 5: get diagnostics x = row_count;

Hi Pavel,

On Sat, May 20, 2017 at 11:55 AM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

2017-05-19 5:48 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

2017-05-19 3:14 GMT+02:00 Peter Eisentraut <peter.eisentraut@2ndquadrant.
com>:

On 5/15/17 14:34, Pavel Stehule wrote:

Now, I when I working on plpgsql_check, I have to check function
parameters. I can use fn_vargargnos and out_param_varno for list of
arguments and related varno(s). when I detect some issue, I am

using

refname. It is not too nice now, because these refnames are $

based.

Long names are alias only. There are not a possibility to find
related alias.

So, my proposal. Now, we can use names as refname of parameter
variable. $ based name can be used as alias. From user perspective
there are not any change.

Comments, notes?

here is a patch

I like the idea of using parameter name instead of $n symbols.

However, I am slightly worried that, at execution time if we want to
know the parameter position in the actual function signature, then it
will become difficult to get that from the corresponding datum
variable. I don't have any use-case for that though. But apart from
this concern, idea looks good to me.

Here are review comments on the patch:

1.
+ char *argname = NULL;

There is no need to initialize argname here. The Later code does that.

2.
+ argname = (argnames && argnames[i][0] != 0) ? argnames[i]
: NULL;

It will be better to check '\0' instead of 0, like we have that already.

3.
Check for argname exists is not consistent. At one place you have used
"argname != NULL" and other place it is "argname != '\0'".
Better to have "argname != NULL" at both the places.

4.
-- should fail -- message should to contain argument name
Should be something like this:
-- Should fail, error message should contain argument name

5.
+                argvariable = plpgsql_build_variable(argname != NULL ?
+                                                           argname : buf,
+                                                           0, argdtype,
false);

Please correct indentation.

---

BTW, instead of doing all these changes, I have done these changes this way:

-               /* Build variable and add to datum list */
-               argvariable = plpgsql_build_variable(buf, 0,
-                                                    argdtype, false);
+               /*
+                * Build variable and add to datum list.  If there's a name
for
+                * the argument, then use that else use $n name.
+                */
+               argvariable = plpgsql_build_variable((argnames &&
argnames[i][0] != '\0') ?
+                                                    argnames[i] : buf,
+                                                    0, argdtype, false);

This requires no new variable and thus no more changes elsewhere.

Attached patch with these changes. Please have a look.

Thanks

--
Jeevan Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Hi

2017-09-08 9:36 GMT+02:00 Jeevan Chalke <jeevan.chalke@enterprisedb.com>:

Hi Pavel,

On Sat, May 20, 2017 at 11:55 AM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

2017-05-19 5:48 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

2017-05-19 3:14 GMT+02:00 Peter Eisentraut <
peter.eisentraut@2ndquadrant.com>:

On 5/15/17 14:34, Pavel Stehule wrote:

Now, I when I working on plpgsql_check, I have to check function
parameters. I can use fn_vargargnos and out_param_varno for list

of

arguments and related varno(s). when I detect some issue, I am

using

refname. It is not too nice now, because these refnames are $

based.

Long names are alias only. There are not a possibility to find
related alias.

So, my proposal. Now, we can use names as refname of parameter
variable. $ based name can be used as alias. From user perspective
there are not any change.

Comments, notes?

here is a patch

I like the idea of using parameter name instead of $n symbols.

However, I am slightly worried that, at execution time if we want to
know the parameter position in the actual function signature, then it
will become difficult to get that from the corresponding datum
variable. I don't have any use-case for that though. But apart from
this concern, idea looks good to me.

Understand - but it was reason why I implemented this function - when I
have to search parameter name via offset, I cannot to use string searching.
When you know the parameter name, you can use a string searching in text
editor, in pager.

It is better supported now, then current behave.

Here are review comments on the patch:

1.
+ char *argname = NULL;

There is no need to initialize argname here. The Later code does that.

2.
+ argname = (argnames && argnames[i][0] != 0) ? argnames[i]
: NULL;

It will be better to check '\0' instead of 0, like we have that already.

This pattern is somewhere in PLpgSQL code. Your proposal is better.

3.
Check for argname exists is not consistent. At one place you have used
"argname != NULL" and other place it is "argname != '\0'".
Better to have "argname != NULL" at both the places.

sure

4.
-- should fail -- message should to contain argument name
Should be something like this:
-- Should fail, error message should contain argument name

5.
+                argvariable = plpgsql_build_variable(argname != NULL ?
+                                                           argname : buf,
+                                                           0, argdtype,
false);

Please correct indentation.

---

BTW, instead of doing all these changes, I have done these changes this
way:

-               /* Build variable and add to datum list */
-               argvariable = plpgsql_build_variable(buf, 0,
-                                                    argdtype, false);
+               /*
+                * Build variable and add to datum list.  If there's a
name for
+                * the argument, then use that else use $n name.
+                */
+               argvariable = plpgsql_build_variable((argnames &&
argnames[i][0] != '\0') ?
+                                                    argnames[i] : buf,
+                                                    0, argdtype, false);

This requires no new variable and thus no more changes elsewhere.

Attached patch with these changes. Please have a look.

Looks great - I added check to NULL only

Thank you

Pavel

Hi Pavel,

On Sat, Sep 9, 2017 at 11:42 AM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

Hi

2017-09-08 9:36 GMT+02:00 Jeevan Chalke <jeevan.chalke@enterprisedb.com>:

Hi Pavel,
I like the idea of using parameter name instead of $n symbols.

However, I am slightly worried that, at execution time if we want to
know the parameter position in the actual function signature, then it
will become difficult to get that from the corresponding datum
variable. I don't have any use-case for that though. But apart from
this concern, idea looks good to me.

Understand - but it was reason why I implemented this function - when I
have to search parameter name via offset, I cannot to use string searching.
When you know the parameter name, you can use a string searching in text
editor, in pager.

It is better supported now, then current behave.

Make sense.

BTW, instead of doing all these changes, I have done these changes this
way:

-               /* Build variable and add to datum list */
-               argvariable = plpgsql_build_variable(buf, 0,
-                                                    argdtype, false);
+               /*
+                * Build variable and add to datum list.  If there's a
name for
+                * the argument, then use that else use $n name.
+                */
+               argvariable = plpgsql_build_variable((argnames &&
argnames[i][0] != '\0') ?
+                                                    argnames[i] : buf,
+                                                    0, argdtype, false);

This requires no new variable and thus no more changes elsewhere.

Attached patch with these changes. Please have a look.

Looks great - I added check to NULL only

Looks good.
I have not made those changes in my earlier patch as I did not want to
update other code which is not touched by this patch.

Anyways, your changes related to NULL check seems reasonable.
However, in attached patch I have fixed indentation.

Passing it on to the committer.

Thanks
--
Jeevan Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

2017-09-11 9:46 GMT+02:00 Jeevan Chalke <jeevan.chalke@enterprisedb.com>:

Hi Pavel,

On Sat, Sep 9, 2017 at 11:42 AM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

Hi

2017-09-08 9:36 GMT+02:00 Jeevan Chalke <jeevan.chalke@enterprisedb.com>:

Hi Pavel,
I like the idea of using parameter name instead of $n symbols.

However, I am slightly worried that, at execution time if we want to
know the parameter position in the actual function signature, then it
will become difficult to get that from the corresponding datum
variable. I don't have any use-case for that though. But apart from
this concern, idea looks good to me.

Understand - but it was reason why I implemented this function - when I
have to search parameter name via offset, I cannot to use string searching.
When you know the parameter name, you can use a string searching in text
editor, in pager.

It is better supported now, then current behave.

Make sense.

BTW, instead of doing all these changes, I have done these changes this
way:

-               /* Build variable and add to datum list */
-               argvariable = plpgsql_build_variable(buf, 0,
-                                                    argdtype, false);
+               /*
+                * Build variable and add to datum list.  If there's a
name for
+                * the argument, then use that else use $n name.
+                */
+               argvariable = plpgsql_build_variable((argnames &&
argnames[i][0] != '\0') ?
+                                                    argnames[i] : buf,
+                                                    0, argdtype,
false);

This requires no new variable and thus no more changes elsewhere.

Attached patch with these changes. Please have a look.

Looks great - I added check to NULL only

Looks good.
I have not made those changes in my earlier patch as I did not want to
update other code which is not touched by this patch.

Anyways, your changes related to NULL check seems reasonable.
However, in attached patch I have fixed indentation.

Passing it on to the committer.

Thank you very much

Regards

Pavel

Jeevan Chalke <jeevan.chalke@enterprisedb.com> writes:
[ psql-named-arguments-03-jeevan.patch ]

Pushed with minor simplification of the test case.

I'm not quite as convinced as Pavel that this is an improvement ---
it will make error messages inconsistent between named and unnamed
arguments. Still, I follow the point that when there are a lot of
arguments, $n is pretty unhelpful. We can always revert this if
we get complaints.

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

2017-09-11 22:28 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:

Jeevan Chalke <jeevan.chalke@enterprisedb.com> writes:
[ psql-named-arguments-03-jeevan.patch ]

Pushed with minor simplification of the test case.

I'm not quite as convinced as Pavel that this is an improvement ---
it will make error messages inconsistent between named and unnamed
arguments. Still, I follow the point that when there are a lot of
arguments, $n is pretty unhelpful. We can always revert this if
we get complaints.

Thank you very much

Regards

Pavel