Re: information schema parameter_default implementation

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

On Wed, Jan 9, 2013 at 4:28 PM, Peter Eisentraut <peter_e(at)gmx(dot)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(at)postgresql(dot)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

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(dot)munir(dot)dar(at)gmail(dot)com> wrote:

> On Wed, Jan 9, 2013 at 4:28 PM, Peter Eisentraut <peter_e(at)gmx(dot)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(at)postgresql(dot)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
>

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.

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

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
>

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
>

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.

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
>

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

Peter Eisentraut <peter_e(at)gmx(dot)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

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

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(at)gmx(dot)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(at)postgresql(dot)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(dot)campero(at)anachronics(dot)com
http://www.anachronics.com

2013/11/20 Rodolfo Campero <rodolfo(dot)campero(at)anachronics(dot)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 [...]

Updated patch

2013/11/20 Peter Eisentraut <peter_e(at)gmx(dot)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

On 11/20/13, 8:39 PM, Rodolfo Campero wrote:
> 2013/11/20 Peter Eisentraut <peter_e(at)gmx(dot)net <mailto:peter_e(at)gmx(dot)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.

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(at)gmx(dot)net>

> On 11/20/13, 8:39 PM, Rodolfo Campero wrote:
> > 2013/11/20 Peter Eisentraut <peter_e(at)gmx(dot)net <mailto:peter_e(at)gmx(dot)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(dot)campero(at)anachronics(dot)com
http://www.anachronics.com