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