information schema parameter_default implementation - Mailing list pgsql-hackers

<br /><br /><br />On 15 September 2013 01:35, Peter Eisentraut <<a
href="mailto:peter_e@gmx.net">peter_e@gmx.net</a>>wrote:<br />><br />> Here is an updated patch which fixes
thebug you have pointed out.<br />><br />> On Thu, 2013-01-31 at 18:59 +0500, Ali Dar wrote:<br /> ><br />>
>I checked our your patch. There seems to be an issue when we have OUT<br />> > parameters after the DEFAULT
values.<br/>><br />> Fixed.<br />><br />> > Some other minor observations:<br />> > 1) Some
variablesare not lined in pg_get_function_arg_default(). <br /> ><br />> Are you referring to indentation issues?
 Ithink the indentation is<br />> good, but pgindent will fix it anyway.<br /><br />I find only proc variable not
aligned.Adding one more tab for all the variables should help. <br /> ><br />> > 2) I found the following
checka bit confusing, maybe you can make it<br />> > better<br />> > if (!argmodes || argmodes[i] ==
PROARGMODE_IN|| argmodes[i] ==<br />> > PROARGMODE_INOUT || argmodes[i] == PROARGMODE_VARIADIC)<br /> ><br
/>>Factored that out into a separate helper function.<br /><br /> <br />The statement needs to be split into 80
columnswidth lines.<br />><br />> ><br />> > 2) inputargn can be assigned in declaration.<br />><br
/>> I'd prefer to initialize it close to where it is used.<br /><br />Me too. <br />><br />> > 3) Function
levelcomment for pg_get_function_arg_default() is<br />> > missing.<br />><br />> I think the purpose of
thefunction is clear.<br /><br />Unless a reader goes through the definition, it is not obvious whether the second
functionargument represents the parameter position in input parameters or it is the parameter position in *all* the
functionparameters (i.e. proargtypes or proallargtypes). I think at least a one-liner description of what this function
doesshould be present. <br /> ><br />> > 4) You can also add comments inside the function, for example the<br
/>>> comment for the line:<br />> > nth = inputargn - 1 - (proc->pronargs -
proc->pronargdefaults);<br/>><br />> Suggestion?<br /><br />Yes, it is difficult to understand what's the
logicbehind this calculation, until we go read the documentation related to pg_proc.proargdefaults. I think this should
besufficient:<br />/*<br />* proargdefaults elements always correspond to the last N input arguments,<br /> * where N =
pronargdefaults.So calculate the nth_default accordingly.<br />*/<br /> <br />><br />> > 5) I think the line
addedin the<br />> > documentation(informational_schema.sgml) is very long. Consider<br />> > revising.
Maybechange from:<br /> > ><br />> > "The default expression of the parameter, or null if none or if the<br
/>>> function is not owned by a currently enabled role." TO<br />> ><br />> > "The default expression
ofthe parameter, or null if none was<br /> > > specified. It will also be null if the function is not owned by
a<br/>> > currently enabled role."<br />> ><br />> > I don't know what do you exactly mean by:
"functionis not owned by a<br /> > > currently enabled role"? <br />><br />> I think this style is used
throughoutthe documentation of the<br />> information schema.  We need to keep the descriptions reasonably<br />>
compact,but I'm willing to entertain other opinions.<br /><br />This requires first an answer to my earlier question
aboutwhy the role-related privilege is needed.<br />---<br />There should be an initial check to see if nth_arg is
passeda 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
returningwith an error "Invalid argument".<br /> ---<br />Instead of :<br />deparse_expression_pretty(node, NIL, false,
false,0, 0) <br />you can use :<br />deparse_expression(node, NIL, false, false)<br /><br />We are anyway not using
prettyprinting.<br />---<br />Other than this, I have no more issues.<br /><br />---<br />After the final review is
over,the catversion.h requires the appropriate date value.<br />><br />><br />><br />><br />> --<br
/>>Sent via pgsql-hackers mailing list (<a
href="mailto:pgsql-hackers@postgresql.org">pgsql-hackers@postgresql.org</a>)<br/> > To make changes to your
subscription:<br/>> <a
href="http://www.postgresql.org/mailpref/pgsql-hackers">http://www.postgresql.org/mailpref/pgsql-hackers</a><br
/>><br/><br />