On Sat, May 21, 2011 at 11:59 AM, Josh Kupershmidt <[email protected]>wrote:
> On Fri, May 20, 2011 at 2:35 PM, Gurjeet Singh <[email protected]>
> wrote:
> > On Sat, May 14, 2011 at 5:03 PM, Josh Kupershmidt <[email protected]>
> > wrote:
> > Thanks a lot for the review. My responses are inline below.
>
> Thanks for the fixes. Your updated patch is sent as a
> patch-upon-a-patch, it'll probably be easier for everyone
> (particularly the final committer) if you send inclusive patches
> instead.
>
My Bad. I did not intend to do that.
>
> >> == 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.
> >
> > Done.
>
> This is a decent description from a technical standpoint:
>
> <para>
> When used within a script, if the <replaceable
> class="parameter">filename</replaceable>
> uses relative path notation, then the file will be looked up
> relative to currently
> executing file's location.
>
> If the <replaceable class="parameter">filename</replaceable>
> uses an absolute path
> notation, or if this command is being used in interactive
> mode, then it behaves the
> same as <literal>\i</> command.
> </para>
>
> but I think these paragraphs could be made a little more clear, by
> making a suggestion about why someone would be interested in \ir. How
> about this:
>
> <para>
> The <literal>\ir</> command is similar to <literal>\i</>, but
> is useful for files which
> load in other files.
>
> When used within a file loaded via <literal>\i</literal>,
> <literal>\ir</literal>, or
> <option>-f</option>, if the <replaceable
> class="parameter">filename</replaceable>
> is specified with a relative path, then the location of the
> file will be determined
> relative to the currently executing file's location.
> </para>
>
> <para>
> If <replaceable class="parameter">filename</replaceable> is given as
> an
> absolute path, or if this command is used in interactive mode, then
> <literal>\ir</> behaves the same as the <literal>\i</> command.
> </para>
>
> The sentence "When used within a file ..." is now a little
> clunky/verbose, but I was trying to avoid potential confusion from
> someone trying \ir via 'cat ../filename.sql | psql', which would be
> "used within a script", but \ir wouldn't know that.
>
Although a bit winded, I think that sounds much clearer.
>
>
> >> == 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;
> >> }
> >
> > Fixed.
>
> Well, this fix:
>
> if (!fd)
> {
> + if (relative_path != NULL)
> + free(relative_path);
> +
> psql_error("%s: %s\n", filename, strerror(errno));
>
> uses the wrong variable name (relative_path instead of relative_file),
> and the subsequent psql_error() call will then reference freed memory,
> since relative_file was assigned to filename.
>
> But even after fixing this snippet to get it to compile, like so:
>
> if (!fd)
> {
> psql_error("%s: %s\n", filename, strerror(errno));
> if (relative_file != NULL)
> free(relative_file);
>
> return EXIT_FAILURE;
> }
>
> I was still seeing memory leaks in valgrind growing with the number of
> \ir calls between files (see valgrind_bad_report.txt attached). I
> think that relative_file needs to be freed even in the non-error case,
> like so:
>
> error:
> if (fd != stdin)
> fclose(fd);
>
> if (relative_file != NULL)
> free(relative_file);
>
> pset.inputfile = oldfilename;
> return result;
>
> At least, that fix seemed to get rid of the ballooning valgrind
> reports for me. I then see a constant-sized < 500 byte leak complaint
> from valgrind, the same as with unpatched psql.
>
Yup, that fixes it. Thanks.
>
> >> 4.) I think the changes to process_file() merit another comment or
> >> two, e.g. describing what relative_file is supposed to be.
>
> > Added.
>
> Some cleanup for this comment:
>
> + /*
> + * If the currently processing file uses \ir command, and
> the parameter
> + * to the command is a relative file path, then we resolve
> this path
> + * relative to currently processing file.
>
> suggested tweak:
>
> If the currently processing file uses the \ir command, and the filename
> parameter is given as a relative path, then we resolve this path
> relative
> to the currently processing file (pset.inputfile).
>
> + *
> + * If the \ir command was executed in interactive mode
> (i.e. not in a
> + * script) the we treat it the same as \i command.
> + */
>
> suggested tweak:
>
> If the \ir command was executed in interactive mode (i.e. not in a
> script, and pset.inputfile will be NULL) then we treat the filename
> the same as the \i command does.
>
Tweaks applied, but omitted the C variable names as I don't think that adds
much value.
New version of the patch attached. Thanks for the review.
--
Gurjeet Singh
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index eaf901d..9bd6d93 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1626,6 +1626,30 @@ Tue Oct 26 21:40:57 CEST 1999
<varlistentry>
+ <term><literal>\ir <replaceable class="parameter">filename</replaceable></literal></term>
+ <listitem>
+ <para>
+ The <literal>\ir</> command is similar to <literal>\i</>, but is useful for files which
+ include and execute other files.
+ </para>
+
+ <para>
+ When used within a file loaded via <literal>\i</literal>, <literal>\ir</literal>, or
+ <option>-f</option>, if the <replaceable class="parameter">filename</replaceable>
+ is specified using a relative path, then the location of the file will be determined
+ relative to the currently executing file's location.
+ </para>
+
+ <para>
+ If <replaceable class="parameter">filename</replaceable> is given as an absolute path,
+ or if this command is used in interactive mode, then <literal>\ir</> behaves the same
+ as the <literal>\i</> command.
+ </para>
+ </listitem>
+ </varlistentry>
+
+
+ <varlistentry>
<term><literal>\l</literal> (or <literal>\list</literal>)</term>
<term><literal>\l+</literal> (or <literal>\list+</literal>)</term>
<listitem>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 378330b..93a7fd2 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -785,10 +785,15 @@ exec_command(const char *cmd,
/* \i is include file */
- else if (strcmp(cmd, "i") == 0 || strcmp(cmd, "include") == 0)
+ /* \ir is include file relative to file currently being processed */
+ else if (strcmp(cmd, "i") == 0 || strcmp(cmd, "include") == 0
+ || strcmp(cmd, "ir") == 0 || strcmp(cmd, "include_relative") == 0)
{
- char *fname = psql_scan_slash_option(scan_state,
- OT_NORMAL, NULL, true);
+ char *fname = psql_scan_slash_option(scan_state,
+ OT_NORMAL, NULL, true);
+
+ bool include_relative = (strcmp(cmd, "ir") == 0
+ || strcmp(cmd, "include_relative") == 0);
if (!fname)
{
@@ -798,7 +803,7 @@ exec_command(const char *cmd,
else
{
expand_tilde(&fname);
- success = (process_file(fname, false) == EXIT_SUCCESS);
+ success = (process_file(fname, false, include_relative) == EXIT_SUCCESS);
free(fname);
}
}
@@ -1969,15 +1974,19 @@ do_edit(const char *filename_arg, PQExpBuffer query_buf,
* process_file
*
* Read commands from filename and then them to the main processing loop
- * Handler for \i, but can be used for other things as well. Returns
+ * Handler for \i and \ir, but can be used for other things as well. Returns
* MainLoop() error code.
+ *
+ * If use_relative_path is true and filename is not an absolute path, then open
+ * the file from where the currently processed file (if any) is located.
*/
int
-process_file(char *filename, bool single_txn)
+process_file(char *filename, bool single_txn, bool use_relative_path)
{
FILE *fd;
int result;
char *oldfilename;
+ char *relative_file = NULL;
PGresult *res;
if (!filename)
@@ -1986,6 +1995,39 @@ process_file(char *filename, bool single_txn)
if (strcmp(filename, "-") != 0)
{
canonicalize_path(filename);
+
+ /*
+ * If the currently processing file uses \ir command, and the 'filename'
+ * parameter to the command is given as a relative file path, then we
+ * resolve this path relative to currently processing file.
+ *
+ * If the \ir command was executed in interactive mode (i.e. not via \i,
+ * \ir or -f) then we treat it the same as \i command.
+ */
+ if (use_relative_path && pset.inputfile)
+ {
+ char *last_slash;
+
+ /* find the / that splits the file from its path */
+ last_slash = strrchr(pset.inputfile, '/');
+
+ if (last_slash && !is_absolute_path(filename))
+ {
+ size_t dir_len = (last_slash - pset.inputfile) + 1;
+ size_t file_len = strlen(filename);
+
+ relative_file = pg_malloc(dir_len + 1 + file_len + 1);
+
+ relative_file[0] = '\0';
+ strncat(relative_file, pset.inputfile, dir_len);
+ strcat(relative_file, filename);
+
+ canonicalize_path(relative_file);
+
+ filename = relative_file;
+ }
+ }
+
fd = fopen(filename, PG_BINARY_R);
}
else
@@ -1993,6 +2035,9 @@ process_file(char *filename, bool single_txn)
if (!fd)
{
+ if (relative_file != NULL)
+ free(relative_file);
+
psql_error("%s: %s\n", filename, strerror(errno));
return EXIT_FAILURE;
}
@@ -2034,6 +2079,9 @@ error:
if (fd != stdin)
fclose(fd);
+ if (relative_file != NULL)
+ free(relative_file);
+
pset.inputfile = oldfilename;
return result;
}
diff --git a/src/bin/psql/command.h b/src/bin/psql/command.h
index 852d645..9d0c31c 100644
--- a/src/bin/psql/command.h
+++ b/src/bin/psql/command.h
@@ -27,7 +27,7 @@ typedef enum _backslashResult
extern backslashResult HandleSlashCmds(PsqlScanState scan_state,
PQExpBuffer query_buf);
-extern int process_file(char *filename, bool single_txn);
+extern int process_file(char *filename, bool single_txn, bool use_relative_path);
extern bool do_pset(const char *param,
const char *value,
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index ac5edca..d459934 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -184,6 +184,7 @@ slashUsage(unsigned short int pager)
fprintf(output, _(" \\copy ... perform SQL COPY with data stream to the client host\n"));
fprintf(output, _(" \\echo [STRING] write string to standard output\n"));
fprintf(output, _(" \\i FILE execute commands from file\n"));
+ fprintf(output, _(" \\ir FILE execute commands from FILE, placed relative to currently processing file\n"));
fprintf(output, _(" \\o [FILE] send all query results to file or |pipe\n"));
fprintf(output, _(" \\qecho [STRING] write string to query output stream (see \\o)\n"));
fprintf(output, "\n");
diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h
index 7228f9d..2e28d86 100644
--- a/src/bin/psql/settings.h
+++ b/src/bin/psql/settings.h
@@ -81,7 +81,7 @@ typedef struct _psqlSettings
bool cur_cmd_interactive;
int sversion; /* backend server version */
const char *progname; /* in case you renamed psql */
- char *inputfile; /* for error reporting */
+ char *inputfile; /* File being currently processed, if any */
char *dirname; /* current directory for \s display */
uint64 lineno; /* also for error reporting */
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 7b8078c..3c17eec 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -256,7 +256,7 @@ main(int argc, char *argv[])
if (!options.no_psqlrc)
process_psqlrc(argv[0]);
- successResult = process_file(options.action_string, options.single_txn);
+ successResult = process_file(options.action_string, options.single_txn, false);
}
/*
@@ -604,9 +604,9 @@ process_psqlrc_file(char *filename)
sprintf(psqlrc, "%s-%s", filename, PG_VERSION);
if (access(psqlrc, R_OK) == 0)
- (void) process_file(psqlrc, false);
+ (void) process_file(psqlrc, false, false);
else if (access(filename, R_OK) == 0)
- (void) process_file(filename, false);
+ (void) process_file(filename, false, false);
free(psqlrc);
}
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 9a7eca0..b86b0f0 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -735,7 +735,7 @@ psql_completion(char *text, int start, int end)
"\\dF", "\\dFd", "\\dFp", "\\dFt", "\\dg", "\\di", "\\dl", "\\dL",
"\\dn", "\\do", "\\dp", "\\drds", "\\ds", "\\dS", "\\dt", "\\dT", "\\dv", "\\du",
"\\e", "\\echo", "\\ef", "\\encoding",
- "\\f", "\\g", "\\h", "\\help", "\\H", "\\i", "\\l",
+ "\\f", "\\g", "\\h", "\\help", "\\H", "\\i", "\\ir", "\\l",
"\\lo_import", "\\lo_export", "\\lo_list", "\\lo_unlink",
"\\o", "\\p", "\\password", "\\prompt", "\\pset", "\\q", "\\qecho", "\\r",
"\\set", "\\sf", "\\t", "\\T",
@@ -2874,6 +2874,7 @@ psql_completion(char *text, int start, int end)
strcmp(prev_wd, "\\e") == 0 || strcmp(prev_wd, "\\edit") == 0 ||
strcmp(prev_wd, "\\g") == 0 ||
strcmp(prev_wd, "\\i") == 0 || strcmp(prev_wd, "\\include") == 0 ||
+ strcmp(prev_wd, "\\ir") == 0 || strcmp(prev_wd, "\\include_relative") == 0 ||
strcmp(prev_wd, "\\o") == 0 || strcmp(prev_wd, "\\out") == 0 ||
strcmp(prev_wd, "\\s") == 0 ||
strcmp(prev_wd, "\\w") == 0 || strcmp(prev_wd, "\\write") == 0
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
