Thread: Review: psql include file using relative path

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

On Sat, May 14, 2011 at 5:03 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote:
> 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

Cool.  I see you (or someone) has added this to the entry for that
patch on commitfest.postgresql.org as well, which is great.  I have
updated that entry to list you as the reviewer and changed the status
of the patch to "Waiting on Author" pending resolution of the issues
you observed.

> == General  ==
> The patch applies cleanly to HEAD. No regression tests are included,
> but I don't think they're needed here.

I agree.

> == 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.

I agree with this too.

> 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.

And this.

[...snip...]
> 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.

Depends if canonicalize_path() has already been applied to that path.

> 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)

pgindent likes to move things backward to make them fit within 80 columns.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

<div dir="ltr"><div class="gmail_quote">On Tue, May 17, 2011 at 2:43 PM, Robert Haas <span dir="ltr"><<a
href="mailto:robertmhaas@gmail.com">robertmhaas@gmail.com</a>></span>wrote:<br /><blockquote class="gmail_quote"
style="margin:0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div class="im">On Sat,
May14, 2011 at 5:03 PM, Josh Kupershmidt <<a href="mailto:schmiddy@gmail.com">schmiddy@gmail.com</a>> wrote:<br
/>> I had a chance to give this patch a look. This review is of the second<br /> > patch posted by Gurjeet,
at:<br/> > <a
href="http://archives.postgresql.org/message-id/AANLkTi=YJb_A+GGt_pXmRqhBhyiD6aSWWB8h-Lw-KVi0@mail.gmail.com"
target="_blank">http://archives.postgresql.org/message-id/AANLkTi=YJb_A+GGt_pXmRqhBhyiD6aSWWB8h-Lw-KVi0@mail.gmail.com</a><br
/><br/></div>Cool.  I see you (or someone) has added this to the entry for that<br /> patch on <a
href="http://commitfest.postgresql.org"target="_blank">commitfest.postgresql.org</a> as well, which is great.  I
have<br/> updated that entry to list you as the reviewer and changed the status<br /> of the patch to "Waiting on
Author"pending resolution of the issues<br /> you observed.<br /><div class="im"><br /></div></blockquote></div><br
/>Well,that was added to commitfest about 3 months ago, which is great because somebody reviewed it without me having
toresubmit it.<br /><br />New patch submitted and marked as 'Needs Review'.<br /><br />Thanks,<br />-- <br /><div
dir="ltr">GurjeetSingh<br />EnterpriseDB Corporation<br />The Enterprise PostgreSQL Company<br /></div><br /></div> 

On Fri, May 20, 2011 at 2:35 PM, Gurjeet Singh <singh.gurjeet@gmail.com> wrote:
> On Sat, May 14, 2011 at 5:03 PM, Josh Kupershmidt <schmiddy@gmail.com>
> 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.

>> == 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
thiscommand 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.


>> == 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.

>> 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,
thenwe 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
wetreat the filename   the same as the \i command does.
 

[snip]
The rest looks good.

Josh

On Sun, Jun 5, 2011 at 10:21 AM, Gurjeet Singh <singh.gurjeet@gmail.com> wrote:
> On Sat, May 21, 2011 at 11:59 AM, Josh Kupershmidt <schmiddy@gmail.com>
> wrote:

> Tweaks applied, but omitted the C variable names as I don't think that adds
> much value.

Your rewordings are fine, but the the article "the" is missing in a
few spots, e.g.* "uses \ir command" -> "uses the \ir command"* "to currently processing file" -> "to the currently
processingfile"* "same as \i command" -> "same as the \i command"
 

I think "processing" is better (and consistent with the rest of the
comments) than "processed" here:
+ * the file from where the currently processed file (if any) is located.

> New version of the patch attached. Thanks for the review.

I think the patch is in pretty good shape now. The memory leak is gone
AFAICT, and the comments and documentation updates look good.

Josh

On Sun, Jun 5, 2011 at 8:16 PM, Gurjeet Singh <singh.gurjeet@gmail.com> wrote:
> Attached an updated patch.
>
> If you find it ready for committer, please mark it so in the commitfest app.

I can't find anything further to nitpick with this patch, and have
marked it Ready For Committer in the CF. Thanks for your work on this,
I am looking forward to the feature.

Josh

On Mon, Jun 6, 2011 at 10:11 PM, Gurjeet Singh <singh.gurjeet@gmail.com> wrote:
> On Mon, Jun 6, 2011 at 9:48 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote:
>>
>> On Sun, Jun 5, 2011 at 8:16 PM, Gurjeet Singh <singh.gurjeet@gmail.com>
>> wrote:
>> > Attached an updated patch.
>> >
>> > If you find it ready for committer, please mark it so in the commitfest
>> > app.
>>
>> I can't find anything further to nitpick with this patch, and have
>> marked it Ready For Committer in the CF. Thanks for your work on this,
>> I am looking forward to the feature.
>
> Thanks for your reviews and perseverance :)

I committed this after substantial further revisions:

- I rewrote the changes to process_file() to use the pathname-handling
functions in src/port, rather custom code.  Along the way, relpath
became a constant-size buffer, which should be OK since
join_pathname_components() knows about MAXPGPATH.  This has what I
consider to be a useful side effect of not calling pg_malloc() here,
which means we don't have to remember to free the memory.

- I added a safeguard against someone doing something like "\ir E:foo"
on Windows.  Although that's not an absolute path, for purposes of \ir
it needs to be treated as one.  I don't have a Windows build
environment handy so someone may want to test that I haven't muffed
this.

- I rewrote the documentation and a number of the comments to be (I
hope) more clear.

- I reverted some unnecessary whitespace changes in exec_command().

- As proposed, the patch declared process_file with a non-constant
initialized and then declared another variable after that.  I believe
some old compilers will barf on that.  Since it isn't needed in that
block anyway, I moved it to an inner block.

- I incremented the pager line count for psql's help.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company