Re: [HACKERS] information schema parameter_default implementation
Review: information schema parameter_default implementation (v2) This is a review of the patch submitted in http://archives.postgresql.org/message-id/1384483678.5008.1.ca...@vanquo.pezone.net (information schema parameter_default implementation). Previous review from Amit Khandekar covers technical aspects: http://www.postgresql.org/message-id/CACoZds0eB3-yZ+pVLp9Sf5Xs_9xsDZ=jdp1d83ra-hjvjjo...@mail.gmail.com 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: http://www.postgresql.org/message-id/CACoZds0eB3-yZ+pVLp9Sf5Xs_9xsDZ=jdp1d83ra-hjvjjo...@mail.gmail.com 2013/11/21 Peter Eisentraut pete...@gmx.net On 11/20/13, 8:39 PM, Rodolfo Campero wrote: 2013/11/20 Peter Eisentraut pete...@gmx.net mailto:pete...@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.camp...@anachronics.com http://www.anachronics.com
Re: [HACKERS] information schema parameter_default implementation
On 11/20/13, 8:39 PM, Rodolfo Campero wrote: 2013/11/20 Peter Eisentraut pete...@gmx.net mailto:pete...@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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] information schema parameter_default implementation
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 pete...@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.camp...@anachronics.com http://www.anachronics.com
Re: [HACKERS] information schema parameter_default implementation
2013/11/20 Rodolfo Campero rodolfo.camp...@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: [HACKERS] information schema parameter_default implementation
Updated patch
From f82bc0c522b7c238b1dd8e5bb3495babd5b6192a Mon Sep 17 00:00:00 2001
From: Peter Eisentraut pete...@gmx.net
Date: Thu, 14 Nov 2013 21:43:15 -0500
Subject: [PATCH v2] Implement information_schema.parameters.parameter_default
column
Reviewed-by: Ali Dar ali.munir@gmail.com
Reviewed-by: Amit Khandekar amit.khande...@enterprisedb.com
---
doc/src/sgml/information_schema.sgml| 9 +++
src/backend/catalog/information_schema.sql | 9 ++-
src/backend/utils/adt/ruleutils.c | 84 +
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, 160 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 @@ titleliteralparameters/literal Columns/title
in future versions.)
/entry
/row
+
+ row
+ entryliteralparameter_default/literal/entry
+ entrytypecharacter_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 5ffce68..dace05a 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -2266,6 +2266,90 @@ 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);
+}
+
+/*
+ * Get textual representation of a function argument's default value. The
+ * second argument of this function is the argument number among all arguments
+ * (i.e. proallargtypes, *not* proargtypes), starting with 1, because that's
+ * how information_schema.sql uses it.
+ */
+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 1 || 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);
+
+ /* Calculate index into proargdefaults: proargdefaults corresponds to the
+ * last N input arguments, where N = pronargdefaults. */
+ nth_default = nth_inputarg - 1 - (proc-pronargs -
Re: [HACKERS] information schema parameter_default implementation
2013/11/20 Peter Eisentraut pete...@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: [HACKERS] information schema parameter_default implementation
Peter Eisentraut pete...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] information schema parameter_default implementation
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] information schema parameter_default implementation
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 From 442911934c1640af18a83ef2efaafc45c569c98f Mon Sep 17 00:00:00 2001 From: Peter Eisentraut pete...@gmx.net Date: Thu, 14 Nov 2013 21:43:15 -0500 Subject: [PATCH] Implement information_schema.parameters.parameter_default column Reviewed-by: Ali Dar ali.munir@gmail.com Reviewed-by: Amit Khandekar amit.khande...@enterprisedb.com --- doc/src/sgml/information_schema.sgml| 9 +++ src/backend/catalog/information_schema.sql | 9 ++- src/backend/utils/adt/ruleutils.c | 84 + 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, 160 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 @@ titleliteralparameters/literal Columns/title in future versions.) /entry /row + + row + entryliteralparameter_default/literal/entry + entrytypecharacter_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 5ffce68..dace05a 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -2266,6 +2266,90 @@ print_function_arguments(StringInfo buf, HeapTuple proctup, return argsprinted; } +static bool +is_input_argument(int nth, const char
Re: [HACKERS] information schema parameter_default implementation
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] information schema parameter_default implementation
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 pete...@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
Re: [HACKERS] information schema parameter_default implementation
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 pete...@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@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 @@ titleliteralparameters/literal Columns/title
in future versions.)
/entry
/row
+
+ row
+ entryliteralparameter_default/literal/entry
+ entrytypecharacter_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)
Re: [HACKERS] information schema parameter_default implementation
On Wed, Jan 9, 2013 at 4:28 PM, Peter Eisentraut pete...@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.ca...@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: [HACKERS] information schema parameter_default implementation
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@gmail.com wrote: On Wed, Jan 9, 2013 at 4:28 PM, Peter Eisentraut pete...@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.ca...@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
