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 > >