Обсуждение: [HACKERS] proposal - using names as primary names of plpgsql functionparameters instead $ based names

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?

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

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

   

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

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,
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.

 

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

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

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