Postgres Pro
Downloads
Home   >   mailing lists

Thread: information schema parameter_default implementation

information schema parameter_default implementation

From
Peter Eisentraut
Date:
Here is an implementation of the
information_schema.parameters.parameter_default column.

I ended up writing a C function to decode the whole thing from the
system catalogs, because it was too complicated in SQL, so I abandoned
the approach discussed in [0].


[0]: http://archives.postgresql.org/message-id/1356092400.25658.6.camel@vanquo.pezone.net
Attachment

Re: information schema parameter_default implementation

From
Ali Dar
Date:
On Wed, Jan 9, 2013 at 4:28 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
Here is an implementation of the
information_schema.parameters.parameter_default column.

I ended up writing a C function to decode the whole thing from the
system catalogs, because it was too complicated in SQL, so I abandoned
the approach discussed in [0].


[0]: http://archives.postgresql.org/message-id/1356092400.25658.6.camel@vanquo.pezone.net


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


I checked our your patch. There seems to be an issue when we have OUT parameters after the DEFAULT values. For example a simple test case is given below:

postgres=# CREATE FUNCTION functest1(a int default 1, out b int)
postgres-#     RETURNS int
postgres-#     LANGUAGE SQL
postgres-#     AS 'SELECT $1';
CREATE FUNCTION
postgres=# 
postgres=# SELECT ordinal_position, parameter_name, parameter_default FROM information_schema.parameters WHERE  specific_name LIKE 'functest%' ORDER BY 1;
 ordinal_position | parameter_name | parameter_default 
------------------+----------------+-------------------
                1 | a              | 1
                2 | b              | 1
(2 rows)

The out parameters gets the same value as the the last default parameter. The patch work only when default values are at the end. Switch the parameters and it starts working(make OUT parameter as first and default one the last one). Below is the example:

postgres=# CREATE FUNCTION functest1(out a int, b int default 1)
postgres-#     RETURNS int
postgres-#     LANGUAGE SQL
postgres-#     AS 'SELECT $1';
CREATE FUNCTION
postgres=# SELECT ordinal_position, parameter_name, parameter_default FROM information_schema.parameters WHERE  specific_name LIKE 'functest%' ORDER BY 1;
 ordinal_position | parameter_name | parameter_default 
------------------+----------------+-------------------
                1 | a              | 
                2 | b              | 1
(2 rows)


Some other minor observations:
1) Some variables are not lined in pg_get_function_arg_default().
2) I found the following check a bit confusing, maybe you can make it better
if (!argmodes || argmodes[i] == PROARGMODE_IN || argmodes[i] == PROARGMODE_INOUT || argmodes[i] == PROARGMODE_VARIADIC)
2) inputargn can be assigned in declaration.
3) Function level comment for pg_get_function_arg_default() is missing.
4) You can also add comments inside the function, for example the comment for the line:
nth = inputargn - 1 - (proc->pronargs - proc->pronargdefaults);
5) I think the line added in the documentation(informational_schema.sgml) is very long. Consider revising. Maybe change from:

"The default expression of the parameter, or null if none or if the function is not owned by a currently enabled role." TO

"The default expression of the parameter, or null if none was specified. It will also be null if the function is not owned by a currently enabled role."

I don't know what do you exactly mean by: "function is not owned by a currently enabled role"?

Regards,

Ali Dar

Re: information schema parameter_default implementation

From
Ali Dar
Date:
Another thing I forget: The patch does not apply because of the changes in "catversion.h"

Regards,
Ali Dar


On Thu, Jan 31, 2013 at 6:59 PM, Ali Dar <ali.munir.dar@gmail.com> wrote:
On Wed, Jan 9, 2013 at 4:28 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
Here is an implementation of the
information_schema.parameters.parameter_default column.

I ended up writing a C function to decode the whole thing from the
system catalogs, because it was too complicated in SQL, so I abandoned
the approach discussed in [0].


[0]: http://archives.postgresql.org/message-id/1356092400.25658.6.camel@vanquo.pezone.net


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


I checked our your patch. There seems to be an issue when we have OUT parameters after the DEFAULT values. For example a simple test case is given below:

postgres=# CREATE FUNCTION functest1(a int default 1, out b int)
postgres-#     RETURNS int
postgres-#     LANGUAGE SQL
postgres-#     AS 'SELECT $1';
CREATE FUNCTION
postgres=# 
postgres=# SELECT ordinal_position, parameter_name, parameter_default FROM information_schema.parameters WHERE  specific_name LIKE 'functest%' ORDER BY 1;
 ordinal_position | parameter_name | parameter_default 
------------------+----------------+-------------------
                1 | a              | 1
                2 | b              | 1
(2 rows)

The out parameters gets the same value as the the last default parameter. The patch work only when default values are at the end. Switch the parameters and it starts working(make OUT parameter as first and default one the last one). Below is the example:

postgres=# CREATE FUNCTION functest1(out a int, b int default 1)
postgres-#     RETURNS int
postgres-#     LANGUAGE SQL
postgres-#     AS 'SELECT $1';
CREATE FUNCTION
postgres=# SELECT ordinal_position, parameter_name, parameter_default FROM information_schema.parameters WHERE  specific_name LIKE 'functest%' ORDER BY 1;
 ordinal_position | parameter_name | parameter_default 
------------------+----------------+-------------------
                1 | a              | 
                2 | b              | 1
(2 rows)


Some other minor observations:
1) Some variables are not lined in pg_get_function_arg_default().
2) I found the following check a bit confusing, maybe you can make it better
if (!argmodes || argmodes[i] == PROARGMODE_IN || argmodes[i] == PROARGMODE_INOUT || argmodes[i] == PROARGMODE_VARIADIC)
2) inputargn can be assigned in declaration.
3) Function level comment for pg_get_function_arg_default() is missing.
4) You can also add comments inside the function, for example the comment for the line:
nth = inputargn - 1 - (proc->pronargs - proc->pronargdefaults);
5) I think the line added in the documentation(informational_schema.sgml) is very long. Consider revising. Maybe change from:

"The default expression of the parameter, or null if none or if the function is not owned by a currently enabled role." TO

"The default expression of the parameter, or null if none was specified. It will also be null if the function is not owned by a currently enabled role."

I don't know what do you exactly mean by: "function is not owned by a currently enabled role"?

Regards,

Ali Dar


Re: information schema parameter_default implementation

From
Peter Eisentraut
Date:
Here is an updated patch which fixes the bug you have pointed out.

On Thu, 2013-01-31 at 18:59 +0500, Ali Dar wrote:

> I checked our your patch. There seems to be an issue when we have OUT
> parameters after the DEFAULT values.

Fixed.

> Some other minor observations:
> 1) Some variables are not lined in pg_get_function_arg_default().

Are you referring to indentation issues?  I think the indentation is
good, but pgindent will fix it anyway.

> 2) I found the following check a bit confusing, maybe you can make it
> better
> if (!argmodes || argmodes[i] == PROARGMODE_IN || argmodes[i] ==
> PROARGMODE_INOUT || argmodes[i] == PROARGMODE_VARIADIC)

Factored that out into a separate helper function.
>
> 2) inputargn can be assigned in declaration.

I'd prefer to initialize it close to where it is used.

> 3) Function level comment for pg_get_function_arg_default() is
> missing.

I think the purpose of the function is clear.

> 4) You can also add comments inside the function, for example the
> comment for the line:
> nth = inputargn - 1 - (proc->pronargs - proc->pronargdefaults);

Suggestion?

> 5) I think the line added in the
> documentation(informational_schema.sgml) is very long. Consider
> revising. Maybe change from:
>
> "The default expression of the parameter, or null if none or if the
> function is not owned by a currently enabled role." TO
>
> "The default expression of the parameter, or null if none was
> specified. It will also be null if the function is not owned by a
> currently enabled role."
>
> I don't know what do you exactly mean by: "function is not owned by a
> currently enabled role"?

I think this style is used throughout the documentation of the
information schema.  We need to keep the descriptions reasonably
compact, but I'm willing to entertain other opinions.


>From 36f25fa2ec96879bda1993818db9a9632d8ac340 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Sat, 14 Sep 2013 15:55:54 -0400
Subject: [PATCH] Implement information_schema.parameters.parameter_default
 column

Reviewed-by: Ali Dar <ali.munir.dar@gmail.com>
---
 doc/src/sgml/information_schema.sgml            |  9 ++++
 src/backend/catalog/information_schema.sql      |  9 +++-
 src/backend/utils/adt/ruleutils.c               | 72 +++++++++++++++++++++++++
 src/include/catalog/catversion.h                |  2 +-
 src/include/catalog/pg_proc.h                   |  2 +
 src/include/utils/builtins.h                    |  1 +
 src/test/regress/expected/create_function_3.out | 33 +++++++++++-
 src/test/regress/sql/create_function_3.sql      | 24 +++++++++
 8 files changed, 148 insertions(+), 4 deletions(-)

diff --git a/doc/src/sgml/information_schema.sgml b/doc/src/sgml/information_schema.sgml
index 22e17bb..22f43c8 100644
--- a/doc/src/sgml/information_schema.sgml
+++ b/doc/src/sgml/information_schema.sgml
@@ -3323,6 +3323,15 @@ <title><literal>parameters</literal> Columns</title>
        in future versions.)
       </entry>
      </row>
+
+     <row>
+      <entry><literal>parameter_default</literal></entry>
+      <entry><type>character_data</type></entry>
+      <entry>
+       The default expression of the parameter, or null if none or if the
+       function is not owned by a currently enabled role.
+      </entry>
+     </row>
     </tbody>
    </tgroup>
   </table>
diff --git a/src/backend/catalog/information_schema.sql b/src/backend/catalog/information_schema.sql
index c5f7a8b..fd706e3 100644
--- a/src/backend/catalog/information_schema.sql
+++ b/src/backend/catalog/information_schema.sql
@@ -1133,10 +1133,15 @@ CREATE VIEW parameters AS
            CAST(null AS sql_identifier) AS scope_schema,
            CAST(null AS sql_identifier) AS scope_name,
            CAST(null AS cardinal_number) AS maximum_cardinality,
-           CAST((ss.x).n AS sql_identifier) AS dtd_identifier
+           CAST((ss.x).n AS sql_identifier) AS dtd_identifier,
+           CAST(
+             CASE WHEN pg_has_role(proowner, 'USAGE')
+                  THEN pg_get_function_arg_default(p_oid, (ss.x).n)
+                  ELSE NULL END
+             AS character_data) AS parameter_default

     FROM pg_type t, pg_namespace nt,
-         (SELECT n.nspname AS n_nspname, p.proname, p.oid AS p_oid,
+         (SELECT n.nspname AS n_nspname, p.proname, p.oid AS p_oid, p.proowner,
                  p.proargnames, p.proargmodes,
                  _pg_expandarray(coalesce(p.proallargtypes, p.proargtypes::oid[])) AS x
           FROM pg_namespace n, pg_proc p
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 9a1d12e..5a05396 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -2265,6 +2265,78 @@ static char *generate_function_name(Oid funcid, int nargs,
     return argsprinted;
 }

+static bool
+is_input_argument(int nth, const char *argmodes)
+{
+    return (!argmodes || argmodes[nth] == PROARGMODE_IN || argmodes[nth] == PROARGMODE_INOUT || argmodes[nth] ==
PROARGMODE_VARIADIC);
+}
+
+Datum
+pg_get_function_arg_default(PG_FUNCTION_ARGS)
+{
+    Oid            funcid = PG_GETARG_OID(0);
+    int32        nth_arg = PG_GETARG_INT32(1);
+    HeapTuple    proctup;
+    Form_pg_proc proc;
+    int            numargs;
+    Oid           *argtypes;
+    char      **argnames;
+    char       *argmodes;
+    int            i;
+    List       *argdefaults;
+    Node       *node;
+    char       *str;
+    int            nth_inputarg;
+    Datum        proargdefaults;
+    bool        isnull;
+    int            nth_default;
+
+    proctup = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcid));
+    if (!HeapTupleIsValid(proctup))
+        elog(ERROR, "cache lookup failed for function %u", funcid);
+
+    numargs = get_func_arg_info(proctup, &argtypes, &argnames, &argmodes);
+    if (nth_arg > numargs || !is_input_argument(nth_arg - 1, argmodes))
+    {
+        ReleaseSysCache(proctup);
+        PG_RETURN_NULL();
+    }
+
+    nth_inputarg = 0;
+    for (i = 0; i < nth_arg; i++)
+        if (is_input_argument(i, argmodes))
+            nth_inputarg++;
+
+    proargdefaults = SysCacheGetAttr(PROCOID, proctup,
+                                     Anum_pg_proc_proargdefaults,
+                                     &isnull);
+    if (isnull)
+    {
+        ReleaseSysCache(proctup);
+        PG_RETURN_NULL();
+    }
+
+    str = TextDatumGetCString(proargdefaults);
+    argdefaults = (List *) stringToNode(str);
+    Assert(IsA(argdefaults, List));
+    pfree(str);
+
+    proc = (Form_pg_proc) GETSTRUCT(proctup);
+
+    nth_default = nth_inputarg - 1 - (proc->pronargs - proc->pronargdefaults);
+    if (nth_default < 0 || nth_default >= list_length(argdefaults))
+    {
+        ReleaseSysCache(proctup);
+        PG_RETURN_NULL();
+    }
+    node = list_nth(argdefaults, nth_default);
+    str = deparse_expression_pretty(node, NIL, false, false, 0, 0);
+
+    ReleaseSysCache(proctup);
+
+    PG_RETURN_TEXT_P(string_to_text(str));
+}
+

 /*
  * deparse_expression            - General utility for deparsing expressions
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 3a18935..a0a9b4c 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
  */

 /*                            yyyymmddN */
-#define CATALOG_VERSION_NO    201309051
+#define CATALOG_VERSION_NO    201309112

 #endif
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index f03dd0b..5a5407c 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -1964,6 +1964,8 @@ DATA(insert OID = 2232 (  pg_get_function_identity_arguments       PGNSP PGUID 12 1
 DESCR("identity argument list of a function");
 DATA(insert OID = 2165 (  pg_get_function_result       PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 25 "26" _null_ _null_
_null__null_ pg_get_function_result _null_ _null_ _null_ )); 
 DESCR("result type of a function");
+DATA(insert OID = 3846 (  pg_get_function_arg_default   PGNSP PGUID 12 1 0 0 0 f f f f t f s 2 0 25 "26 23" _null_
_null__null_ _null_ pg_get_function_arg_default _null_ _null_ _null_ )); 
+DESCR("function argument default");

 DATA(insert OID = 1686 (  pg_get_keywords        PGNSP PGUID 12 10 400 0 0 f f f f t t s 0 0 2249 "" "{25,18,25}"
"{o,o,o}""{word,catcode,catdesc}" _null_ pg_get_keywords _null_ _null_ _null_ )); 
 DESCR("list of SQL keywords");
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index a5a0561..540bd0d 100644
--- a/src/include/utils/builtins.h
+++ b/src/include/utils/builtins.h
@@ -658,6 +658,7 @@ extern Datum pg_get_functiondef(PG_FUNCTION_ARGS);
 extern Datum pg_get_function_arguments(PG_FUNCTION_ARGS);
 extern Datum pg_get_function_identity_arguments(PG_FUNCTION_ARGS);
 extern Datum pg_get_function_result(PG_FUNCTION_ARGS);
+extern Datum pg_get_function_arg_default(PG_FUNCTION_ARGS);
 extern char *deparse_expression(Node *expr, List *dpcontext,
                    bool forceprefix, bool showimplicit);
 extern List *deparse_context_for(const char *aliasname, Oid relid);
diff --git a/src/test/regress/expected/create_function_3.out b/src/test/regress/expected/create_function_3.out
index e795232..486ae7a 100644
--- a/src/test/regress/expected/create_function_3.out
+++ b/src/test/regress/expected/create_function_3.out
@@ -425,9 +425,37 @@ SELECT proname, proisstrict FROM pg_proc
  functext_f_4 | t
 (4 rows)

+-- information_schema tests
+CREATE FUNCTION functest_IS_1(a int, b int default 1, c text default 'foo')
+    RETURNS int
+    LANGUAGE SQL
+    AS 'SELECT $1 + $2';
+CREATE FUNCTION functest_IS_2(out a int, b int default 1)
+    RETURNS int
+    LANGUAGE SQL
+    AS 'SELECT $1';
+CREATE FUNCTION functest_IS_3(a int default 1, out b int)
+    RETURNS int
+    LANGUAGE SQL
+    AS 'SELECT $1';
+SELECT routine_name, ordinal_position, parameter_name, parameter_default
+    FROM information_schema.parameters JOIN information_schema.routines USING (specific_schema, specific_name)
+    WHERE routine_schema = 'temp_func_test' AND routine_name ~ '^functest_is_'
+    ORDER BY 1, 2;
+ routine_name  | ordinal_position | parameter_name | parameter_default
+---------------+------------------+----------------+-------------------
+ functest_is_1 |                1 | a              |
+ functest_is_1 |                2 | b              | 1
+ functest_is_1 |                3 | c              | 'foo'::text
+ functest_is_2 |                1 | a              |
+ functest_is_2 |                2 | b              | 1
+ functest_is_3 |                1 | a              | 1
+ functest_is_3 |                2 | b              |
+(7 rows)
+
 -- Cleanups
 DROP SCHEMA temp_func_test CASCADE;
-NOTICE:  drop cascades to 16 other objects
+NOTICE:  drop cascades to 19 other objects
 DETAIL:  drop cascades to function functest_a_1(text,date)
 drop cascades to function functest_a_2(text[])
 drop cascades to function functest_a_3()
@@ -444,5 +472,8 @@ drop cascades to function functext_f_1(integer)
 drop cascades to function functext_f_2(integer)
 drop cascades to function functext_f_3(integer)
 drop cascades to function functext_f_4(integer)
+drop cascades to function functest_is_1(integer,integer,text)
+drop cascades to function functest_is_2(integer)
+drop cascades to function functest_is_3(integer)
 DROP USER regtest_unpriv_user;
 RESET search_path;
diff --git a/src/test/regress/sql/create_function_3.sql b/src/test/regress/sql/create_function_3.sql
index e2dd9a3..54b25e6 100644
--- a/src/test/regress/sql/create_function_3.sql
+++ b/src/test/regress/sql/create_function_3.sql
@@ -138,6 +138,30 @@ CREATE FUNCTION functext_F_4(int) RETURNS bool LANGUAGE 'sql'
                      'functext_F_3'::regproc,
                      'functext_F_4'::regproc) ORDER BY proname;

+
+-- information_schema tests
+
+CREATE FUNCTION functest_IS_1(a int, b int default 1, c text default 'foo')
+    RETURNS int
+    LANGUAGE SQL
+    AS 'SELECT $1 + $2';
+
+CREATE FUNCTION functest_IS_2(out a int, b int default 1)
+    RETURNS int
+    LANGUAGE SQL
+    AS 'SELECT $1';
+
+CREATE FUNCTION functest_IS_3(a int default 1, out b int)
+    RETURNS int
+    LANGUAGE SQL
+    AS 'SELECT $1';
+
+SELECT routine_name, ordinal_position, parameter_name, parameter_default
+    FROM information_schema.parameters JOIN information_schema.routines USING (specific_schema, specific_name)
+    WHERE routine_schema = 'temp_func_test' AND routine_name ~ '^functest_is_'
+    ORDER BY 1, 2;
+
+
 -- Cleanups
 DROP SCHEMA temp_func_test CASCADE;
 DROP USER regtest_unpriv_user;
--
1.8.4.rc3

Re: information schema parameter_default implementation

From
Amit Khandekar
Date:
I have assigned myself as reviewer for this one.

The logic of pg_get_function_arg_default() looks good. I will reply with any code-level comments later, but just a quick question before that:

What's the reason behind calling pg_has_role(proowner, 'USAGE') before calling pg_get_function_arg_default() ? :

             CASE WHEN pg_has_role(proowner, 'USAGE')
                  THEN pg_get_function_arg_default(p_oid, (ss.x).n)
                  ELSE NULL END

There is already a pg_has_role() filter added while fetching the pg_proc entries   :
          FROM pg_namespace n, pg_proc p
          WHERE n.oid = p.pronamespace
                AND (pg_has_role(p.proowner, 'USAGE') OR
                     has_function_privilege(p.oid, 'EXECUTE'))) AS ss

So the proc oid  in pg_get_function_arg_default(p_oid, (ss.x).n) belongs to a procedure for which the current user has USAGE privilege.


On 15 September 2013 01:35, Peter Eisentraut <peter_e@gmx.net> wrote:
Here is an updated patch which fixes the bug you have pointed out.

On Thu, 2013-01-31 at 18:59 +0500, Ali Dar wrote:

> I checked our your patch. There seems to be an issue when we have OUT
> parameters after the DEFAULT values.

Fixed.

> Some other minor observations:
> 1) Some variables are not lined in pg_get_function_arg_default().

Are you referring to indentation issues?  I think the indentation is
good, but pgindent will fix it anyway.

> 2) I found the following check a bit confusing, maybe you can make it
> better
> if (!argmodes || argmodes[i] == PROARGMODE_IN || argmodes[i] ==
> PROARGMODE_INOUT || argmodes[i] == PROARGMODE_VARIADIC)

Factored that out into a separate helper function.
>
> 2) inputargn can be assigned in declaration.

I'd prefer to initialize it close to where it is used.

> 3) Function level comment for pg_get_function_arg_default() is
> missing.

I think the purpose of the function is clear.

> 4) You can also add comments inside the function, for example the
> comment for the line:
> nth = inputargn - 1 - (proc->pronargs - proc->pronargdefaults);

Suggestion?

> 5) I think the line added in the
> documentation(informational_schema.sgml) is very long. Consider
> revising. Maybe change from:
>
> "The default expression of the parameter, or null if none or if the
> function is not owned by a currently enabled role." TO
>
> "The default expression of the parameter, or null if none was
> specified. It will also be null if the function is not owned by a
> currently enabled role."
>
> I don't know what do you exactly mean by: "function is not owned by a
> currently enabled role"?

I think this style is used throughout the documentation of the
information schema.  We need to keep the descriptions reasonably
compact, but I'm willing to entertain other opinions.




--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


information schema parameter_default implementation

From
Amit Khandekar
Date:
<br /><br /><br />On 15 September 2013 01:35, Peter Eisentraut <<a
href="mailto:peter_e@gmx.net">peter_e@gmx.net</a>>wrote:<br />><br />> Here is an updated patch which fixes
thebug you have pointed out.<br />><br />> On Thu, 2013-01-31 at 18:59 +0500, Ali Dar wrote:<br /> ><br />>
>I checked our your patch. There seems to be an issue when we have OUT<br />> > parameters after the DEFAULT
values.<br/>><br />> Fixed.<br />><br />> > Some other minor observations:<br />> > 1) Some
variablesare not lined in pg_get_function_arg_default(). <br /> ><br />> Are you referring to indentation issues?
 Ithink the indentation is<br />> good, but pgindent will fix it anyway.<br /><br />I find only proc variable not
aligned.Adding one more tab for all the variables should help. <br /> ><br />> > 2) I found the following
checka bit confusing, maybe you can make it<br />> > better<br />> > if (!argmodes || argmodes[i] ==
PROARGMODE_IN|| argmodes[i] ==<br />> > PROARGMODE_INOUT || argmodes[i] == PROARGMODE_VARIADIC)<br /> ><br
/>>Factored that out into a separate helper function.<br /><br /> <br />The statement needs to be split into 80
columnswidth lines.<br />><br />> ><br />> > 2) inputargn can be assigned in declaration.<br />><br
/>> I'd prefer to initialize it close to where it is used.<br /><br />Me too. <br />><br />> > 3) Function
levelcomment for pg_get_function_arg_default() is<br />> > missing.<br />><br />> I think the purpose of
thefunction is clear.<br /><br />Unless a reader goes through the definition, it is not obvious whether the second
functionargument represents the parameter position in input parameters or it is the parameter position in *all* the
functionparameters (i.e. proargtypes or proallargtypes). I think at least a one-liner description of what this function
doesshould be present. <br /> ><br />> > 4) You can also add comments inside the function, for example the<br
/>>> comment for the line:<br />> > nth = inputargn - 1 - (proc->pronargs -
proc->pronargdefaults);<br/>><br />> Suggestion?<br /><br />Yes, it is difficult to understand what's the
logicbehind this calculation, until we go read the documentation related to pg_proc.proargdefaults. I think this should
besufficient:<br />/*<br />* proargdefaults elements always correspond to the last N input arguments,<br /> * where N =
pronargdefaults.So calculate the nth_default accordingly.<br />*/<br /> <br />><br />> > 5) I think the line
addedin the<br />> > documentation(informational_schema.sgml) is very long. Consider<br />> > revising.
Maybechange from:<br /> > ><br />> > "The default expression of the parameter, or null if none or if the<br
/>>> function is not owned by a currently enabled role." TO<br />> ><br />> > "The default expression
ofthe parameter, or null if none was<br /> > > specified. It will also be null if the function is not owned by
a<br/>> > currently enabled role."<br />> ><br />> > I don't know what do you exactly mean by:
"functionis not owned by a<br /> > > currently enabled role"? <br />><br />> I think this style is used
throughoutthe documentation of the<br />> information schema.  We need to keep the descriptions reasonably<br />>
compact,but I'm willing to entertain other opinions.<br /><br />This requires first an answer to my earlier question
aboutwhy the role-related privilege is needed.<br />---<br />There should be an initial check to see if nth_arg is
passeda negative value. It is used as an index into the argmodes array, so a -ve array index can cause a crash. Because
pg_get_function_arg_default()can now be called by a user also, we need to validate the input values. I am ok with
returningwith an error "Invalid argument".<br /> ---<br />Instead of :<br />deparse_expression_pretty(node, NIL, false,
false,0, 0) <br />you can use :<br />deparse_expression(node, NIL, false, false)<br /><br />We are anyway not using
prettyprinting.<br />---<br />Other than this, I have no more issues.<br /><br />---<br />After the final review is
over,the catversion.h requires the appropriate date value.<br />><br />><br />><br />><br />> --<br
/>>Sent via pgsql-hackers mailing list (<a
href="mailto:pgsql-hackers@postgresql.org">pgsql-hackers@postgresql.org</a>)<br/> > To make changes to your
subscription:<br/>> <a
href="http://www.postgresql.org/mailpref/pgsql-hackers">http://www.postgresql.org/mailpref/pgsql-hackers</a><br
/>><br/><br />

information schema parameter_default implementation

From
Amit Khandekar
Date:
<br /><br /><br />On 15 September 2013 01:35, Peter Eisentraut <<a
href="mailto:peter_e@gmx.net">peter_e@gmx.net</a>>wrote:<br />><br />> Here is an updated patch which fixes
thebug you have pointed out.<br />><br />> On Thu, 2013-01-31 at 18:59 +0500, Ali Dar wrote:<br /> ><br />>
>I checked our your patch. There seems to be an issue when we have OUT<br />> > parameters after the DEFAULT
values.<br/>><br />> Fixed.<br />><br />> > Some other minor observations:<br />> > 1) Some
variablesare not lined in pg_get_function_arg_default(). <br /> ><br />> Are you referring to indentation issues?
 Ithink the indentation is<br />> good, but pgindent will fix it anyway.<br /><br />I find only proc variable not
aligned.Adding one more tab for all the variables should help. <br /> ><br />> > 2) I found the following
checka bit confusing, maybe you can make it<br />> > better<br />> > if (!argmodes || argmodes[i] ==
PROARGMODE_IN|| argmodes[i] ==<br />> > PROARGMODE_INOUT || argmodes[i] == PROARGMODE_VARIADIC)<br /> ><br
/>>Factored that out into a separate helper function.<br /><br /> <br />The statement needs to be split into 80
columnswidth lines.<br />><br />> ><br />> > 2) inputargn can be assigned in declaration.<br />><br
/>> I'd prefer to initialize it close to where it is used.<br /><br />Me too. <br />><br />> > 3) Function
levelcomment for pg_get_function_arg_default() is<br />> > missing.<br />><br />> I think the purpose of
thefunction is clear.<br /><br />Unless a reader goes through the definition, it is not obvious whether the second
functionargument represents the parameter position in input parameters or it is the parameter position in *all* the
functionparameters (i.e. proargtypes or proallargtypes). I think at least a one-liner description of what this function
doesshould be present. <br /> ><br />> > 4) You can also add comments inside the function, for example the<br
/>>> comment for the line:<br />> > nth = inputargn - 1 - (proc->pronargs -
proc->pronargdefaults);<br/>><br />> Suggestion?<br /><br />Yes, it is difficult to understand what's the
logicbehind this calculation, until we go read the documentation related to pg_proc.proargdefaults. I think this should
besufficient:<br />/*<br />* proargdefaults elements always correspond to the last N input arguments,<br /> * where N =
pronargdefaults.So calculate the nth_default accordingly.<br />*/<br /> <br />><br />> > 5) I think the line
addedin the<br />> > documentation(informational_schema.sgml) is very long. Consider<br />> > revising.
Maybechange from:<br /> > ><br />> > "The default expression of the parameter, or null if none or if the<br
/>>> function is not owned by a currently enabled role." TO<br />> ><br />> > "The default expression
ofthe parameter, or null if none was<br /> > > specified. It will also be null if the function is not owned by
a<br/>> > currently enabled role."<br />> ><br />> > I don't know what do you exactly mean by:
"functionis not owned by a<br /> > > currently enabled role"? <br />><br />> I think this style is used
throughoutthe documentation of the<br />> information schema.  We need to keep the descriptions reasonably<br />>
compact,but I'm willing to entertain other opinions.<br /><br />This requires first an answer to my earlier question
aboutwhy the role-related privilege is needed.<br />---<br />There should be an initial check to see if nth_arg is
passeda negative value. It is used as an index into the argmodes array, so a -ve array index can cause a crash. Because
pg_get_function_arg_default()can now be called by a user also, we need to validate the input values. I am ok with
returningwith an error "Invalid argument".<br /> ---<br />Instead of :<br />deparse_expression_pretty(node, NIL, false,
false,0, 0) <br />you can use :<br />deparse_expression(node, NIL, false, false)<br /><br />We are anyway not using
prettyprinting.<br />---<br />Other than this, I have no more issues.<br /><br />---<br />After the final review is
over,the catversion.h requires the appropriate date value.<br />><br />><br />><br />><br />> --<br
/>>Sent via pgsql-hackers mailing list (<a
href="mailto:pgsql-hackers@postgresql.org">pgsql-hackers@postgresql.org</a>)<br/> > To make changes to your
subscription:<br/>> <a
href="http://www.postgresql.org/mailpref/pgsql-hackers">http://www.postgresql.org/mailpref/pgsql-hackers</a><br
/>><br/><br />

Re: information schema parameter_default implementation

From
Peter Eisentraut
Date:
On Wed, 2013-09-18 at 20:13 +0530, Amit Khandekar wrote:
> What's the reason behind calling pg_has_role(proowner, 'USAGE') before
> calling pg_get_function_arg_default() ? : 
> 
>              CASE WHEN pg_has_role(proowner, 'USAGE')
> THEN pg_get_function_arg_default(p_oid, (ss.x).n)
> ELSE NULL END 
> 
> There is already a pg_has_role() filter added while fetching the
> pg_proc entries   :           FROM pg_namespace n, pg_proc p
> WHERE n.oid = p.pronamespace                 AND
> (pg_has_role(p.proowner, 'USAGE') OR
>  has_function_privilege(p.oid, 'EXECUTE'))) AS ss 
> 
> So the proc oid  in pg_get_function_arg_default(p_oid, (ss.x).n)
> belongs to a procedure for which the current user has USAGE
> privilege. 

No, the pg_proc entry belongs to a function for which the current user
is the owner *or* has EXECUTE privilege.  The default, however, is only
shown to the owner.  This is per SQL standard.

information schema parameter_default implementation

From
Amit Khandekar
Date:
<br /><br /><br />On 15 September 2013 01:35, Peter Eisentraut <<a
href="mailto:peter_e@gmx.net">peter_e@gmx.net</a>>wrote:<br />><br />> Here is an updated patch which fixes
thebug you have pointed out.<br />><br />> On Thu, 2013-01-31 at 18:59 +0500, Ali Dar wrote:<br /> ><br />>
>I checked our your patch. There seems to be an issue when we have OUT<br />> > parameters after the DEFAULT
values.<br/>><br />> Fixed.<br />><br />> > Some other minor observations:<br />> > 1) Some
variablesare not lined in pg_get_function_arg_default(). <br /> ><br />> Are you referring to indentation issues?
 Ithink the indentation is<br />> good, but pgindent will fix it anyway.<br /><br />I find only proc variable not
aligned.Adding one more tab for all the variables should help. <br /> ><br />> > 2) I found the following
checka bit confusing, maybe you can make it<br />> > better<br />> > if (!argmodes || argmodes[i] ==
PROARGMODE_IN|| argmodes[i] ==<br />> > PROARGMODE_INOUT || argmodes[i] == PROARGMODE_VARIADIC)<br /> ><br
/>>Factored that out into a separate helper function.<br /><br /> <br />The statement needs to be split into 80
columnswidth lines.<br />><br />> ><br />> > 2) inputargn can be assigned in declaration.<br />><br
/>> I'd prefer to initialize it close to where it is used.<br /><br />Me too. <br />><br />> > 3) Function
levelcomment for pg_get_function_arg_default() is<br />> > missing.<br />><br />> I think the purpose of
thefunction is clear.<br /><br />Unless a reader goes through the definition, it is not obvious whether the second
functionargument represents the parameter position in input parameters or it is the parameter position in *all* the
functionparameters (i.e. proargtypes or proallargtypes). I think at least a one-liner description of what this function
doesshould be present. <br /> ><br />> > 4) You can also add comments inside the function, for example the<br
/>>> comment for the line:<br />> > nth = inputargn - 1 - (proc->pronargs -
proc->pronargdefaults);<br/>><br />> Suggestion?<br /><br />Yes, it is difficult to understand what's the
logicbehind this calculation, until we go read the documentation related to pg_proc.proargdefaults. I think this should
besufficient:<br />/*<br />* proargdefaults elements always correspond to the last N input arguments,<br /> * where N =
pronargdefaults.So calculate the nth_default accordingly.<br />*/<br /> <br />><br />> > 5) I think the line
addedin the<br />> > documentation(informational_schema.sgml) is very long. Consider<br />> > revising.
Maybechange from:<br /> > ><br />> > "The default expression of the parameter, or null if none or if the<br
/>>> function is not owned by a currently enabled role." TO<br />> ><br />> > "The default expression
ofthe parameter, or null if none was<br /> > > specified. It will also be null if the function is not owned by
a<br/>> > currently enabled role."<br />> ><br />> > I don't know what do you exactly mean by:
"functionis not owned by a<br /> > > currently enabled role"? <br />><br />> I think this style is used
throughoutthe documentation of the<br />> information schema.  We need to keep the descriptions reasonably<br />>
compact,but I'm willing to entertain other opinions.<br /><br />This requires first an answer to my earlier question
aboutwhy the role-related privilege is needed.<br />---<br />There should be an initial check to see if nth_arg is
passeda negative value. It is used as an index into the argmodes array, so a -ve array index can cause a crash. Because
pg_get_function_arg_default()can now be called by a user also, we need to validate the input values. I am ok with
returningwith an error "Invalid argument".<br /> ---<br />Instead of :<br />deparse_expression_pretty(node, NIL, false,
false,0, 0) <br />you can use :<br />deparse_expression(node, NIL, false, false)<br /><br />We are anyway not using
prettyprinting.<br />---<br />Other than this, I have no more issues.<br /><br />---<br />After the final review is
over,the catversion.h requires the appropriate date value.<br />><br />><br />><br />><br />> --<br
/>>Sent via pgsql-hackers mailing list (<a
href="mailto:pgsql-hackers@postgresql.org">pgsql-hackers@postgresql.org</a>)<br/> > To make changes to your
subscription:<br/>> <a
href="http://www.postgresql.org/mailpref/pgsql-hackers">http://www.postgresql.org/mailpref/pgsql-hackers</a><br
/>><br/><br />

Re: information schema parameter_default implementation

From
Peter Eisentraut
Date:
Updated patch attached.

On Sat, 2013-11-09 at 12:09 +0530, Amit Khandekar wrote:
> > > 2) I found the following check a bit confusing, maybe you can make
> it
> > > better
> > > if (!argmodes || argmodes[i] == PROARGMODE_IN || argmodes[i] ==
> > > PROARGMODE_INOUT || argmodes[i] == PROARGMODE_VARIADIC)
> >
> > Factored that out into a separate helper function.
>
>
> The statement needs to be split into 80 columns width lines.

done

> > > 3) Function level comment for pg_get_function_arg_default() is
> > > missing.
> >
> > I think the purpose of the function is clear.
>
> Unless a reader goes through the definition, it is not obvious whether
> the second function argument represents the parameter position in
> input parameters or it is the parameter position in *all* the function
> parameters (i.e. proargtypes or proallargtypes). I think at least a
> one-liner description of what this function does should be present.

done

> >
> > > 4) You can also add comments inside the function, for example the
> > > comment for the line:
> > > nth = inputargn - 1 - (proc->pronargs - proc->pronargdefaults);
> >
> > Suggestion?
>
> Yes, it is difficult to understand what's the logic behind this
> calculation, until we go read the documentation related to
> pg_proc.proargdefaults. I think this should be sufficient:
> /*
> * proargdefaults elements always correspond to the last N input
> arguments,
> * where N = pronargdefaults. So calculate the nth_default accordingly.
> */

done

> There should be an initial check to see if nth_arg is passed a
> negative value. It is used as an index into the argmodes array, so a
> -ve array index can cause a crash. Because
> pg_get_function_arg_default() can now be called by a user also, we
> need to validate the input values. I am ok with returning with an
> error "Invalid argument".

done

> ---
> Instead of :
> deparse_expression_pretty(node, NIL, false, false, 0, 0)
> you can use :
> deparse_expression(node, NIL, false, false)
>
done
Attachment

Re: information schema parameter_default implementation

From
Tom Lane
Date:
Peter Eisentraut <peter_e@gmx.net> writes:
> [ 0001-Implement-information_schema.parameters.parameter_de.patch ]

I'm a bit confused as to where this column is coming from?  There's
no such thing in SQL:2008 as far as I can see.  If it's coming from
some not-yet-ratified draft, maybe we should wait for ratification.
It's impossible for a bystander to tell if this implementation conforms
to what the spec is expecting.

BTW, although SQL:2008 lacks this column in the parameters view, there
are about six columns it has that we don't: see the from_sql_xxx and
to_sql_xxx columns.  Surely we should put those in (at least as dummy
columns) before trying to claim adherence to some even-newer spec draft.

As far as the code goes, I have no particular objections, modulo the
question about whether this patch is implementing spec-compatible
behavior.  A small stylistic idea is that maybe the computation of
nth_inputarg should be moved down nearer where it's used.  Really
that's just part of the calculation of nth_default, and it wouldn't
be unreasonable to stick it under the comment explaining why we're
doing that calculation like that.
        regards, tom lane

Re: information schema parameter_default implementation

From
Peter Eisentraut
Date:
On Sun, 2013-11-17 at 16:38 -0500, Tom Lane wrote:
> I'm a bit confused as to where this column is coming from?  There's
> no such thing in SQL:2008 as far as I can see.

SQL:2011

Re: information schema parameter_default implementation

From
Rodolfo Campero
Date:
Peter,

This patch no longer applies, because CATALOG_VERSION_NO in src/include/catalog/catversion.h has changed. I touched the patch and got it to apply without other problems (I haven't built yet).

Regards,


2013/11/14 Peter Eisentraut <peter_e@gmx.net>
Updated patch attached.

On Sat, 2013-11-09 at 12:09 +0530, Amit Khandekar wrote:
> > > 2) I found the following check a bit confusing, maybe you can make
> it
> > > better
> > > if (!argmodes || argmodes[i] == PROARGMODE_IN || argmodes[i] ==
> > > PROARGMODE_INOUT || argmodes[i] == PROARGMODE_VARIADIC)
> >
> > Factored that out into a separate helper function.
>
>
> The statement needs to be split into 80 columns width lines.

done

> > > 3) Function level comment for pg_get_function_arg_default() is
> > > missing.
> >
> > I think the purpose of the function is clear.
>
> Unless a reader goes through the definition, it is not obvious whether
> the second function argument represents the parameter position in
> input parameters or it is the parameter position in *all* the function
> parameters (i.e. proargtypes or proallargtypes). I think at least a
> one-liner description of what this function does should be present.

done

> >
> > > 4) You can also add comments inside the function, for example the
> > > comment for the line:
> > > nth = inputargn - 1 - (proc->pronargs - proc->pronargdefaults);
> >
> > Suggestion?
>
> Yes, it is difficult to understand what's the logic behind this
> calculation, until we go read the documentation related to
> pg_proc.proargdefaults. I think this should be sufficient:
> /*
> * proargdefaults elements always correspond to the last N input
> arguments,
> * where N = pronargdefaults. So calculate the nth_default accordingly.
> */

done

> There should be an initial check to see if nth_arg is passed a
> negative value. It is used as an index into the argmodes array, so a
> -ve array index can cause a crash. Because
> pg_get_function_arg_default() can now be called by a user also, we
> need to validate the input values. I am ok with returning with an
> error "Invalid argument".

done

> ---
> Instead of :
> deparse_expression_pretty(node, NIL, false, false, 0, 0)
> you can use :
> deparse_expression(node, NIL, false, false)
>
done



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers




--
Rodolfo Campero
Anachronics S.R.L.
Tel: (54 11) 4899 2088
rodolfo.campero@anachronics.com
http://www.anachronics.com

Re: information schema parameter_default implementation

From
Rodolfo Campero
Date:
2013/11/20 Rodolfo Campero <rodolfo.campero@anachronics.com>
Peter,

This patch no longer applies, because CATALOG_VERSION_NO in src/include/catalog/catversion.h has changed. I touched the patch and got it to apply without other problems (I haven't built yet).

 
Make fails:
[...]
make -C catalog schemapg.h
make[3]: se ingresa al directorio `/home/rodolfo/trabajo/postgresql/src/backend/catalog'
cd ../../../src/include/catalog && '/usr/bin/perl' ./duplicate_oids
3846
make[3]: *** [postgres.bki] Error 1
[...]

OID = 3846 is duplicated, see:

./src/include/catalog/pg_proc.h:1976:DATA(insert OID = 3846 (  pg_get_function_arg_default [...]
./src/include/catalog/pg_proc.h:4679:DATA(insert OID = 3846 ( make_date [...]

Re: information schema parameter_default implementation

From
Peter Eisentraut
Date:
Updated patch
Attachment

Re: information schema parameter_default implementation

From
Rodolfo Campero
Date:
2013/11/20 Peter Eisentraut <peter_e@gmx.net>
Updated patch

I can't apply the patch; maybe I'm doing something wrong?

$ git apply v2-0001-Implement-information_schema.parameters.parameter.patch
v2-0001-Implement-information_schema.parameters.parameter.patch:49: trailing whitespace.
           CAST((ss.x).n AS sql_identifier) AS dtd_identifier,
v2-0001-Implement-information_schema.parameters.parameter.patch:50: trailing whitespace.
           CAST(
v2-0001-Implement-information_schema.parameters.parameter.patch:51: trailing whitespace.
             CASE WHEN pg_has_role(proowner, 'USAGE')
v2-0001-Implement-information_schema.parameters.parameter.patch:52: trailing whitespace.
                  THEN pg_get_function_arg_default(p_oid, (ss.x).n)
v2-0001-Implement-information_schema.parameters.parameter.patch:53: trailing whitespace.
                  ELSE NULL END
error: patch failed: doc/src/sgml/information_schema.sgml:3323
error: doc/src/sgml/information_schema.sgml: patch does not apply
error: patch failed: src/backend/catalog/information_schema.sql:1133
error: src/backend/catalog/information_schema.sql: patch does not apply
error: patch failed: src/backend/utils/adt/ruleutils.c:2266
error: src/backend/utils/adt/ruleutils.c: patch does not apply
error: patch failed: src/include/catalog/catversion.h:53
error: src/include/catalog/catversion.h: patch does not apply
error: patch failed: src/include/catalog/pg_proc.h:1973
error: src/include/catalog/pg_proc.h: patch does not apply
error: patch failed: src/include/utils/builtins.h:665
error: src/include/utils/builtins.h: patch does not apply
error: patch failed: src/test/regress/expected/create_function_3.out:425
error: src/test/regress/expected/create_function_3.out: patch does not apply
error: patch failed: src/test/regress/sql/create_function_3.sql:138
error: src/test/regress/sql/create_function_3.sql: patch does not apply


Re: information schema parameter_default implementation

From
Peter Eisentraut
Date:
On 11/20/13, 8:39 PM, Rodolfo Campero wrote:
> 2013/11/20 Peter Eisentraut <peter_e@gmx.net <mailto:peter_e@gmx.net>>
> 
>     Updated patch
> 
> 
> I can't apply the patch; maybe I'm doing something wrong?

It looks like you are not in the right directory.

Re: information schema parameter_default implementation

From
Rodolfo Campero
Date:
Review: information schema parameter_default implementation (v2)

(information schema parameter_default implementation).

Previous review from Amit Khandekar covers technical aspects:

Submission review
=================
 * Is the patch in a patch format which has context? (eg: context diff format)
Yes

 * Does it apply cleanly to the current git master?
I had to apply "fromdos" to remove trailing whitespace. 
After that, the patch applies cleanly to HEAD.
Make builds without warnings, except for:
In file included from gram.y:13675:0:
scan.c: In function ‘yy_try_NUL_trans’:
scan.c:10185:23: warning: unused variable ‘yyg’ [-Wunused-variable]
but from previous messages in this mailing list I think that's unrelated to this patch and normal.
The regression tests all pass successfully against the new patch.

 * Does it include reasonable tests, necessary doc patches, etc?
Yes

Usability review
================
 * Does the patch actually implement that?
The patch implements the column "parameter_default" of information schema view "parameters", defined in the SQL:2011 standard.
I could not get a hand to the spec, but I found a document where it is mentioned: http://msdn.microsoft.com/en-us/library/jj191733(v=sql.105).aspx

 * Do we want that?
I think we do, as it is defined in the spec.

 * Do we already have it?
No

 * Does it follow SQL spec, or the community-agreed behavior?
SQL:2011.

 * Does it include pg_dump support (if applicable)?
N/A

 * Are there dangers?
None AFAICS.

 * Have all the bases been covered?
Yes.

Feature test
============
 * Does the feature work as advertised?
Yes

 * Are there corner cases the author has failed to consider?
None that I can see.

 * Are there any assertion failures or crashes?
No

Performance review
==================
N/A

Coding review
=============
I'm not skilled enough to do a code review; see previous review from Amit:



2013/11/21 Peter Eisentraut <peter_e@gmx.net>
On 11/20/13, 8:39 PM, Rodolfo Campero wrote:
> 2013/11/20 Peter Eisentraut <peter_e@gmx.net <mailto:peter_e@gmx.net>>
>
>     Updated patch
>
>
> I can't apply the patch; maybe I'm doing something wrong?

It looks like you are not in the right directory.




--
Rodolfo Campero
Anachronics S.R.L.
Tel: (54 11) 4899 2088
rodolfo.campero@anachronics.com
http://www.anachronics.com