Hi
2017-09-08 9:36 GMT+02:00 Jeevan Chalke <jeevan.cha...@enterprisedb.com>:
> Hi Pavel,
>
> On Sat, May 20, 2017 at 11:55 AM, Pavel Stehule <pavel.steh...@gmail.com>
> wrote:
>
>>
>>
>>
>> 2017-05-19 5:48 GMT+02:00 Pavel Stehule <pavel.steh...@gmail.com>:
>>
>>>
>>>
>>> 2017-05-19 3:14 GMT+02:00 Peter Eisentraut <
>>> peter.eisentr...@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
>
>
diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
index e9d7ef55e9..f320059fd0 100644
--- a/src/pl/plpgsql/src/pl_comp.c
+++ b/src/pl/plpgsql/src/pl_comp.c
@@ -433,9 +433,14 @@ do_compile(FunctionCallInfo fcinfo,
errmsg("PL/pgSQL functions cannot accept type %s",
format_type_be(argtypeid))));
- /* 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 != NULL
+ && argnames[i][0] != '\0') ?
+ argnames[i] : buf,
+ 0, argdtype, false);
if (argvariable->dtype == PLPGSQL_DTYPE_VAR)
{
@@ -461,7 +466,7 @@ do_compile(FunctionCallInfo fcinfo,
add_parameter_name(argitemtype, argvariable->dno, buf);
/* If there's a name for the argument, make an alias */
- if (argnames && argnames[i][0] != '\0')
+ if (argnames != NULL && argnames[i][0] != '\0')
add_parameter_name(argitemtype, argvariable->dno,
argnames[i]);
}
diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index 71099969a4..cf589d5390 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -6029,3 +6029,17 @@ SELECT * FROM list_partitioned_table() AS t;
2
(2 rows)
+--
+-- Check argument name is used instead of $n
+--
+CREATE TYPE ct AS (a int, b int);
+-- Should fail, error message should contain argument name instead of $1
+CREATE OR REPLACE FUNCTION fx(x ct) RETURNS void AS $$
+BEGIN
+ GET DIAGNOSTICS x = ROW_COUNT;
+ RETURN;
+END; $$ LANGUAGE plpgsql;
+ERROR: "x" is not a scalar variable
+LINE 3: GET DIAGNOSTICS x = ROW_COUNT;
+ ^
+DROP TYPE ct;
diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index 771d68282e..42f51e9a80 100644
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -4811,3 +4811,17 @@ BEGIN
END; $$ LANGUAGE plpgsql;
SELECT * FROM list_partitioned_table() AS t;
+
+--
+-- Check argument name is used instead of $n
+--
+CREATE TYPE ct AS (a int, b int);
+
+-- Should fail, error message should contain argument name instead of $1
+CREATE OR REPLACE FUNCTION fx(x ct) RETURNS void AS $$
+BEGIN
+ GET DIAGNOSTICS x = ROW_COUNT;
+ RETURN;
+END; $$ LANGUAGE plpgsql;
+
+DROP TYPE ct;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
