Re: [HACKERS] Review: psql include file using relative path

2011-07-06 Thread Robert Haas 
On Mon, Jun 6, 2011 at 10:11 PM, Gurjeet Singh singh.gurj...@gmail.com wrote:
 On Mon, Jun 6, 2011 at 9:48 PM, Josh Kupershmidt schmi...@gmail.com wrote:

 On Sun, Jun 5, 2011 at 8:16 PM, Gurjeet Singh singh.gurj...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Review: psql include file using relative path

2011-07-06 Thread Gurjeet Singh 
On Wed, Jul 6, 2011 at 11:58 AM, Robert Haas robertmh...@gmail.com wrote:

 On Mon, Jun 6, 2011 at 10:11 PM, Gurjeet Singh singh.gurj...@gmail.com
 wrote:
  On Mon, Jun 6, 2011 at 9:48 PM, Josh Kupershmidt schmi...@gmail.com
 wrote:
 
  On Sun, Jun 5, 2011 at 8:16 PM, Gurjeet Singh singh.gurj...@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

Re: [HACKERS] Review: psql include file using relative path

2011-06-06 Thread Josh Kupershmidt 
On Sun, Jun 5, 2011 at 8:16 PM, Gurjeet Singh singh.gurj...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Review: psql include file using relative path

2011-06-06 Thread Gurjeet Singh 
On Mon, Jun 6, 2011 at 9:48 PM, Josh Kupershmidt schmi...@gmail.com wrote:

 On Sun, Jun 5, 2011 at 8:16 PM, Gurjeet Singh singh.gurj...@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

Re: [HACKERS] Review: psql include file using relative path

2011-06-05 Thread Gurjeet Singh 
On Sat, May 21, 2011 at 11:59 AM, Josh Kupershmidt schmi...@gmail.comwrote:

 On Fri, May 20, 2011 at 2:35 PM, Gurjeet Singh singh.gurj...@gmail.com
 wrote:
  On Sat, May 14, 2011 at 5:03 PM, Josh Kupershmidt schmi...@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=parameterfilename/replaceable
uses relative path notation, then the file will be looked up
 relative to currently
executing file's location.

If the replaceable class=parameterfilename/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=parameterfilename/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=parameterfilename/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

Re: [HACKERS] Review: psql include file using relative path

2011-06-05 Thread Josh Kupershmidt 
On Sun, Jun 5, 2011 at 10:21 AM, Gurjeet Singh singh.gurj...@gmail.com wrote:
 On Sat, May 21, 2011 at 11:59 AM, Josh Kupershmidt schmi...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Review: psql include file using relative path

2011-06-05 Thread Gurjeet Singh 
On Sun, Jun 5, 2011 at 1:06 PM, Josh Kupershmidt schmi...@gmail.com wrote:

 On Sun, Jun 5, 2011 at 10:21 AM, Gurjeet Singh singh.gurj...@gmail.com
 wrote:
  On Sat, May 21, 2011 at 11:59 AM, Josh Kupershmidt schmi...@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
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index eaf901d..9bd6d93 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1626,6 +1626,30 @@ Tue Oct 26 21:40:57 CEST 1999
 
 
   varlistentry
+termliteral\ir replaceable class=parameterfilename/replaceable/literal/term
+listitem
+para
+The literal\ir/ command is similar to literal\i/, but is useful for files which
+include and execute other files.
+/para
+
+para
+When used within a file loaded via literal\i/literal, literal\ir/literal, or
+option-f/option, if the replaceable class=parameterfilename/replaceable
+is specified using 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=parameterfilename/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
+/listitem
+  /varlistentry
+
+
+  varlistentry
 termliteral\l/literal (or literal\list/literal)/term
 termliteral\l+/literal (or literal\list+/literal)/term
 listitem
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 378330b..faa3087 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -785,10 +785,15 @@ exec_command(const char *cmd,
 
 
 	/* \i is include file */
-	else if (strcmp(cmd, i) == 0 || strcmp(cmd, include) == 0)
+	/* \ir is include file relative to file currently being processed */
+	else if (strcmp(cmd, i) == 0 || strcmp(cmd, include) == 0
+			|| strcmp(cmd, ir) == 0 || strcmp(cmd, include_relative) == 0)
 	{
-		char	   *fname = psql_scan_slash_option(scan_state,
-   OT_NORMAL, NULL, true);
+		char   *fname = psql_scan_slash_option(scan_state,
+			   OT_NORMAL, NULL, true);
+
+		bool	include_relative = (strcmp(cmd, ir) == 0
+	|| strcmp(cmd, include_relative) == 0);
 
 		if (!fname)
 		{
@@ -798,7 +803,7 @@ exec_command(const char *cmd,
 		else
 		{
 			expand_tilde(fname);
-			success = (process_file(fname, false) == EXIT_SUCCESS);
+			success = (process_file(fname, false, include_relative) == EXIT_SUCCESS);
 			free(fname);
 		}
 	}
@@ -1969,15 +1974,19 @@ do_edit(const char *filename_arg, PQExpBuffer query_buf,
  * process_file
  *
  * Read commands from filename and then them to the main processing loop
- * Handler for \i, but can be used for other things as well.  Returns
+ * Handler for \i and \ir, but can be used for other things as well.  Returns
  * MainLoop() error code.
+ *
+ * If use_relative_path is true and filename is not an absolute path, then open
+ * the file from where the currently processed file (if any) is located.
  */
 int
-process_file(char *filename, bool single_txn)
+process_file(char *filename, bool single_txn, bool use_relative_path)
 {
 	FILE	   *fd;
 	int			result;
 	char	   *oldfilename;
+	char	   *relative_file = NULL;
 	PGresult   *res;
 
 	if (!filename)
@@ -1986,6 +1995,39 @@ process_file(char *filename, bool single_txn)
 	if (strcmp(filename, -) != 0)
 	{
 		canonicalize_path(filename);
+
+		/*
+		 * If the currently processing file uses the \ir command, and the
+		 * 'filename' parameter to the command is given as a relative file path,
+		 * then we resolve this path relative to the currently processing file.
+		 *
+		 * If the \ir command was executed in interactive mode (i.e. not via \i,
+		 * \ir or -f) the we treat it the same as the \i command.
+		 */
+		if (use_relative_path  pset.inputfile)
+		{
+			char	   *last_slash;
+
+			/* find the / that splits the file from its path */
+			last_slash = strrchr(pset.inputfile, '/');
+
+

Re: [HACKERS] Review: psql include file using relative path

2011-05-21 Thread Josh Kupershmidt 
On Fri, May 20, 2011 at 2:35 PM, Gurjeet Singh singh.gurj...@gmail.com wrote:
 On Sat, May 14, 2011 at 5:03 PM, Josh Kupershmidt schmi...@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=parameterfilename/replaceable
uses relative path notation, then the file will be looked up
relative to currently
executing file's location.

If the replaceable class=parameterfilename/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=parameterfilename/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=parameterfilename/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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Review: psql include file using relative path

2011-05-20 Thread Gurjeet Singh 
Thanks a lot for the review. My responses are inline below.

On Sat, May 14, 2011 at 5:03 PM, Josh Kupershmidt schmi...@gmail.comwrote:

 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-k...@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
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ac351d3..e37f75f 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1628,6 +1628,22 @@ Tue Oct 26 21:40:57 CEST 1999
 
 
   varlistentry
+termliteral\ir replaceable class=parameterfilename/replaceable/literal/term
+listitem
+para
+When used within a script, if the replaceable class=parameterfilename/replaceable
+uses relative path notation, then the file will be looked up relative to currently
+executing file's location.
+
+If the replaceable class=parameterfilename/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
+/listitem
+  /varlistentry
+
+
+  varlistentry
 termliteral\l/literal (or literal\list/literal)/term
 termliteral\l+/literal (or literal\list+/literal)/term
 listitem

Re: [HACKERS] Review: psql include file using relative path

2011-05-20 Thread Gurjeet Singh 
On Tue, May 17, 2011 at 2:43 PM, Robert Haas robertmh...@gmail.com wrote:

 On Sat, May 14, 2011 at 5:03 PM, Josh Kupershmidt schmi...@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-k...@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

Re: [HACKERS] Review: psql include file using relative path

2011-05-17 Thread Robert Haas 
On Sat, May 14, 2011 at 5:03 PM, Josh Kupershmidt schmi...@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-k...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Review: psql include file using relative path

2011-05-14 Thread Josh Kupershmidt 
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-k...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers