Review: psql include file using relative path

From: Josh Kupershmidt <schmiddy(at)gmail(dot)com>
To: singh(dot)gurjeet(at)gmail(dot)com
Cc: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Review: psql include file using relative path
Date: 2011-05-14 21:03:32
Message-ID: BANLkTi=vAUov651uWOYgYaCv3VF4unP6NA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: 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

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Kevin Grittner 2011-05-14 21:11:40 Re: patch for new feature: Buffer Cache Hibernation
Previous Message Mitsuru IWASAKI 2011-05-14 18:58:07 Re: patch for new feature: Buffer Cache Hibernation