psql \ir filename normalization
Hi all,
Commit c7f23494c1103f87bcf1ef7cbfcd626e73edb337 editorialized a bit on
Gurjeet Singh's patch to implement \ir for psql, particularly in
process_file(). Unfortunately, it looks like it broke the common case
of loading a .SQL file in psql's working directory. Consider the
following test case:
mkdir -p /tmp/psql_test/subdir/
mkdir -p /tmp/psql_test/path2/
echo "SELECT 'hello 1';" > /tmp/psql_test/hello.sql
echo "SELECT 'hello from parent';" > /tmp/psql_test/hello_parent.sql
echo "SELECT 'hello from absolute path';" >
/tmp/psql_test/path2/absolute_path.sql
echo -e "SELECT 'hello 2';\n\ir ../hello_parent.sql\n\ir
/tmp/psql_test/path2/absolute_path.sql" >
/tmp/psql_test/subdir/hello2.sql
echo -e "\ir hello.sql\n\ir subdir/hello2.sql" > /tmp/psql_test/load.sql
If you try to load in "load.sql" from any working directory other than
/tmp/psql_test/ , you should correctly see four output statements.
However, if you:
cd /tmp/psql_test/ && psql test -f load.sql
You will get:
psql:load.sql:1: /hello.sql: No such file or directory
psql:load.sql:2: /subdir/hello2.sql: No such file or directory
Attached is a patch which fixes this, by recycling the bit of
Gurjeet's code which used "last_slash". (I have a feeling there's a
simpler way to fix it which avoids the last_slash complications.)
Josh
Attachments:
psql_fix_ir.v2.diffapplication/octet-stream; name=psql_fix_ir.v2.diffDownload
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
new file mode 100644
index 9cc73be..b3acb27
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
*************** process_file(char *filename, bool single
*** 2020,2031 ****
if (use_relative_path && pset.inputfile && !is_absolute_path(filename)
&& !has_drive_prefix(filename))
{
! snprintf(relpath, MAXPGPATH, "%s", pset.inputfile);
! get_parent_directory(relpath);
! join_path_components(relpath, relpath, filename);
! canonicalize_path(relpath);
! filename = relpath;
}
fd = fopen(filename, PG_BINARY_R);
--- 2020,2053 ----
if (use_relative_path && pset.inputfile && !is_absolute_path(filename)
&& !has_drive_prefix(filename))
{
! char *last_slash;
! /* find the / that splits the file from its path */
! last_slash = strrchr(pset.inputfile, '/');
!
! if (last_slash)
! {
! size_t dir_len = (last_slash - pset.inputfile) + 1;
!
! /* Check to make sure we won't overflow the array boundaries
! * of relpath.
! */
! if (dir_len + strlen(filename) >= MAXPGPATH)
! {
! psql_error("Filename(s) too long: %s %s\n", pset.inputfile, filename);
! return EXIT_FAILURE;
! }
!
! /* Construct an absolute filename relative to the directory
! * containing pset.inputfile.
! */
! relpath[0] = '\0';
! strncat(relpath, pset.inputfile, dir_len);
! strcat(relpath, filename);
!
! canonicalize_path(relpath);
! filename = relpath;
! }
}
fd = fopen(filename, PG_BINARY_R);
On Tue, Nov 15, 2011 at 6:54 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote:
Commit c7f23494c1103f87bcf1ef7cbfcd626e73edb337 editorialized a bit on
Gurjeet Singh's patch to implement \ir for psql, particularly in
process_file(). Unfortunately, it looks like it broke the common case
of loading a .SQL file in psql's working directory. Consider the
following test case:mkdir -p /tmp/psql_test/subdir/
mkdir -p /tmp/psql_test/path2/echo "SELECT 'hello 1';" > /tmp/psql_test/hello.sql
echo "SELECT 'hello from parent';" > /tmp/psql_test/hello_parent.sql
echo "SELECT 'hello from absolute path';" >
/tmp/psql_test/path2/absolute_path.sql
echo -e "SELECT 'hello 2';\n\ir ../hello_parent.sql\n\ir
/tmp/psql_test/path2/absolute_path.sql" >
/tmp/psql_test/subdir/hello2.sql
echo -e "\ir hello.sql\n\ir subdir/hello2.sql" > /tmp/psql_test/load.sqlIf you try to load in "load.sql" from any working directory other than
/tmp/psql_test/ , you should correctly see four output statements.
However, if you:
cd /tmp/psql_test/ && psql test -f load.sqlYou will get:
psql:load.sql:1: /hello.sql: No such file or directory
psql:load.sql:2: /subdir/hello2.sql: No such file or directoryAttached is a patch which fixes this, by recycling the bit of
Gurjeet's code which used "last_slash". (I have a feeling there's a
simpler way to fix it which avoids the last_slash complications.)
Argh. The root of the problem here seems to be that
join_path_components() feels entitled to arbitrarily insert a pathname
separator at the front of the output string even if its first input
didn't begin with one originally. Lame!
While looking for other places where this behavior might cause a
problem, I noticed something else that doesn't seem right. On
REL9_1_STABLE, if I initdb and then change the first "all" on the
"local" line to "@foo", I get this:
LOG: could not open secondary authentication file "@foo" as
"/Users/rhaas/pgsql/x/foo": No such file or directory
...and then the server starts up. But on master, I get:
LOG: could not open secondary authentication file "@foo" as
"/Users/rhaas/pgsql/y/foo": No such file or directory
LOG: end-of-line before database specification
CONTEXT: line 84 of configuration file "/Users/rhaas/pgsql/y/pg_hba.conf"
LOG: invalid connection type "all"
CONTEXT: line 85 of configuration file "/Users/rhaas/pgsql/y/pg_hba.conf"
FATAL: could not load pg_hba.conf
...which doesn't look right.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas wrote:
Argh. The root of the problem here seems to be that
join_path_components() feels entitled to arbitrarily insert a pathname
separator at the front of the output string even if its first input
didn't begin with one originally. Lame!
The attached patch fixes this report, I think.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
Attachments:
/rtmp/jointext/x-diffDownload
diff --git a/src/port/path.c b/src/port/path.c
new file mode 100644
index 13ca4f3..9cb0b01
*** a/src/port/path.c
--- b/src/port/path.c
*************** join_path_components(char *ret_path,
*** 212,218 ****
}
if (*tail)
snprintf(ret_path + strlen(ret_path), MAXPGPATH - strlen(ret_path),
! "/%s", tail);
}
--- 212,219 ----
}
if (*tail)
snprintf(ret_path + strlen(ret_path), MAXPGPATH - strlen(ret_path),
! /* only add slash if there is something already in head */
! "%s%s", head[0] ? "/" : "", tail);
}
On Mon, Nov 21, 2011 at 1:05 PM, Bruce Momjian <bruce@momjian.us> wrote:
Robert Haas wrote:
Argh. The root of the problem here seems to be that
join_path_components() feels entitled to arbitrarily insert a pathname
separator at the front of the output string even if its first input
didn't begin with one originally. Lame!The attached patch fixes this report, I think.
Looks sensible. Keep in mind we need to back-patch this.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas wrote:
On Mon, Nov 21, 2011 at 1:05 PM, Bruce Momjian <bruce@momjian.us> wrote:
Robert Haas wrote:
Argh. ?The root of the problem here seems to be that
join_path_components() feels entitled to arbitrarily insert a pathname
separator at the front of the output string even if its first input
didn't begin with one originally. ?Lame!The attached patch fixes this report, I think.
Looks sensible. Keep in mind we need to back-patch this.
Oh. Well, with no bug reports about it, does that make sense? Do we
have any code that relies on the old behavior?
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
On Mon, Nov 21, 2011 at 2:30 PM, Bruce Momjian <bruce@momjian.us> wrote:
Robert Haas wrote:
On Mon, Nov 21, 2011 at 1:05 PM, Bruce Momjian <bruce@momjian.us> wrote:
Robert Haas wrote:
Argh. ?The root of the problem here seems to be that
join_path_components() feels entitled to arbitrarily insert a pathname
separator at the front of the output string even if its first input
didn't begin with one originally. ?Lame!The attached patch fixes this report, I think.
Looks sensible. Keep in mind we need to back-patch this.
Oh. Well, with no bug reports about it, does that make sense? Do we
have any code that relies on the old behavior?
Oh, wait a minute. I was thinking \ir was in 9.1, but it's not: it
was committed after the branch. So I guess this only needs to be
fixed in master, which is much less scary.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas wrote:
On Mon, Nov 21, 2011 at 2:30 PM, Bruce Momjian <bruce@momjian.us> wrote:
Robert Haas wrote:
On Mon, Nov 21, 2011 at 1:05 PM, Bruce Momjian <bruce@momjian.us> wrote:
Robert Haas wrote:
Argh. ?The root of the problem here seems to be that
join_path_components() feels entitled to arbitrarily insert a pathname
separator at the front of the output string even if its first input
didn't begin with one originally. ?Lame!The attached patch fixes this report, I think.
Looks sensible. ?Keep in mind we need to back-patch this.
Oh. ?Well, with no bug reports about it, does that make sense? ?Do we
have any code that relies on the old behavior?Oh, wait a minute. I was thinking \ir was in 9.1, but it's not: it
was committed after the branch. So I guess this only needs to be
fixed in master, which is much less scary.
Agreed. I realize it is wrong but I have no idea what impact fixing it
in back branches might have, or people who are relying on the broken
behavior in some way.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
Bruce Momjian wrote:
Robert Haas wrote:
On Mon, Nov 21, 2011 at 2:30 PM, Bruce Momjian <bruce@momjian.us> wrote:
Robert Haas wrote:
On Mon, Nov 21, 2011 at 1:05 PM, Bruce Momjian <bruce@momjian.us> wrote:
Robert Haas wrote:
Argh. ?The root of the problem here seems to be that
join_path_components() feels entitled to arbitrarily insert a pathname
separator at the front of the output string even if its first input
didn't begin with one originally. ?Lame!The attached patch fixes this report, I think.
Looks sensible. ?Keep in mind we need to back-patch this.
Oh. ?Well, with no bug reports about it, does that make sense? ?Do we
have any code that relies on the old behavior?Oh, wait a minute. I was thinking \ir was in 9.1, but it's not: it
was committed after the branch. So I guess this only needs to be
fixed in master, which is much less scary.Agreed. I realize it is wrong but I have no idea what impact fixing it
in back branches might have, or people who are relying on the broken
behavior in some way.
Patch applied.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +