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