information schema parameter_default implementation

On 15 September 2013 01:35, Peter Eisentraut <peter_e(at)gmx(dot)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.

I find only proc variable not aligned. Adding one more tab for all the
variables should help.
>
> > 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.
>
> >
> > 2) inputargn can be assigned in declaration.
>
> I'd prefer to initialize it close to where it is used.

Me too.
>
> > 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.
>
> > 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.
*/

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

This requires first an answer to my earlier question about why the
role-related privilege is needed.
---
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".
---
Instead of :
deparse_expression_pretty(node, NIL, false, false, 0, 0)
you can use :
deparse_expression(node, NIL, false, false)

We are anyway not using pretty printing.
---
Other than this, I have no more issues.

---
After the final review is over, the catversion.h requires the appropriate
date value.
>
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>