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

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


--