Re: psql :: support for \ev viewname and \sv viewname - Mailing list pgsql-hackers

The following review has been posted through the commitfest application:
make installcheck-world:  tested, failed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, failed

I have reviewed this patch. Most of the code is just rearranged.
Since this is based upon, \ef and \sf, code is almost similar.

However hare are my review comments:

1.
make failed with docs

2.
> \ev vw1 3

This syntax is supported. But documentation only says:
\ev [ viewname ] 
Missing optional line_number clause

3.
> strip_lineno_from_objdesc(char *func)

Can we have parameter name as obj instead of func.
You have renamed the function name, as it is now called in case of views as
well. Better rename the parameter names as well.

4.
Also please update the comments above strip_lineno_from_objdesc().
It is specific to functions which is not the case now.

5.
> print_with_linenumbers(FILE *output,
>                       char *lines,
>                       const char *header_cmp_keyword,
>                       size_t header_cmp_sz)

Can't we calculate the length of header (header_cmp_sz) inside function?
This will avoid any sloppy changes like, change in the keyword but forgot to
change the size.
Lets just accept the keyword and calculate the size within the function.

6.
>                 *
>                 * Note that this loop scribbles on func_buf.
>                 */

These lines at commands.c:1357, looks NO more valid now as there is NO loop
there.

7.
I see few comment lines explaining which is line 1 in case of function, for
which "AS " is used. Similarly, for view "SELECT " is used.
Can you add similar kind of explanation there?

8.
> get_create_object_cmd_internal
> get_create_function_cmd
> get_create_view_cmd

Can these three functions grouped together in just get_create_object_cmd(). 
This function will take an extra parameter to indicate the object type.
Say O_FUNC and O_VIEW for example.

For distinct part, just have a switch case over this type.

This will add a flexibility that if we add another such \e and \s options, we
don't need new functions, rather just need new enum like O_new and a new case
in this switch statement.
Also it will look good to read the code as well.

similarly you can do it for
> lookup_object_oid_internal
> get_create_function_cmd
> lookup_function_oid

9.
> static int count_lines_in_buf(PQExpBuffer buf)
> static void print_with_linenumbers(FILE *output, .. )
> static bool lookup_view_oid(const char *desc, Oid *view_oid)
> static bool lookup_object_oid_internal(PQExpBuffer query, Oid *obj_oid)

Can we have smaller description, explaining what's the function doing for
these functions at the definition?

10.
> +        "\\e", "\\echo", "\\ef", "\\ev", "\\encoding",

Can you keep this sorted?
It will be good if it sorted, but I see no such restriction as I see few out
of order options. But better keep it ordered.
Ignore if you dis-agree.


The new status of this patch is: Waiting on Author