Обсуждение: information schema parameter_default implementation
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
Вложения
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
"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
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-hackersI 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 intpostgres-# LANGUAGE SQLpostgres-# AS 'SELECT $1';CREATE FUNCTIONpostgres=#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 | 12 | 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 intpostgres-# LANGUAGE SQLpostgres-# AS 'SELECT $1';CREATE FUNCTIONpostgres=# 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 betterif (!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
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
Here is an updated patch which fixes the bug you have pointed out.Fixed.
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.Are you referring to indentation issues? I think the indentation is
> Some other minor observations:
> 1) Some variables are not lined in pg_get_function_arg_default().
good, but pgindent will fix it anyway.Factored that out into a separate helper function.
> 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)>I'd prefer to initialize it close to where it is used.
> 2) inputargn can be assigned in declaration.I think the purpose of the function is clear.
> 3) Function level comment for pg_get_function_arg_default() is
> missing.Suggestion?
> 4) You can also add comments inside the function, for example the
> comment for the line:
> nth = inputargn - 1 - (proc->pronargs - proc->pronargdefaults);I think this style is used throughout the documentation of the
> 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"?
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
<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 />
<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 />
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.
<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 />
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
Вложения
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
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
Updated patch attached.done
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)
>
--
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
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).
Updated patch
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.
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]
On 11/20/13, 8:39 PM, Rodolfo Campero wrote:
> 2013/11/20 Peter Eisentraut <peter_e@gmx.net <mailto:peter_e@gmx.net>>>It looks like you are not in the right directory.
> Updated patch
>
>
> I can't apply the patch; maybe I'm doing something wrong?
Rodolfo Campero
Anachronics S.R.L.
Tel: (54 11) 4899 2088
rodolfo.campero@anachronics.com
http://www.anachronics.com