Skip site navigation (1) Skip section navigation (2)

Re: Review: psql include file using relative path

From: Josh Kupershmidt <schmiddy(at)gmail(dot)com>
To: Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)com>
Cc: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Review: psql include file using relative path
Date: 2011-05-21 15:59:05
Message-ID: BANLkTin_EX4wJpKtpusgiBOms3KH2BDbaw@mail.gmail.com (view raw or flat)
Thread:
Lists: pgsql-hackers
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

In response to

Responses

pgsql-hackers by date

Next:From: Alvaro HerreraDate: 2011-05-21 16:13:50
Subject: Re: eviscerating the parser
Previous:From: Tom LaneDate: 2011-05-21 15:38:42
Subject: Re: minor patch submission: CREATE CAST ... AS EXPLICIT

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group