Re: psql :: support for \ev viewname and \sv viewname

From: Petr Korobeinikov <pkorobeinikov(at)gmail(dot)com>
To: Jeevan Chalke <jeevan(dot)chalke(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: psql :: support for \ev viewname and \sv viewname
Date: 2015-06-06 19:59:21
Message-ID: CAJL5ff_PTEt0qq26+mVF-psGj=6dUBeECoVb0sYdErJ0rHXpGg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> 1.
> make failed with docs
>
Fixed.

> 2.
> > \ev vw1 3
>
> This syntax is supported. But documentation only says:
> \ev [ viewname ]
> Missing optional line_number clause
>
Fixed. Documented.

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.

Renamed.

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

> 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.
>
Now header_cmp_sz calculated inside 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.
>
Removed.

>
> 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?
>
Explanation added.

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
>
Reworked.
New enum PgObjType introduced.

>
> 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?
>
Description added.

>
> 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.
>
Hmm, sorted now.
Sort is based on my feelings.

Attachment Content-Type Size
psql-ev-sv-support-v4.diff application/octet-stream 23.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2015-06-06 20:29:57 Re: Restore-reliability mode
Previous Message Noah Misch 2015-06-06 19:58:05 Re: Restore-reliability mode