Re: 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(at)gmail(dot)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(at)gmail(dot)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(at)gmail(dot)com> wrote:

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

> On Fri, May 20, 2011 at 2:35 PM, Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)com>
> wrote:
> > On Sat, May 14, 2011 at 5:03 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)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(dot)gurjeet(at)gmail(dot)com> wrote:
> On Sat, May 21, 2011 at 11:59 AM, Josh Kupershmidt <schmiddy(at)gmail(dot)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(at)gmail(dot)com> wrote:

> On Sun, Jun 5, 2011 at 10:21 AM, Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)com>
> wrote:
> > On Sat, May 21, 2011 at 11:59 AM, Josh Kupershmidt <schmiddy(at)gmail(dot)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(dot)gurjeet(at)gmail(dot)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(at)gmail(dot)com> wrote:

> On Sun, Jun 5, 2011 at 8:16 PM, Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)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(dot)gurjeet(at)gmail(dot)com> wrote:
> On Mon, Jun 6, 2011 at 9:48 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
>>
>> On Sun, Jun 5, 2011 at 8:16 PM, Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)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(at)gmail(dot)com> wrote:

> On Mon, Jun 6, 2011 at 10:11 PM, Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)com>
> wrote:
> > On Mon, Jun 6, 2011 at 9:48 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com>
> wrote:
> >>
> >> On Sun, Jun 5, 2011 at 8:16 PM, Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)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