Обсуждение: 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@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
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:
Done.
Fixed.
Added.
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 */
Added.
Yes, the canonicalize_path() function call done just a few lines above converts the Windows style separator to Unix style one.
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.
Thanks Josh. Updated patch attached.
-- 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
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Вложения
<div dir="ltr"><div class="gmail_quote">On Tue, May 17, 2011 at 2:43 PM, Robert Haas <span dir="ltr"><<a href="mailto:robertmhaas@gmail.com">robertmhaas@gmail.com</a>></span>wrote:<br /><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div class="im">On Sat, May14, 2011 at 5:03 PM, Josh Kupershmidt <<a href="mailto:schmiddy@gmail.com">schmiddy@gmail.com</a>> wrote:<br />> I had a chance to give this patch a look. This review is of the second<br /> > patch posted by Gurjeet, at:<br/> > <a href="http://archives.postgresql.org/message-id/AANLkTi=YJb_A+GGt_pXmRqhBhyiD6aSWWB8h-Lw-KVi0@mail.gmail.com" target="_blank">http://archives.postgresql.org/message-id/AANLkTi=YJb_A+GGt_pXmRqhBhyiD6aSWWB8h-Lw-KVi0@mail.gmail.com</a><br /><br/></div>Cool. I see you (or someone) has added this to the entry for that<br /> patch on <a href="http://commitfest.postgresql.org"target="_blank">commitfest.postgresql.org</a> as well, which is great. I have<br/> updated that entry to list you as the reviewer and changed the status<br /> of the patch to "Waiting on Author"pending resolution of the issues<br /> you observed.<br /><div class="im"><br /></div></blockquote></div><br />Well,that was added to commitfest about 3 months ago, which is great because somebody reviewed it without me having toresubmit it.<br /><br />New patch submitted and marked as 'Needs Review'.<br /><br />Thanks,<br />-- <br /><div dir="ltr">GurjeetSingh<br />EnterpriseDB Corporation<br />The Enterprise PostgreSQL Company<br /></div><br /></div>
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 thiscommand 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, thenwe 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 wetreat 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@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
Вложения
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 processingfile"* "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@gmail.com> wrote:
Attached an updated patch.
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 addsYour rewordings are fine, but the the article "the" is missing in a
> much value.
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.I think the patch is in pretty good shape now. The memory leak is gone
> New version of the patch attached. Thanks for the review.
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
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Вложения
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
On Mon, Jun 6, 2011 at 9:48 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote:
Thanks for your reviews and perseverance :)
On Sun, Jun 5, 2011 at 8:16 PM, Gurjeet Singh <singh.gurjeet@gmail.com> wrote:I can't find anything further to nitpick with this patch, and have
> Attached an updated patch.
>
> If you find it ready for committer, please mark it so in the commitfest app.
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
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
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
On Wed, Jul 6, 2011 at 11:58 AM, Robert Haas <robertmhaas@gmail.com> wrote:
Thank you Robert and Josh for all the help.
On Mon, Jun 6, 2011 at 10:11 PM, Gurjeet Singh <singh.gurjeet@gmail.com> wrote:I committed this after substantial further revisions:
> 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 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
EnterpriseDB Corporation
The Enterprise PostgreSQL Company