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 <[email protected]> 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 ([email protected])
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>
