I had a chance to give this patch a look. This review is of the second
patch posted by Gurjeet, at:
http://archives.postgresql.org/message-id/AANLkTi=yjb_a+ggt_pxmrqhbhyid6aswwb8h-lw-k...@mail.gmail.com
== Summary ==
This patch implements the \ir command for psql, with a long alias
\include_relative. This new backslash command is similar to the
existing \i or \include command, but it allows loading .sql files with
paths in reference to the currently-executing script's directory, not
the CWD of where psql is called from. This feature would be used when
one .sql file needs to load another .sql file in a related directory.
== Utility ==
I would find the \ir command useful for constructing small packages of
SQL to be loaded together. For example, I keep the DDL for non-trivial
databases in source control, often broken down into one file or more
per schema, with a master file loading in all the sub-files; this
command would help the master file be sure it's referencing the
locations of other files correctly.
== General ==
The patch applies cleanly to HEAD. No regression tests are included,
but I don't think they're needed here.
== Documentation ==
The patch includes the standard psql help output description for the
new \ir command. I think ./doc/src/sgml/ref/psql-ref.sgml needs to be
patched as well, though.
Tangent: AFAICT we're not documenting the long form of psql commands,
such as \print, anywhere. Following that precedent, this patch doesn't
document \include_relative. Not sure if we want to document such
options anywhere, but in any case a separate issue from this patch.
== Code ==
1.) I have some doubts about whether the memory allocated here:
char *relative_file = pg_malloc(dir_len + 1 + file_len + 1);
is always free()'d, particularly if this condition is hit:
if (!fd)
{
psql_error("%s: %s\n", filename, strerror(errno));
return EXIT_FAILURE;
}
2.) This comment should mention \ir
* Handler for \i, but can be used for other things as well. ...
3.) settings.h has the comment about pset.inputfile :
char *inputfile; /* for error reporting */
But this variable is use for more than just "error reporting" now
(i.e. determining whether psql is executing a file).
4.) I think the changes to process_file() merit another comment or
two, e.g. describing what relative_file is supposed to be.
5.) I tried the patch out on Linux and OS X; perhaps someone should
give it a quick check on Windows as well -- I'm not sure if pathname
manipulations like:
last_slash = strrchr(pset.inputfile, '/');
work OK on Windows.
6.) The indentation of these lines in tab-complete.c around line 2876 looks off:
strcmp(prev_wd, "\\i") == 0 || strcmp(prev_wd, "\\include") == 0 ||
strcmp(prev_wd, "\\ir") == 0 || strcmp(prev_wd,
"\\include_relative") == 0 ||
(I think the first of those lines was off before the patch, and the
patch followed its example)
That's it for now. Overall, I think this patch provides a useful
feature, and am hoping it could be ready for commit in 9.2 in the
2011-next commitfest with some polishing.
Josh
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
