Review: psql include file using relative path - Mailing list pgsql-hackers

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