Horton

Thread

  1. Review: psql include file using relative path

    Josh Kupershmidt <schmiddy@gmail.com> — 2011-05-14T21:03:32Z

    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

  2. Re: Review: psql include file using relative path

    Robert Haas <robertmhaas@gmail.com> — 2011-05-17T18:43:49Z

    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

  3. Re: Review: psql include file using relative path

    Gurjeet Singh <singh.gurjeet@gmail.com> — 2011-05-20T18:35:31Z

    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

  4. Re: Review: psql include file using relative path

    Gurjeet Singh <singh.gurjeet@gmail.com> — 2011-05-20T18:43:31Z

    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

  5. Re: Review: psql include file using relative path

    Josh Kupershmidt <schmiddy@gmail.com> — 2011-05-21T15:59:05Z

    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

  6. Re: Review: psql include file using relative path

    Gurjeet Singh <singh.gurjeet@gmail.com> — 2011-06-05T14:21:36Z

    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

  7. Re: Review: psql include file using relative path

    Josh Kupershmidt <schmiddy@gmail.com> — 2011-06-05T17:06:17Z

    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

  8. Re: Review: psql include file using relative path

    Gurjeet Singh <singh.gurjeet@gmail.com> — 2011-06-06T00:16:00Z

    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

  9. Re: Review: psql include file using relative path

    Josh Kupershmidt <schmiddy@gmail.com> — 2011-06-07T01:48:53Z

    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

  10. Re: Review: psql include file using relative path

    Gurjeet Singh <singh.gurjeet@gmail.com> — 2011-06-07T02:11:20Z

    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

  11. Re: Review: psql include file using relative path

    Robert Haas <robertmhaas@gmail.com> — 2011-07-06T15:58:19Z

    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

  12. Re: Review: psql include file using relative path

    Gurjeet Singh <singh.gurjeet@gmail.com> — 2011-07-06T17:07:35Z

    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