Thread

1. information schema parameter_default implementation

   Peter Eisentraut <peter_e@gmx.net> — 2013-01-09T11:28:32Z

   Here is an implementation of the
   information_schema.parameters.parameter_default column.
   
   I ended up writing a C function to decode the whole thing from the
   system catalogs, because it was too complicated in SQL, so I abandoned
   the approach discussed in [0].
   
   
   [0]: http://archives.postgresql.org/message-id/1356092400.25658.6.camel@vanquo.pezone.net
   

2. Re: information schema parameter_default implementation

   Ali Dar <ali.munir.dar@gmail.com> — 2013-01-31T13:59:01Z

   On Wed, Jan 9, 2013 at 4:28 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
   
   > Here is an implementation of the
   > information_schema.parameters.parameter_default column.
   >
   > I ended up writing a C function to decode the whole thing from the
   > system catalogs, because it was too complicated in SQL, so I abandoned
   > the approach discussed in [0].
   >
   >
   > [0]:
   > http://archives.postgresql.org/message-id/1356092400.25658.6.camel@vanquo.pezone.net
   >
   >
   > --
   > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
   > To make changes to your subscription:
   > http://www.postgresql.org/mailpref/pgsql-hackers
   >
   >
   I checked our your patch. There seems to be an issue when we have OUT
   parameters after the DEFAULT values. For example a simple test case is
   given below:
   
   postgres=# CREATE FUNCTION functest1(a int default 1, out b int)
   postgres-#     RETURNS int
   postgres-#     LANGUAGE SQL
   postgres-#     AS 'SELECT $1';
   CREATE FUNCTION
   postgres=#
   postgres=# SELECT ordinal_position, parameter_name, parameter_default FROM
   information_schema.parameters WHERE  specific_name LIKE 'functest%' ORDER
   BY 1;
    ordinal_position | parameter_name | parameter_default
   ------------------+----------------+-------------------
                   1 | a              | 1
                   2 | b              | 1
   (2 rows)
   
   The out parameters gets the same value as the the last default parameter.
   The patch work only when default values are at the end. Switch the
   parameters and it starts working(make OUT parameter as first and default
   one the last one). Below is the example:
   
   postgres=# CREATE FUNCTION functest1(out a int, b int default 1)
   postgres-#     RETURNS int
   postgres-#     LANGUAGE SQL
   postgres-#     AS 'SELECT $1';
   CREATE FUNCTION
   postgres=# SELECT ordinal_position, parameter_name, parameter_default FROM
   information_schema.parameters WHERE  specific_name LIKE 'functest%' ORDER
   BY 1;
    ordinal_position | parameter_name | parameter_default
   ------------------+----------------+-------------------
                   1 | a              |
                   2 | b              | 1
   (2 rows)
   
   
   Some other minor observations:
   1) Some variables are not lined in pg_get_function_arg_default().
   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)
   2) inputargn can be assigned in declaration.
   3) Function level comment for pg_get_function_arg_default() is missing.
   4) You can also add comments inside the function, for example the comment
   for the line:
   nth = inputargn - 1 - (proc->pronargs - proc->pronargdefaults);
   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"?
   
   Regards,
   
   Ali Dar
   

3. Re: information schema parameter_default implementation

   Ali Dar <ali.munir.dar@gmail.com> — 2013-01-31T14:02:43Z

   Another thing I forget: The patch does not apply because of the changes in
   "catversion.h"
   
   Regards,
   Ali Dar
   
   
   On Thu, Jan 31, 2013 at 6:59 PM, Ali Dar <ali.munir.dar@gmail.com> wrote:
   
   > On Wed, Jan 9, 2013 at 4:28 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
   >
   >> Here is an implementation of the
   >> information_schema.parameters.parameter_default column.
   >>
   >> I ended up writing a C function to decode the whole thing from the
   >> system catalogs, because it was too complicated in SQL, so I abandoned
   >> the approach discussed in [0].
   >>
   >>
   >> [0]:
   >> http://archives.postgresql.org/message-id/1356092400.25658.6.camel@vanquo.pezone.net
   >>
   >>
   >> --
   >> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
   >> To make changes to your subscription:
   >> http://www.postgresql.org/mailpref/pgsql-hackers
   >>
   >>
   > I checked our your patch. There seems to be an issue when we have OUT
   > parameters after the DEFAULT values. For example a simple test case is
   > given below:
   >
   > postgres=# CREATE FUNCTION functest1(a int default 1, out b int)
   > postgres-#     RETURNS int
   > postgres-#     LANGUAGE SQL
   > postgres-#     AS 'SELECT $1';
   > CREATE FUNCTION
   > postgres=#
   > postgres=# SELECT ordinal_position, parameter_name, parameter_default FROM
   > information_schema.parameters WHERE  specific_name LIKE 'functest%' ORDER
   > BY 1;
   >  ordinal_position | parameter_name | parameter_default
   > ------------------+----------------+-------------------
   >                 1 | a              | 1
   >                 2 | b              | 1
   > (2 rows)
   >
   > The out parameters gets the same value as the the last default parameter.
   > The patch work only when default values are at the end. Switch the
   > parameters and it starts working(make OUT parameter as first and default
   > one the last one). Below is the example:
   >
   > postgres=# CREATE FUNCTION functest1(out a int, b int default 1)
   > postgres-#     RETURNS int
   > postgres-#     LANGUAGE SQL
   > postgres-#     AS 'SELECT $1';
   > CREATE FUNCTION
   > postgres=# SELECT ordinal_position, parameter_name, parameter_default FROM
   > information_schema.parameters WHERE  specific_name LIKE 'functest%' ORDER
   > BY 1;
   >  ordinal_position | parameter_name | parameter_default
   > ------------------+----------------+-------------------
   >                 1 | a              |
   >                 2 | b              | 1
   > (2 rows)
   >
   >
   > Some other minor observations:
   > 1) Some variables are not lined in pg_get_function_arg_default().
   > 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)
   > 2) inputargn can be assigned in declaration.
   > 3) Function level comment for pg_get_function_arg_default() is missing.
   > 4) You can also add comments inside the function, for example the comment
   > for the line:
   > nth = inputargn - 1 - (proc->pronargs - proc->pronargdefaults);
   > 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"?
   >
   > Regards,
   >
   > Ali Dar
   >
   

4. Re: information schema parameter_default implementation

   Peter Eisentraut <peter_e@gmx.net> — 2013-09-14T20:05:25Z

   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.
   
   
   

5. Re: information schema parameter_default implementation

   Amit Khandekar <amit.khandekar@enterprisedb.com> — 2013-09-18T14:43:02Z

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

6. information schema parameter_default implementation

   Amit Khandekar <amit.khandekar@enterprisedb.com> — 2013-09-19T12:56:17Z

   On 15 September 2013 01:35, Peter Eisentraut <peter_e@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
   >
   

7. information schema parameter_default implementation

   Amit Khandekar <amit.khandekar@enterprisedb.com> — 2013-09-28T13:25:57Z

   On 15 September 2013 01:35, Peter Eisentraut <peter_e@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
   >
   

8. Re: information schema parameter_default implementation

   Peter Eisentraut <peter_e@gmx.net> — 2013-10-02T00:59:34Z

   On Wed, 2013-09-18 at 20:13 +0530, Amit Khandekar wrote:
   > 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. 
   
   No, the pg_proc entry belongs to a function for which the current user
   is the owner *or* has EXECUTE privilege.  The default, however, is only
   shown to the owner.  This is per SQL standard.
   
   
   
   
   

9. information schema parameter_default implementation

   Amit Khandekar <amit.khandekar@enterprisedb.com> — 2013-11-09T06:39:41Z

   On 15 September 2013 01:35, Peter Eisentraut <peter_e@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
   >
   

10. Re: information schema parameter_default implementation

    Peter Eisentraut <peter_e@gmx.net> — 2013-11-15T02:47:58Z

    Updated patch attached.
    
    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)
    > 
    done
    
    

11. Re: information schema parameter_default implementation

    Tom Lane <tgl@sss.pgh.pa.us> — 2013-11-17T21:38:45Z

    Peter Eisentraut <peter_e@gmx.net> writes:
    > [ 0001-Implement-information_schema.parameters.parameter_de.patch ]
    
    I'm a bit confused as to where this column is coming from?  There's
    no such thing in SQL:2008 as far as I can see.  If it's coming from
    some not-yet-ratified draft, maybe we should wait for ratification.
    It's impossible for a bystander to tell if this implementation conforms
    to what the spec is expecting.
    
    BTW, although SQL:2008 lacks this column in the parameters view, there
    are about six columns it has that we don't: see the from_sql_xxx and
    to_sql_xxx columns.  Surely we should put those in (at least as dummy
    columns) before trying to claim adherence to some even-newer spec draft.
    
    As far as the code goes, I have no particular objections, modulo the
    question about whether this patch is implementing spec-compatible
    behavior.  A small stylistic idea is that maybe the computation of
    nth_inputarg should be moved down nearer where it's used.  Really
    that's just part of the calculation of nth_default, and it wouldn't
    be unreasonable to stick it under the comment explaining why we're
    doing that calculation like that.
    
    			regards, tom lane
    
    
    

12. Re: information schema parameter_default implementation

    Peter Eisentraut <peter_e@gmx.net> — 2013-11-18T03:29:41Z

    On Sun, 2013-11-17 at 16:38 -0500, Tom Lane wrote:
    > I'm a bit confused as to where this column is coming from?  There's
    > no such thing in SQL:2008 as far as I can see.
    
    SQL:2011
    
    
    
    

13. Re: information schema parameter_default implementation

    Rodolfo Campero <rodolfo.campero@anachronics.com> — 2013-11-20T11:53:38Z

    Peter,
    
    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).
    
    Regards,
    
    
    2013/11/14 Peter Eisentraut <peter_e@gmx.net>
    
    > Updated patch attached.
    >
    > 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)
    > >
    > done
    >
    >
    >
    > --
    > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
    > To make changes to your subscription:
    > http://www.postgresql.org/mailpref/pgsql-hackers
    >
    >
    
    
    -- 
    Rodolfo Campero
    Anachronics S.R.L.
    Tel: (54 11) 4899 2088
    rodolfo.campero@anachronics.com
    http://www.anachronics.com
    

14. Re: information schema parameter_default implementation

    Rodolfo Campero <rodolfo.campero@anachronics.com> — 2013-11-20T12:30:13Z

    2013/11/20 Rodolfo Campero <rodolfo.campero@anachronics.com>
    
    > Peter,
    >
    > 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).
    >
    >
    Make fails:
    [...]
    make -C catalog schemapg.h
    make[3]: se ingresa al directorio
    `/home/rodolfo/trabajo/postgresql/src/backend/catalog'
    cd ../../../src/include/catalog && '/usr/bin/perl' ./duplicate_oids
    3846
    make[3]: *** [postgres.bki] Error 1
    [...]
    
    OID = 3846 is duplicated, see:
    
    ./src/include/catalog/pg_proc.h:1976:DATA(insert OID = 3846 (
     pg_get_function_arg_default [...]
    ./src/include/catalog/pg_proc.h:4679:DATA(insert OID = 3846 (
    make_date [...]
    

15. Re: information schema parameter_default implementation

    Peter Eisentraut <peter_e@gmx.net> — 2013-11-20T13:33:06Z

    Updated patch
    

16. Re: information schema parameter_default implementation

    Rodolfo Campero <rodolfo.campero@anachronics.com> — 2013-11-21T01:39:49Z

    2013/11/20 Peter Eisentraut <peter_e@gmx.net>
    
    > Updated patch
    >
    
    I can't apply the patch; maybe I'm doing something wrong?
    
    $ git apply v2-0001-Implement-information_schema.parameters.parameter.patch
    v2-0001-Implement-information_schema.parameters.parameter.patch:49:
    trailing whitespace.
               CAST((ss.x).n AS sql_identifier) AS dtd_identifier,
    v2-0001-Implement-information_schema.parameters.parameter.patch:50:
    trailing whitespace.
               CAST(
    v2-0001-Implement-information_schema.parameters.parameter.patch:51:
    trailing whitespace.
                 CASE WHEN pg_has_role(proowner, 'USAGE')
    v2-0001-Implement-information_schema.parameters.parameter.patch:52:
    trailing whitespace.
                      THEN pg_get_function_arg_default(p_oid, (ss.x).n)
    v2-0001-Implement-information_schema.parameters.parameter.patch:53:
    trailing whitespace.
                      ELSE NULL END
    error: patch failed: doc/src/sgml/information_schema.sgml:3323
    error: doc/src/sgml/information_schema.sgml: patch does not apply
    error: patch failed: src/backend/catalog/information_schema.sql:1133
    error: src/backend/catalog/information_schema.sql: patch does not apply
    error: patch failed: src/backend/utils/adt/ruleutils.c:2266
    error: src/backend/utils/adt/ruleutils.c: patch does not apply
    error: patch failed: src/include/catalog/catversion.h:53
    error: src/include/catalog/catversion.h: patch does not apply
    error: patch failed: src/include/catalog/pg_proc.h:1973
    error: src/include/catalog/pg_proc.h: patch does not apply
    error: patch failed: src/include/utils/builtins.h:665
    error: src/include/utils/builtins.h: patch does not apply
    error: patch failed: src/test/regress/expected/create_function_3.out:425
    error: src/test/regress/expected/create_function_3.out: patch does not apply
    error: patch failed: src/test/regress/sql/create_function_3.sql:138
    error: src/test/regress/sql/create_function_3.sql: patch does not apply
    

17. Re: information schema parameter_default implementation

    Peter Eisentraut <peter_e@gmx.net> — 2013-11-21T22:15:21Z

    On 11/20/13, 8:39 PM, Rodolfo Campero wrote:
    > 2013/11/20 Peter Eisentraut <peter_e@gmx.net <mailto:peter_e@gmx.net>>
    > 
    >     Updated patch
    > 
    > 
    > I can't apply the patch; maybe I'm doing something wrong?
    
    It looks like you are not in the right directory.
    
    
    
    

18. Re: information schema parameter_default implementation

    Rodolfo Campero <rodolfo.campero@anachronics.com> — 2013-11-22T12:37:25Z

    Review: information schema parameter_default implementation (v2)
    
    This is a review of the patch submitted in
    http://archives.postgresql.org/message-id/1384483678.5008.1.camel@vanquo.pezone.net
    (information schema parameter_default implementation).
    
    Previous review from Amit Khandekar covers technical aspects:
    http://www.postgresql.org/message-id/CACoZds0eB3-yZ+pVLp9Sf5Xs_9xsDZ=JdP1D83RA-hjvJJofiA@mail.gmail.com
    
    Submission review
    =================
     * Is the patch in a patch format which has context? (eg: context diff
    format)
    Yes
    
     * Does it apply cleanly to the current git master?
    I had to apply "fromdos" to remove trailing whitespace.
    After that, the patch applies cleanly to HEAD.
    Make builds without warnings, except for:
    
    In file included from gram.y:13675:0:
    scan.c: In function ‘yy_try_NUL_trans’:
    scan.c:10185:23: warning: unused variable ‘yyg’ [-Wunused-variable]
    
    but from previous messages in this mailing list I think that's unrelated to
    this patch and normal.
    The regression tests all pass successfully against the new patch.
    
     * Does it include reasonable tests, necessary doc patches, etc?
    Yes
    
    Usability review
    ================
     * Does the patch actually implement that?
    The patch implements the column "parameter_default" of information schema
    view "parameters", defined in the SQL:2011 standard.
    I could not get a hand to the spec, but I found a document where it is
    mentioned: http://msdn.microsoft.com/en-us/library/jj191733(v=sql.105).aspx
    
     * Do we want that?
    I think we do, as it is defined in the spec.
    
     * Do we already have it?
    No
    
     * Does it follow SQL spec, or the community-agreed behavior?
    SQL:2011.
    
     * Does it include pg_dump support (if applicable)?
    N/A
    
     * Are there dangers?
    None AFAICS.
    
     * Have all the bases been covered?
    Yes.
    
    Feature test
    ============
     * Does the feature work as advertised?
    Yes
    
     * Are there corner cases the author has failed to consider?
    None that I can see.
    
     * Are there any assertion failures or crashes?
    No
    
    Performance review
    ==================
    N/A
    
    Coding review
    =============
    I'm not skilled enough to do a code review; see previous review from Amit:
    http://www.postgresql.org/message-id/CACoZds0eB3-yZ+pVLp9Sf5Xs_9xsDZ=JdP1D83RA-hjvJJofiA@mail.gmail.com
    
    
    
    2013/11/21 Peter Eisentraut <peter_e@gmx.net>
    
    > On 11/20/13, 8:39 PM, Rodolfo Campero wrote:
    > > 2013/11/20 Peter Eisentraut <peter_e@gmx.net <mailto:peter_e@gmx.net>>
    > >
    > >     Updated patch
    > >
    > >
    > > I can't apply the patch; maybe I'm doing something wrong?
    >
    > It looks like you are not in the right directory.
    >
    >
    
    
    -- 
    Rodolfo Campero
    Anachronics S.R.L.
    Tel: (54 11) 4899 2088
    rodolfo.campero@anachronics.com
    http://www.anachronics.com