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 |
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 |