Re: [PATCHES] remove BufferBlockPointers for speed and space
Mark Kirkwood wrote: Looks to me like -O2 makes the difference very small (on this platform/gcc combo) - is 5/169 worth doing? Ahem - misunderstood your comment here, sorry. Qingqing Zhou wrote: compiled with gcc testbuf.c. I tried -O2 actually, and it turns out that the timing is reduced a lot so not believable. ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] 5 new entries for FAQ
On Wed, Aug 10, 2005 at 10:04:22AM +0200, Martijn van Oosterhout wrote: After going through pgsql-general a bit I figured there were a few important questions missing from the FAQ, so I wrote some. Given the additions to the FAQ and suggestions made by Bruce about how often the questions come up, I've decided to withdraw this patch. Have a nice day, -- Martijn van Oosterhout kleptog@svana.org http://svana.org/kleptog/ Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a tool for doing 5% of the work and then sitting around waiting for someone else to do the other 95% so you can sue them. pgpeCTDeJFnOe.pgp Description: PGP signature
Re: [PATCHES] remove BufferBlockPointers for speed and space
Gavin Sherry wrote: Or more than one hardware architecture (which you didn't even say what you tested...) Well, he tested on SunOS (!) and Linux -- I presume that's two architectures. Sun still calls Solaris SunOs - try doing uname -s on a Solaris box (or look at a buildfarm solaris build info) So what he said he tested could mean Solaris/x86 + Linux x86 or Solaris/Sparc + Linux Sparc among other possible combos. Given the results I assume they are at least not the same box ;-) cheers andrew ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] remove BufferBlockPointers for speed and space
On Thu, 11 Aug 2005, Andrew Dunstan wrote: Gavin Sherry wrote: Or more than one hardware architecture (which you didn't even say what you tested...) Well, he tested on SunOS (!) and Linux -- I presume that's two architectures. Sun still calls Solaris SunOs - try doing uname -s on a Solaris box (or look at a buildfarm solaris build info) True. But my previous experience in university environments is that SunOS usually refers to SunOS 2.6 -- and the performance indicates old hardware. The thing is, compilser optimised versions of the test reveal very little difference in performance. This may be because the compiler is very good at optimising sequential annd predictable access to the array. Instead, we should mimic what we see in the real world: random access. Gavin ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] PL/pgSQL: #option select_into_1_row (was SELECT INTO
On Tue, 2005-08-09 at 15:01 +, Matt Miller wrote: Attached is a patch that implements the #option select_into_1_row directive as suggested. Is this patch good-to-go? Can it be queued? ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] PL/pgSQL: #option select_into_1_row (was SELECT INTO
Matt Miller wrote: On Tue, 2005-08-09 at 15:01 +, Matt Miller wrote: Attached is a patch that implements the #option select_into_1_row directive as suggested. Is this patch good-to-go? Can it be queued? We are in feature freeze, so unless there is overwhelming community support, it will be held for 8.2. -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] [HACKERS] For review: Server instrumentation patch
Andreas Pflug wrote: Bruce Momjian wrote: Dave Page wrote: The only part I didn't like about the patch is the stat display: test= select pg_file_stat('postgresql.conf'); pg_file_stat - (12287,2005-08-11 00:06:30,2005-08-11 00:06:43,2005-08-11 00:06:30,f) (1 row) Shouldn't this return multiple labeled columns rather than an array? pg_show_all_settings output is equally unreadable, designed not to be used directly. True, but we have SELECT * FROM pg_settings() that does display the titles. Right now the values aren't even documented. -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] [HACKERS] For review: Server instrumentation patch
Bruce Momjian wrote: Dave Page wrote: The only part I didn't like about the patch is the stat display: test= select pg_file_stat('postgresql.conf'); pg_file_stat - (12287,2005-08-11 00:06:30,2005-08-11 00:06:43,2005-08-11 00:06:30,f) (1 row) Shouldn't this return multiple labeled columns rather than an array? pg_show_all_settings output is equally unreadable, designed not to be used directly. Regards, Andreas ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] remove BufferBlockPointers for speed and space
Hello, With gcc-4 on Ubuntu AMD Athlon 2600 Barton: [EMAIL PROTECTED]:~$ ./a.out duration round 1 of array method: 0.539 ms duration round 2 of array method: 0.529 ms duration round 3 of array method: 0.530 ms duration round 1 of mul method: 0.258 ms duration round 2 of mul method: 5.563 ms duration round 3 of mul method: 0.258 ms duration round 1 of shift method: 0.240 ms duration round 2 of shift method: 0.248 ms duration round 3 of shift method: 0.240 ms [EMAIL PROTECTED]:~$ Sincerely, Joshua D. Drake Qingqing Zhou wrote: Tom Lane [EMAIL PROTECTED] writes: Also, I would like to see the actual test code. I wonder whether what you measured is the ability of the compiler to optimize references to successive elements of an array inside a loop; that has little or nothing to do with the typical usage of BufferGetBlock(). The source code is attached. compiled with gcc testbuf.c. I tried -O2 actually, and it turns out that the timing is reduced a lot so not believable. --- /* * testbuf.c */ #include stdio.h #include sys/file.h #include sys/param.h #include sys/stat.h #include sys/time.h #include unistd.h #include fcntl.h #define BLCKSZ 8192 #define NBuffers 8 typedef void* Block; int main(void) { int i, round, method; Block k, start; struct timeval start_t, stop_t; long usecs; Block *array = (Block *) calloc(NBuffers, sizeof(Block)); start = (Block)0xff3386; for (i = 0; i NBuffers; i++) array[i] = start + BLCKSZ*i; for (method = 0; method 3; method ++) { start = (Block)0xff3386; for (round = 0; round 3; round ++) { gettimeofday(start_t, NULL); if (method == 0) { for (i = 0; i NBuffers; i++) k = array[i]; } if (method == 1) { for (i = 0; i NBuffers; i++) k = start + i*BLCKSZ; } if (method == 2) { for (i = 0; i NBuffers; i++) k = start + (i13); } gettimeofday(stop_t, NULL); if (stop_t.tv_usec start_t.tv_usec) { stop_t.tv_sec--; stop_t.tv_usec += 100; } usecs = (long) (stop_t.tv_sec - start_t.tv_sec) * 100 + (long) (stop_t.tv_usec - start_t.tv_usec); fprintf (stdout, duration round %d of %s method: %ld.%03ld ms\n, round + 1, method==0?array:method==1?mul:shift, (long) ((stop_t.tv_sec - start_t.tv_sec) * 1000 + (stop_t.tv_usec - start_t.tv_usec) / 1000), (long) (stop_t.tv_usec - start_t.tv_usec) % 1000); } } } ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [HACKERS] For review: Server instrumentation patch
On Thu, Aug 11, 2005 at 11:34:43AM -0400, Bruce Momjian wrote: Andreas Pflug wrote: Bruce Momjian wrote: Dave Page wrote: The only part I didn't like about the patch is the stat display: test= select pg_file_stat('postgresql.conf'); pg_file_stat - (12287,2005-08-11 00:06:30,2005-08-11 00:06:43,2005-08-11 00:06:30,f) (1 row) Shouldn't this return multiple labeled columns rather than an array? pg_show_all_settings output is equally unreadable, designed not to be used directly. True, but we have SELECT * FROM pg_settings() that does display the titles. Right now the values aren't even documented. So, did you try select * from pg_file_stats('postgresql.conf'); -- Alvaro Herrera (alvherre[a]alvh.no-ip.org) Si no sabes adonde vas, es muy probable que acabes en otra parte. ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] [HACKERS] For review: Server instrumentation patch
On Thu, Aug 11, 2005 at 01:19:29PM -0400, Bruce Momjian wrote: Alvaro Herrera wrote: So, did you try select * from pg_file_stats('postgresql.conf'); Yes, I got: ERROR: a column definition list is required for functions returning record Interesting. I wonder why the function is not defined instead with OUT parameters. That way you don't have to declare the record at execution time. See my patch to pgstattuple for an example. -- Alvaro Herrera (alvherre[a]alvh.no-ip.org) No es bueno caminar con un hombre muerto ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] [HACKERS] For review: Server instrumentation patch
Alvaro Herrera wrote: On Thu, Aug 11, 2005 at 01:19:29PM -0400, Bruce Momjian wrote: Alvaro Herrera wrote: So, did you try select * from pg_file_stats('postgresql.conf'); Yes, I got: ERROR: a column definition list is required for functions returning record Interesting. I wonder why the function is not defined instead with OUT parameters. That way you don't have to declare the record at execution time. See my patch to pgstattuple for an example. Uh, where is that patch? I can't find it. -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] [HACKERS] For review: Server instrumentation patch
Alvaro Herrera wrote: On Thu, Aug 11, 2005 at 11:34:43AM -0400, Bruce Momjian wrote: Andreas Pflug wrote: Bruce Momjian wrote: Dave Page wrote: The only part I didn't like about the patch is the stat display: test= select pg_file_stat('postgresql.conf'); pg_file_stat - (12287,2005-08-11 00:06:30,2005-08-11 00:06:43,2005-08-11 00:06:30,f) (1 row) Shouldn't this return multiple labeled columns rather than an array? pg_show_all_settings output is equally unreadable, designed not to be used directly. True, but we have SELECT * FROM pg_settings() that does display the titles. Right now the values aren't even documented. So, did you try select * from pg_file_stats('postgresql.conf'); Yes, I got: ERROR: a column definition list is required for functions returning record -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] [HACKERS] For review: Server instrumentation patch
On Thu, Aug 11, 2005 at 02:26:16PM -0400, Bruce Momjian wrote: Alvaro Herrera wrote: Interesting. I wonder why the function is not defined instead with OUT parameters. That way you don't have to declare the record at execution time. See my patch to pgstattuple for an example. Uh, where is that patch? I can't find it. Hmm, seems it never made it to the list, I can't find it in the archives either. Here it is. -- Alvaro Herrera (alvherre[a]alvh.no-ip.org) ¿Qué importan los años? Lo que realmente importa es comprobar que a fin de cuentas la mejor edad de la vida es estar vivo (Mafalda) Index: pgstattuple.c === RCS file: /home/alvherre/cvs/pgsql/contrib/pgstattuple/pgstattuple.c,v retrieving revision 1.19 diff -c -r1.19 pgstattuple.c *** pgstattuple.c 30 May 2005 23:09:06 - 1.19 --- pgstattuple.c 28 Jul 2005 14:48:44 - *** *** 52,58 */ #define NCOLUMNS 9 - #define NCHARS 32 Datum pgstattuple(PG_FUNCTION_ARGS) --- 52,57 *** *** 105,117 uint64 dead_tuple_len = 0; uint64 tuple_count = 0; uint64 dead_tuple_count = 0; ! double tuple_percent; ! double dead_tuple_percent; uint64 free_space = 0; /* free/reusable space in bytes */ ! double free_percent; /* free/reusable space in % */ TupleDesc tupdesc; ! AttInMetadata *attinmeta; ! char **values; int i; Datum result; --- 104,116 uint64 dead_tuple_len = 0; uint64 tuple_count = 0; uint64 dead_tuple_count = 0; ! float8 tuple_percent; ! float8 dead_tuple_percent; uint64 free_space = 0; /* free/reusable space in bytes */ ! float8 free_percent; /* free/reusable space in % */ TupleDesc tupdesc; ! Datum *values; ! bool *nulls; int i; Datum result; *** *** 122,133 /* make sure we have a persistent copy of the tupdesc */ tupdesc = CreateTupleDescCopy(tupdesc); - /* -* Generate attribute metadata needed later to produce tuples from raw -* C strings -*/ - attinmeta = TupleDescGetAttInMetadata(tupdesc); - scan = heap_beginscan(rel, SnapshotAny, 0, NULL); nblocks = scan-rs_nblocks; /* # blocks to be scanned */ --- 121,126 *** *** 191,230 } else { ! tuple_percent = (double) tuple_len *100.0 / table_len; ! dead_tuple_percent = (double) dead_tuple_len *100.0 / table_len; ! free_percent = (double) free_space *100.0 / table_len; } /* !* Prepare a values array for constructing the tuple. This should be !* an array of C strings which will be processed later by the !* appropriate in functions. */ ! values = (char **) palloc(NCOLUMNS * sizeof(char *)); ! for (i = 0; i NCOLUMNS; i++) ! values[i] = (char *) palloc(NCHARS * sizeof(char)); i = 0; ! snprintf(values[i++], NCHARS, INT64_FORMAT, table_len); ! snprintf(values[i++], NCHARS, INT64_FORMAT, tuple_count); ! snprintf(values[i++], NCHARS, INT64_FORMAT, tuple_len); ! snprintf(values[i++], NCHARS, %.2f, tuple_percent); ! snprintf(values[i++], NCHARS, INT64_FORMAT, dead_tuple_count); ! snprintf(values[i++], NCHARS, INT64_FORMAT, dead_tuple_len); ! snprintf(values[i++], NCHARS, %.2f, dead_tuple_percent); ! snprintf(values[i++], NCHARS, INT64_FORMAT, free_space); ! snprintf(values[i++], NCHARS, %.2f, free_percent); /* build a tuple */ ! tuple = BuildTupleFromCStrings(attinmeta, values); /* make the tuple into a datum */ result = HeapTupleGetDatum(tuple); /* Clean up */ - for (i = 0; i NCOLUMNS; i++) - pfree(values[i]); pfree(values); ! return (result); } --- 184,220 } else { ! tuple_percent = (float8) tuple_len *100.0 / table_len; ! dead_tuple_percent = (float8) dead_tuple_len *100.0 / table_len; ! free_percent = (float8) free_space *100.0 / table_len; } /* !* Prepare a values array for constructing the tuple. */ ! values = (Datum *) palloc(NCOLUMNS * sizeof(Datum)); ! nulls = (bool *) palloc0(NCOLUMNS * sizeof(bool)); i = 0; ! ! values[i++] = Int64GetDatum(table_len); ! values[i++] = Int64GetDatum(tuple_count); ! values[i++] = Int64GetDatum(tuple_len); ! values[i++] = Float8GetDatum(tuple_percent); ! values[i++] =
Re: [PATCHES] [HACKERS] For review: Server instrumentation patch
pgman wrote: Andreas Pflug wrote: Bruce Momjian wrote: Dave Page wrote: The only part I didn't like about the patch is the stat display: test= select pg_file_stat('postgresql.conf'); pg_file_stat - (12287,2005-08-11 00:06:30,2005-08-11 00:06:43,2005-08-11 00:06:30,f) (1 row) Shouldn't this return multiple labeled columns rather than an array? pg_show_all_settings output is equally unreadable, designed not to be used directly. True, but we have SELECT * FROM pg_settings() that does display the titles. Right now the values aren't even documented. Sorry, I now see the return values are documented. -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [HACKERS] [PATCHES] O_DIRECT for WAL writes
Ok, I finally got a couple of tests done against CVS from Aug 3, 2005. I'm not sure if I'm showing anything insightful though. I've learned that fdatasync and O_DSYNC are simply fsync and O_SYNC respectively on Linux, which you guys may have already known. There appears to be a fair performance decrease in using open_sync. Just to double check, am I correct in understanding only open_sync uses O_DIRECT? fdatasync http://www.testing.osdl.org/projects/dbt2dev/results/dev4-015/38/ 5462 notpm open_sync http://www.testing.osdl.org/projects/dbt2dev/results/dev4-015/40/ 4860 notpm Mark ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [HACKERS] [PATCHES] O_DIRECT for WAL writes
Mark Wong wrote: Ok, I finally got a couple of tests done against CVS from Aug 3, 2005. I'm not sure if I'm showing anything insightful though. I've learned that fdatasync and O_DSYNC are simply fsync and O_SYNC respectively on Linux, which you guys may have already known. There appears to be a That is not what we thought for Linux, but many other OS's behave that way. fair performance decrease in using open_sync. Just to double check, am I correct in understanding only open_sync uses O_DIRECT? Right. fdatasync http://www.testing.osdl.org/projects/dbt2dev/results/dev4-015/38/ 5462 notpm open_sync http://www.testing.osdl.org/projects/dbt2dev/results/dev4-015/40/ 4860 notpm Right now open_sync is our last choice, which seems to still be valid for Linux, at least. -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] [HACKERS] Autovacuum loose ends
Alvaro Herrera [EMAIL PROTECTED] writes: Updated this patch again: - vacuum_cost_delay and vacuum_cost_limit can be set per table, as well as globally with autovacuum_vacuum_cost_{limit,delay} - pgstat is reset if recovery is required - pgstat reset at postmaster start is disabled by default - Xid-wraparound VACUUM is now FREEZE without ANALYZE, iff the database has datallowconn=false or datistemplate=true - A database-wide vacuum is also issued if the vacuumxid is found to be very old. Applied with minor tweaks --- mostly, fixing it so the custom cost settings are applied for ANALYZE as well as VACUUM. regards, tom lane ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [HACKERS] For review: Server instrumentation patch
Alvaro Herrera [EMAIL PROTECTED] writes: Interesting. I wonder why the function is not defined instead with OUT parameters. Because bootstrap mode isn't capable of dealing with array columns, so you can't define stuff in pg_proc.h that sets up an array of OUT parameter types. I tried to apply that idea for the pg_locks function a month or two ago, but it blew up in my face :-(. It'd be nice to fix this sometime, but not while we are so far past feature freeze. regards, tom lane ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] [HACKERS] Autovacuum loose ends
On Thu, Aug 11, 2005 at 05:13:15PM -0400, Tom Lane wrote: Alvaro Herrera [EMAIL PROTECTED] writes: Updated this patch again: Applied with minor tweaks --- mostly, fixing it so the custom cost settings are applied for ANALYZE as well as VACUUM. Ok, cool, thanks. I think this completes the autovacuum work I wanted to do for 8.1. AFAIK the only thing that we are still badly missing is the documentation update. (Matthew, if you are too busy to write it, please let me know so I know I have to tackle it.) For 8.2 my first priority (autovac-related) is to eliminate, or at least alleviate, the need for database-wide vacuums, by keeping track of Xid wraparound issues on a per-relation basis. -- Alvaro Herrera (alvherre[a]alvh.no-ip.org) Este mail se entrega garantizadamente 100% libre de sarcasmo. ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] [HACKERS] For review: Server instrumentation patch
Tom Lane wrote: Alvaro Herrera [EMAIL PROTECTED] writes: Interesting. I wonder why the function is not defined instead with OUT parameters. Because bootstrap mode isn't capable of dealing with array columns, so you can't define stuff in pg_proc.h that sets up an array of OUT parameter types. I tried to apply that idea for the pg_locks function a month or two ago, but it blew up in my face :-(. It'd be nice to fix this sometime, but not while we are so far past feature freeze. The patch includes documentation about the meaning the return values, and I think that is good enough. -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] BUG #1467: fe_connect doesn't handle EINTR right
AgentM [EMAIL PROTECTED] writes: Attached is a patch which corrects the behavior. I verified that the patch does not interfere with normal operation (using psql) but unfortunately the code path is virtually impossible to test without a really slow connection to a postgresql server [which I thankfully don't have]. To test the patch, you would need to send an interrupt at the exact time that the kernel is connect()ing in blocking mode- good luck. I've applied a simplified version of this patch --- there is no need to duplicate the functionality that the calling level must have anyway. regards, tom lane Index: fe-connect.c === RCS file: /cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v retrieving revision 1.316 retrieving revision 1.317 diff -c -r1.316 -r1.317 *** fe-connect.c9 Aug 2005 05:14:26 - 1.316 --- fe-connect.c11 Aug 2005 22:53:41 - 1.317 *** *** 1082,1096 * since we are in nonblock mode. If it does, well, * too bad. */ - retry_connect: if (connect(conn-sock, addr_cur-ai_addr, addr_cur-ai_addrlen) 0) { - if (SOCK_ERRNO == EINTR) - /* Interrupted system call - just try again */ - goto retry_connect; if (SOCK_ERRNO == EINPROGRESS || SOCK_ERRNO == EWOULDBLOCK || SOCK_ERRNO == 0) { /* --- 1082,1093 * since we are in nonblock mode. If it does, well, * too bad. */ if (connect(conn-sock, addr_cur-ai_addr, addr_cur-ai_addrlen) 0) { if (SOCK_ERRNO == EINPROGRESS || SOCK_ERRNO == EWOULDBLOCK || + SOCK_ERRNO == EINTR || SOCK_ERRNO == 0) { /* ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] Bug in canonicalize_path()
What if we let the trailing . or .. as it is? On Windows, GetFullPathName() can canonicalize a given path. $ uname -a MINGW32_NT-5.0 DEV 1.0.10(0.46/3/2) 2003-10-11 10:14 i686 unknown $ cat path.c #include windows.h int main(void) { CHAR Buf[1024]; GetFullPathName(C:\\Temp, 1024, Buf, NULL); printf(%s\n, Buf); GetFullPathName(C:\\Temp\\, 1024, Buf, NULL); printf(%s\n, Buf); GetFullPathName(C:\\Temp\\..\\, 1024, Buf, NULL); printf(%s\n, Buf); GetFullPathName(C:\\Temp\\..\\.., 1024, Buf, NULL); printf(%s\n, Buf); return 0; } $ gcc path.c $ ./a.exe C:\Temp C:\Temp\# trailing \ is not removed. C:\ # C:\ # C:\Temp\..\.. Bruce Momjian pgman@candle.pha.pa.us wrote news:[EMAIL PROTECTED] I found that in port/path.c::canonicalize_path, that if the path was supplied as /usr/local/bin/../.. we would return /usr/local/bin. The problem is then when we saw a trailing .. we stripped it off and the previous directory, but we never checked if the previous directory was itself ... Patch applied to suppress trimming of .. if .. is above it. I tried coding something that would handle ../.. but is started to look too messy and not worth the effort. I don't see a need to backpatch this, but it could produce errors with weird supplied paths. Comments? -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ? pg_config_paths.h Index: path.c === RCS file: /cvsroot/pgsql/src/port/path.c,v retrieving revision 1.51 diff -c -r1.51 path.c *** path.c 26 Jan 2005 19:24:03 - 1.51 --- path.c 11 Aug 2005 03:52:06 - *** *** 284,290 if (len 2 strcmp(path + len - 2, /.) == 0) trim_directory(path); ! else if (len 3 strcmp(path + len - 3, /..) == 0) { trim_directory(path); trim_directory(path); /* remove directory above */ --- 284,293 if (len 2 strcmp(path + len - 2, /.) == 0) trim_directory(path); ! /* We can only deal with /usr/local/.., not /usr/local/../.. */ ! else if (len 3 strcmp(path + len - 3, /..) == 0 ! (len != 5 || strcmp(path, ../..) != 0) ! (len 6 || strcmp(path + len - 6, /../..) != 0)) { trim_directory(path); trim_directory(path); /* remove directory above */ ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] Bug in canonicalize_path()
Yep, it is a bug on 8.0.X. Are you suggesting a backpatch? We can do it. Any objections? --- William ZHANG wrote: What if we let the trailing . or .. as it is? On Windows, GetFullPathName() can canonicalize a given path. $ uname -a MINGW32_NT-5.0 DEV 1.0.10(0.46/3/2) 2003-10-11 10:14 i686 unknown $ cat path.c #include windows.h int main(void) { CHAR Buf[1024]; GetFullPathName(C:\\Temp, 1024, Buf, NULL); printf(%s\n, Buf); GetFullPathName(C:\\Temp\\, 1024, Buf, NULL); printf(%s\n, Buf); GetFullPathName(C:\\Temp\\..\\, 1024, Buf, NULL); printf(%s\n, Buf); GetFullPathName(C:\\Temp\\..\\.., 1024, Buf, NULL); printf(%s\n, Buf); return 0; } $ gcc path.c $ ./a.exe C:\Temp C:\Temp\# trailing \ is not removed. C:\ # C:\ # C:\Temp\..\.. Bruce Momjian pgman@candle.pha.pa.us wrote news:[EMAIL PROTECTED] I found that in port/path.c::canonicalize_path, that if the path was supplied as /usr/local/bin/../.. we would return /usr/local/bin. The problem is then when we saw a trailing .. we stripped it off and the previous directory, but we never checked if the previous directory was itself ... Patch applied to suppress trimming of .. if .. is above it. I tried coding something that would handle ../.. but is started to look too messy and not worth the effort. I don't see a need to backpatch this, but it could produce errors with weird supplied paths. Comments? -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ? pg_config_paths.h Index: path.c === RCS file: /cvsroot/pgsql/src/port/path.c,v retrieving revision 1.51 diff -c -r1.51 path.c *** path.c 26 Jan 2005 19:24:03 - 1.51 --- path.c 11 Aug 2005 03:52:06 - *** *** 284,290 if (len 2 strcmp(path + len - 2, /.) == 0) trim_directory(path); ! else if (len 3 strcmp(path + len - 3, /..) == 0) { trim_directory(path); trim_directory(path); /* remove directory above */ --- 284,293 if (len 2 strcmp(path + len - 2, /.) == 0) trim_directory(path); ! /* We can only deal with /usr/local/.., not /usr/local/../.. */ ! else if (len 3 strcmp(path + len - 3, /..) == 0 ! (len != 5 || strcmp(path, ../..) != 0) ! (len 6 || strcmp(path + len - 6, /../..) != 0)) { trim_directory(path); trim_directory(path); /* remove directory above */ ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] Bug in canonicalize_path()
Bruce Momjian pgman@candle.pha.pa.us writes: Yep, it is a bug on 8.0.X. Are you suggesting a backpatch? We can do it. Any objections? I think he's suggesting that we depend on GetFullPathName(), which seems a bit pointless considering we have to write the code anyway for other platforms. I didn't much like the patch as-is; we should think a little harder about fixing the logic properly. regards, tom lane ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] Bug in canonicalize_path()
Tom Lane wrote: Bruce Momjian pgman@candle.pha.pa.us writes: Yep, it is a bug on 8.0.X. Are you suggesting a backpatch? We can do it. Any objections? I think he's suggesting that we depend on GetFullPathName(), which seems a bit pointless considering we have to write the code anyway for other platforms. Oh. I didn't much like the patch as-is; we should think a little harder about fixing the logic properly. I thought about it. Here is where we get stuck: usr/local/../../.. has to return .. Yuck. If it an absolute path like /usr, you are fine. In my first attempt, I counted the number of .. groups, then went up to remove each .., and them remove a regular directory for each ... And then you have this case: /usr/local/../bin/../.. Here you hit the first .. as you are going up. It just seemed like a lost cause. -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] Bug in canonicalize_path()
- Original Message - From: Tom Lane [EMAIL PROTECTED] To: Bruce Momjian pgman@candle.pha.pa.us Cc: William ZHANG [EMAIL PROTECTED]; pgsql-patches@postgresql.org Sent: Friday, August 12, 2005 10:31 AM Subject: Re: [PATCHES] Bug in canonicalize_path() Bruce Momjian pgman@candle.pha.pa.us writes: Yep, it is a bug on 8.0.X. Are you suggesting a backpatch? We can do it. Any objections? I think he's suggesting that we depend on GetFullPathName(), which seems a bit pointless considering we have to write the code anyway for other platforms. If it must be fixed, I think we can learn sth. from GetFullPathName(). Since we can not rely on this function on other platforms, we should write it from scratch. I didn't much like the patch as-is; we should think a little harder about fixing the logic properly. regards, tom lane ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] Bug in canonicalize_path()
I wrote: I didn't much like the patch as-is; we should think a little harder about fixing the logic properly. Like, say, this. (Very poorly tested but I think it's right.) regards, tom lane *** src/port/path.c.origThu Aug 11 21:39:22 2005 --- src/port/path.c Thu Aug 11 22:46:05 2005 *** *** 226,231 --- 226,232 { char *p, *to_p; boolwas_sep = false; + int pending_strips; #ifdef WIN32 /* *** *** 277,296 /* * Remove any trailing uses of . and process .. ourselves */ for (;;) { int len = strlen(path); if (len 2 strcmp(path + len - 2, /.) == 0) trim_directory(path); ! /* We can only deal with /usr/local/.., not /usr/local/../.. */ ! else if (len 3 strcmp(path + len - 3, /..) == 0 !(len != 5 || strcmp(path, ../..) != 0) !(len 6 || strcmp(path + len - 6, /../..) != 0)) { trim_directory(path); ! trim_directory(path); /* remove directory above */ } else break; --- 278,302 /* * Remove any trailing uses of . and process .. ourselves +* +* This is tricky because of the possibility of /foo/bar/baz/../.. */ + pending_strips = 0; for (;;) { int len = strlen(path); if (len 2 strcmp(path + len - 2, /.) == 0) trim_directory(path); ! else if (len 3 strcmp(path + len - 3, /..) == 0) { trim_directory(path); ! pending_strips++; /* must remove directory above */ ! } ! else if (pending_strips 0) ! { ! trim_directory(path); ! pending_strips--; } else break; ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] Bug in canonicalize_path()
Bruce Momjian pgman@candle.pha.pa.us writes: And then you have this case: /usr/local/../bin/../.. AFAICS, the patch I just proposed handles this. If I recall the code properly, we do not have to make canonicalize_path remove embedded . or .. --- that is, we do not have to simplify /usr/local/../bin But we do have to get rid of every trailing . or .., else there are failure cases when we replace the trailing component with an ordinary file name. regards, tom lane ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] Bug in canonicalize_path()
Tom Lane wrote: Bruce Momjian pgman@candle.pha.pa.us writes: And then you have this case: /usr/local/../bin/../.. AFAICS, the patch I just proposed handles this. If I recall the code properly, we do not have to make canonicalize_path remove embedded . or .. --- that is, we do not have to simplify /usr/local/../bin But we do have to get rid of every trailing . or .., else there are failure cases when we replace the trailing component with an ordinary file name. But what about usr/local/../../..? You loop is similar to what I coded, but then when I realized I had to check for pending trims after I run out of directories, and start appending them to create things like ../.., I gave up. -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] Bug in canonicalize_path()
Bruce Momjian pgman@candle.pha.pa.us writes: But what about usr/local/../../..? What about it? The case of /usr/local/../../.. is handled correctly, and the case where it's an underspecified relative path doesn't seem that interesting to me --- certainly that is not so important that we should get the wrong answer on cases that *are* plausible. Most of the uses of canonicalize_path are on paths that are required to be absolute, anyway. It wouldn't be too implausible to error out if pending_strips0 after exiting the loop. regards, tom lane ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] Bug in canonicalize_path()
Tom Lane wrote: Bruce Momjian pgman@candle.pha.pa.us writes: But what about usr/local/../../..? What about it? The case of /usr/local/../../.. is handled correctly, and the case where it's an underspecified relative path doesn't seem that interesting to me --- certainly that is not so important that we should get the wrong answer on cases that *are* plausible. Most of the uses of canonicalize_path are on paths that are required to be absolute, anyway. It wouldn't be too implausible to error out if pending_strips0 after exiting the loop. I figured it would be best to leave it alone if we can't process it, but if you think it is more imporant to trim in cases like ../.., go ahead. -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] Bug in canonicalize_path()
I have test the following on Windows and Linux: Windows: C:\cd Winnt C:\WINNTcd C:\Temp\..\..\ C:\ Linux: $ cd /usr/../../ $ pwd / We should handle this correctly. 1 Single dot in the path can be removed safety. (except the first one. e.g. ./usr/local) 2 Every double dot may need a removal of the last part of the path. (except the first one. e.g. ../local) And if there are not enough part left, keep the last part as it is. We can even make it easier by adding step 0: make sure path is an absolute path. - Original Message - From: Bruce Momjian pgman@candle.pha.pa.us To: Tom Lane [EMAIL PROTECTED] Cc: William ZHANG [EMAIL PROTECTED]; pgsql-patches@postgresql.org Sent: Friday, August 12, 2005 11:15 AM Subject: Re: [PATCHES] Bug in canonicalize_path() Tom Lane wrote: Bruce Momjian pgman@candle.pha.pa.us writes: But what about usr/local/../../..? What about it? The case of /usr/local/../../.. is handled correctly, and the case where it's an underspecified relative path doesn't seem that interesting to me --- certainly that is not so important that we should get the wrong answer on cases that *are* plausible. Most of the uses of canonicalize_path are on paths that are required to be absolute, anyway. It wouldn't be too implausible to error out if pending_strips0 after exiting the loop. I figured it would be best to leave it alone if we can't process it, but if you think it is more imporant to trim in cases like ../.., go ahead. -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] remove BufferBlockPointers for speed and space
Tom Lane [EMAIL PROTECTED] writes: This is definitely going to tell us a lot more about the compiler's loop strength reduction algorithms than it is going to tell about the time to evaluate any one of these expressions in isolation. What's worse, the compiler would be quite within its rights to detect that k isn't used anywhere, and optimize the loop away completely. Yes, it is possible. A safter way is to examine the assembling code. And I found there is no such optimization in Linux/gcc3.2. What I would suggest is first to fill another array with some large number of buffer numbers (randomly chosen from 1..N) and then time loops of the form for (i = 0; i arraysize; i++) { bn = buffernumbers[i]; bufferlocs[i] = BufferGetBlock(bn); } for all three possible definitions of BufferGetBlock (where both arrays are global variables, just to be sure the compiler doesn't think it can discard the store). This should give some numbers worth trusting. I've patched the code according to your suggestion. Result is: SunOS/gcc 3.2 verify: 1308873472 :duration round 1 of array method: 8.906 ms verify: 1308873472 :duration round 2 of array method: 8.775 ms verify: 1308873472 :duration round 3 of array method: 8.761 ms verify: 1308873472 :duration round 1 of mul method: 6.454 ms verify: 1308873472 :duration round 2 of mul method: 6.545 ms verify: 1308873472 :duration round 3 of mul method: 6.535 ms verify: 1308873472 :duration round 1 of shift method: 6.534 ms verify: 1308873472 :duration round 2 of shift method: 6.597 ms verify: 1308873472 :duration round 3 of shift method: 6.540 ms Linux/gcc 3.2 verify: -2141277440 :duration round 1 of array method: 2.385 ms verify: -2141277440 :duration round 2 of array method: 2.116 ms verify: -2141277440 :duration round 3 of array method: 2.057 ms verify: -2141277440 :duration round 1 of mul method: 0.943 ms verify: -2141277440 :duration round 2 of mul method: 0.983 ms verify: -2141277440 :duration round 3 of mul method: 0.840 ms verify: -2141277440 :duration round 1 of shift method: 0.864 ms verify: -2141277440 :duration round 2 of shift method: 0.931 ms verify: -2141277440 :duration round 3 of shift method: 0.968 ms The test file is attached: --- /* * buftest.c */ #include stdio.h #include stdlib.h #include sys/file.h #include sys/time.h #include unistd.h #define BLCKSZ 8192 #define NBuffers 8 typedef void* Block; Block *array = NULL; Block *reslt = NULL; int *bufnm = NULL; static int grand(int min, int max) { return (min + (int) (max * 1.0 * rand() / (RAND_MAX + 1.0))); } int main(void) { int n, i, round, method; Block start; struct timeval start_t, stop_t; long usecs; srandom(getpid()); array = (Block *) calloc(NBuffers, sizeof(Block)); reslt = (Block *) calloc(NBuffers, sizeof(Block)); bufnm = (int *) calloc(NBuffers, sizeof(int)); start = (Block)0xff3386; for (i = 0; i NBuffers; i++) array[i] = start + BLCKSZ*i; for (i = 0; i NBuffers; i++) bufnm[i] = grand(0, NBuffers); for (method = 0; method 3; method ++) { start = (Block)0xff3386; for (round = 0; round 3; round ++) { gettimeofday(start_t, NULL); if (method == 0) { for (i = 0; i NBuffers; i++) { n = bufnm[i]; reslt[i] = array[n]; } } if (method == 1) { for (i = 0; i NBuffers; i++) { n = bufnm[i]; reslt[i] = start + n*BLCKSZ; } } if (method == 2) { for (i = 0; i NBuffers; i++) { n = bufnm[i]; reslt[i] = start + (n13); } } gettimeofday(stop_t, NULL); usecs = 0; for (i = 0; i NBuffers; i++) usecs += (long) reslt[i]; fprintf(stdout, verify: %ld :, usecs ); if (stop_t.tv_usec start_t.tv_usec) { stop_t.tv_sec--; stop_t.tv_usec += 100; } usecs = (long) (stop_t.tv_sec - start_t.tv_sec) * 100 + (long) (stop_t.tv_usec - start_t.tv_usec); fprintf (stdout, duration round %d of %s method: %ld.%03ld ms\n, round + 1, method==0?array:method==1?mul:shift, (long) ((stop_t.tv_sec - start_t.tv_sec) * 1000 + (stop_t.tv_usec - start_t.tv_usec) / 1000), (long) (stop_t.tv_usec - start_t.tv_usec) % 1000); } } free(array); free(reslt); free(bufnm); return 0; } ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [HACKERS] For review: Server instrumentation patch
Here is an updated patch I have just applied (catalog version updated). I have added these functions: pg_stat_file() pg_read_file() pg_ls_dir() pg_reload_conf() pg_rotate_logfile() I did not include pg_logdir_ls because that was basically pg_ls_dir with logic to decode the log file name and convert it to a timestamp. That seemed best done in the client. I also renamed a number of the functions to be use verb-noun, rather than noun-verb. -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 Index: doc/src/sgml/func.sgml === RCS file: /cvsroot/pgsql/doc/src/sgml/func.sgml,v retrieving revision 1.276 diff -c -c -r1.276 func.sgml *** doc/src/sgml/func.sgml 2 Aug 2005 16:11:56 - 1.276 --- doc/src/sgml/func.sgml 12 Aug 2005 03:18:29 - *** *** 9061,9066 --- 9061,9069 indexterm zone=functions-admin primarypg_cancel_backend/primary /indexterm +indexterm zone=functions-admin + primarypg_reload_conf/primary +/indexterm indexterm zone=functions-admin primarysignal/primary *** *** 9068,9074 /indexterm para ! The function shown in xref linkend=functions-admin-signal-table sends control signals to other server processes. Use of this function is restricted to superusers. --- 9071,9077 /indexterm para ! The functions shown in xref linkend=functions-admin-signal-table sends control signals to other server processes. Use of this function is restricted to superusers. *** *** 9090,9115 entrytypeint/type/entry entryCancel a backend's current query/entry /row /tbody /tgroup /table para ! This function returns 1 if successful, 0 if not successful. The process ID (literalpid/literal) of an active backend can be found from the structfieldprocpid/structfield column in the structnamepg_stat_activity/structname view, or by listing the commandpostgres/command processes on the server with applicationps/. /para indexterm zone=functions-admin primarypg_start_backup/primary /indexterm - indexterm zone=functions-admin primarypg_stop_backup/primary /indexterm - indexterm zone=functions-admin primarybackup/primary /indexterm --- 9093,9128 entrytypeint/type/entry entryCancel a backend's current query/entry /row + row +entry + literalfunctionpg_reload_conf/function()/literal + /entry +entrytypeint/type/entry +entryCauses server processes to reload their configuration files/entry + /row /tbody /tgroup /table para ! These functions return 1 if successful, 0 if not successful. The process ID (literalpid/literal) of an active backend can be found from the structfieldprocpid/structfield column in the structnamepg_stat_activity/structname view, or by listing the commandpostgres/command processes on the server with applicationps/. /para +para + functionpg_reload_conf/ sends a SIGHUP signal to the + postmaster, causing the reload of the configuration files + in all backend processes. +/para indexterm zone=functions-admin primarypg_start_backup/primary /indexterm indexterm zone=functions-admin primarypg_stop_backup/primary /indexterm indexterm zone=functions-admin primarybackup/primary /indexterm *** *** 9309,9314 --- 9322,9434 appropriate. /para +para + The functions shown in xref + linkend=functions-admin-genfile provide native file access to + files on the machine hosting the server. Only files relative to + the cluster directory are allowed, and the logfile directory, + because the logfile directory might be stored outside the + cluster directory. Use of these functions is restricted to + superusers. +/para + +table id=functions-admin-genfile + titleGeneric File Access Functions/title + tgroup cols=3 + thead + rowentryName/entry entryReturn Type/entry entryDescription/entry + /row + /thead + + tbody + row +entry + literalfunctionpg_file_length/function(parameterfilename_text/parameter)/literal + indexterm zone=functions-admin + primarypg_file_length/primary + /indexterm +/entry +entrytypeint8/type/entry +entryReturns the file length/entry + /row + row +entry +
Re: [PATCHES] Bug in canonicalize_path()
Bruce Momjian pgman@candle.pha.pa.us writes: I figured it would be best to leave it alone if we can't process it, but if you think it is more imporant to trim in cases like ../.., go ahead. My recollection of this patch: 2004-08-09 16:20 tgl * src/: port/exec.c, port/path.c, bin/initdb/initdb.c: Path-mangling logic was failing to account for paths containing mentions of '.' or '..'. Extend canonicalize_path() to trim off trailing occurrences of these things, and use it to fix up paths where needed (which I think is only after places where we trim the last path component, but maybe some others will turn up). Fixes Josh's complaint that './initdb' does not work. is that it's part of the API contract of canonicalize_path() that it will not return something with trailing . or ... It's not just neatnik-ism to strip those, it's necessary to allow higher-level path-mangling routines to reason about how to do what they need. The entire concept of trim the last directory fails if you have to account for . or ... regards, tom lane ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] Bug in canonicalize_path()
Tom Lane wrote: Bruce Momjian pgman@candle.pha.pa.us writes: I figured it would be best to leave it alone if we can't process it, but if you think it is more imporant to trim in cases like ../.., go ahead. My recollection of this patch: 2004-08-09 16:20 tgl * src/: port/exec.c, port/path.c, bin/initdb/initdb.c: Path-mangling logic was failing to account for paths containing mentions of '.' or '..'. Extend canonicalize_path() to trim off trailing occurrences of these things, and use it to fix up paths where needed (which I think is only after places where we trim the last path component, but maybe some others will turn up). Fixes Josh's complaint that './initdb' does not work. is that it's part of the API contract of canonicalize_path() that it will not return something with trailing . or ... It's not just neatnik-ism to strip those, it's necessary to allow higher-level path-mangling routines to reason about how to do what they need. The entire concept of trim the last directory fails if you have to account for . or ... OK, new patch which I think handles all cases. -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 Index: src/port/path.c === RCS file: /cvsroot/pgsql/src/port/path.c,v retrieving revision 1.54 diff -c -c -r1.54 path.c *** src/port/path.c 12 Aug 2005 03:07:45 - 1.54 --- src/port/path.c 12 Aug 2005 04:47:59 - *** *** 226,231 --- 226,232 { char *p, *to_p; boolwas_sep = false; + int pending_strips = 0; #ifdef WIN32 /* *** *** 284,306 if (len 2 strcmp(path + len - 2, /.) == 0) trim_directory(path); ! /* !* Process only a single trailing .., and only if .. does !* not preceed it. !* So, we only deal with /usr/local/.., not with /usr/local/../... !* We don't handle the even more complex cases, like !* usr/local/../../... !*/ ! else if (len 3 strcmp(path + len - 3, /..) == 0 !(len != 5 || strcmp(path, ../..) != 0) !(len 6 || strcmp(path + len - 6, /../..) != 0)) { trim_directory(path); ! trim_directory(path); /* remove directory above */ } else break; } } --- 285,311 if (len 2 strcmp(path + len - 2, /.) == 0) trim_directory(path); ! else if (len 3 strcmp(path + len - 3, /..) == 0) { trim_directory(path); ! pending_strips++; ! } ! /* for absolute paths, we just keep trimming */ ! else if (*path != '\0' pending_strips 0) ! { ! trim_directory(path); ! pending_strips--; } else break; } + + if (pending_strips 0) + { + for (; pending_strips 0; pending_strips--) + strcat(path, ../); + trim_trailing_separator(path); + } } ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] remove BufferBlockPointers for speed and space
Qingqing Zhou [EMAIL PROTECTED] writes: I've patched the code according to your suggestion. Result is: [ snip ] OK, that test seems a little more believable. One point you didn't consider is that on 64-bit machines, the integer bufnum really has to be coerced to size_t to avoid overflow if the buffer array exceeds 2Gb. (Which we don't support today, but might well by the end of day tomorrow, seeing that there's a patch in the queue about it.) But I ran the test case with the extra coercion on an IA64 machine at Red Hat, and got substantially the same results as you did: the array method is just slower. Another consideration is that the array is competing for L2 cache --- the test program can't really show that, since it has no other use for L2 cache, but in the context of the real database I suspect this is at least as much of a win as shaving a few nanoseconds off the BufferGetBlock macro itself. So ... patch applied, and thanks for the good idea! regards, tom lane ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] Bug in canonicalize_path()
Bruce Momjian pgman@candle.pha.pa.us writes: Tom Lane wrote: ... it's part of the API contract of canonicalize_path() that it will not return something with trailing . or ... OK, new patch which I think handles all cases. + if (pending_strips 0) + { + for (; pending_strips 0; pending_strips--) + strcat(path, ../); + trim_trailing_separator(path); + } Uh, that hardly meets the API contract that I mentioned. I think we really have to throw an error if the path tries to .. above the starting point. (Remember again that most of the uses of this thing are dealing with absolute paths anyway, so this isn't that big a deal.) regards, tom lane ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org