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.chalke@enterprisedb.com>:

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

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