Review: psql include file using relative path

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

Thanks a lot for the review. My responses are inline below.

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

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

Done.

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;
}

Fixed.

2.) This comment should mention \ir
* Handler for \i, but can be used for other things as well. ...

Added.

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

IMHO, this structure member was already being used for a bit more than error
reporting, so changed the comment to just say

/* File being currently processed, if any */

4.) I think the changes to process_file() merit another comment or
two, e.g. describing what relative_file is supposed to be.

Added.

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.

Yes, the canonicalize_path() function call done just a few lines above
converts the Windows style separator to Unix style one.

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)

Yes, I just followed the precedent; that line still crosses the 80 column
limit, though. I'd leave for a pgindent run or the commiter to fix as I
don't know what the right fix would be.

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.

Thanks Josh. Updated patch attached.
--
Gurjeet Singh
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

On Tue, May 17, 2011 at 2:43 PM, Robert Haas <robertmhaas@gmail.com> wrote:

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.

Well, that was added to commitfest about 3 months ago, which is great
because somebody reviewed it without me having to resubmit it.

New patch submitted and marked as 'Needs Review'.

Thanks,
--
Gurjeet Singh
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

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

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

[snip]
The rest looks good.

Josh

On Sat, May 21, 2011 at 11:59 AM, Josh Kupershmidt <schmiddy@gmail.com>wrote:

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.

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

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 processing file"
* "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 1:06 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote:

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 processing file"
* "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.

Attached an updated patch.

If you find it ready for committer, please mark it so in the commitfest app.

Thanks,
--
Gurjeet Singh
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

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

--
Gurjeet Singh
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

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

On Wed, Jul 6, 2011 at 11:58 AM, Robert Haas <robertmhaas@gmail.com> wrote:

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.

Thank you Robert and Josh for all the help.

--
Gurjeet Singh
EnterpriseDB Corporation
The Enterprise PostgreSQL Company