Обсуждение: 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
Вложения
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.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.) 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. + 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. +