Re: Review: psql include file using relative path

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