> 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.
psql-ev-sv-support-v4.diff
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
