Re: [HACKERS] Re: proposal - using names as primary names of plpgsqlfunction parameters instead $ based names

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