Re: information schema parameter_default implementation

From: Peter Eisentraut <peter_e(at)gmx(dot)net>
To: Amit Khandekar <amit(dot)khandekar(at)enterprisedb(dot)com>
Cc: Ali Dar <ali(dot)munir(dot)dar(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: information schema parameter_default implementation
Date: 2013-11-15 02:47:58
Message-ID: 1384483678.5008.1.camel@vanquo.pezone.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: 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

Attachment Content-Type Size
0001-Implement-information_schema.parameters.parameter_de.patch text/x-patch 10.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2013-11-15 02:55:16 Database disconnection and switch inside a single bgworker
Previous Message Michael Paquier 2013-11-15 02:40:17 REINDEX CONCURRENTLY 2.0