Re: Review: psql include file using relative path
От | Gurjeet Singh |
---|---|
Тема | Re: Review: psql include file using relative path |
Дата | |
Msg-id | BANLkTi=zgBVOfT076X9N1zRZRvfZwq24Xg@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Review: psql include file using relative path (Josh Kupershmidt <schmiddy@gmail.com>) |
Ответы |
Re: Review: psql include file using relative path
(Josh Kupershmidt <schmiddy@gmail.com>)
|
Список | pgsql-hackers |
On Sat, May 21, 2011 at 11:59 AM, Josh Kupershmidt <schmiddy@gmail.com> wrote:
My Bad. I did not intend to do that.
Although a bit winded, I think that sounds much clearer.
Yup, that fixes it. Thanks.
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.On Fri, May 20, 2011 at 2:35 PM, Gurjeet Singh <singh.gurjeet@gmail.com> wrote:Thanks for the fixes. Your updated patch is sent as a
> 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.
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.
This is a decent description from a technical standpoint:
>> == 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.
<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.
Well, this fix:
>> == 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.
if (!fd)
{
+ if (relative_path != NULL)
+ free(relative_path);+uses the wrong variable name (relative_path instead of relative_file),
psql_error("%s: %s\n", filename, strerror(errno));
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 (relative_file != NULL)
if (!fd)
{
psql_error("%s: %s\n", filename, strerror(errno));
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.
Some cleanup for this comment:
>> 4.) I think the changes to process_file() merit another comment or
>> two, e.g. describing what relative_file is supposed to be.
> Added.
+ /*
+ * 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.
--
Gurjeet Singh
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Вложения
В списке pgsql-hackers по дате отправления: