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.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.
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
--
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 e9d7ef5..dd17bb5 100644
--- a/src/pl/plpgsql/src/pl_comp.c
+++ b/src/pl/plpgsql/src/pl_comp.c
@@ -433,9 +433,13 @@ 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 && argnames[i][0] != '\0') ?
+ argnames[i] : buf,
+ 0, argdtype, false);
if (argvariable->dtype == PLPGSQL_DTYPE_VAR)
{
diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index 7109996..cf589d5 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 771d682..42f51e9 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
