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

Hi

2017-09-08 9:36 GMT+02:00 Jeevan Chalke <jeevan(dot)chalke(at)enterprisedb(dot)com>:

> Hi Pavel,
>
> On Sat, May 20, 2017 at 11:55 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
>
>>
>>
>>
>> 2017-05-19 5:48 GMT+02:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
>>
>>>
>>>
>>> 2017-05-19 3:14 GMT+02:00 Peter Eisentraut <
>>> peter(dot)eisentraut(at)2ndquadrant(dot)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

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