Re: information schema parameter_default implementation

From: Peter Eisentraut <peter_e(at)gmx(dot)net>
To: Ali Dar <ali(dot)munir(dot)dar(at)gmail(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: information schema parameter_default implementation
Date: 2013-09-14 20:05:25
Message-ID: 1379189125.19286.25.camel@vanquo.pezone.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Dimitri Fontaine 2013-09-14 20:15:58 Where to load modules from?
Previous Message Dimitri Fontaine 2013-09-14 20:04:48 PL Code Archive Proposal