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