Обсуждение: BUG #3969: pg_ctl cannot detect server startup
The following bug has been logged online: Bug reference: 3969 Logged by: ITAGAKI Takahiro Email address: itagaki.takahiro@oss.ntt.co.jp PostgreSQL version: 8.2.6, 8.3.0 Operating system: CentOS 5, Windows XP Description: pg_ctl cannot detect server startup Details: pg_ctl -w -o "-p 5432 -c max_connections=100" start waits for server startup forever and timeout, but server has successfully started actually. The following work as expected. * pg_ctl -w -o "-p 5432" start * pg_ctl -w -o "-c max_connections=100" start * pg_ctl -w -o "-c max_connections=100 -p 5432" start Are there any limitation in ordering of -o arguments?
"ITAGAKI Takahiro" <itagaki.takahiro@oss.ntt.co.jp> wrote: > Bug reference: 3969 > Description: pg_ctl cannot detect server startup > Details: > pg_ctl -w -o "-p 5432 -c max_connections=100" start > waits for server startup forever and timeout, > but server has successfully started actually. I found this bug comes from the definition of WHITESPACE characters in pg_ctl.c. WHITESPACE is defined as folows: #define WHITESPACE "\f\n\r\t\v" In fact, WHITESPACE does not contain whilespace (0x20) :-( I attach a patch to fix it. BTW, I also found similar definitions in some places. (Please grep with "\t\n\r".) They are a bit different from each other. For example, whitespaces is defined as " \t\n\r" in tzparser.c. Is it ok in the inconsistency? Or, should we always use " \f\n\r\t\v" ? Index: src/bin/pg_ctl/pg_ctl.c =================================================================== --- src/bin/pg_ctl/pg_ctl.c (HEAD) +++ src/bin/pg_ctl/pg_ctl.c (working copy) @@ -49,7 +49,7 @@ typedef long pgpid_t; -#define WHITESPACE "\f\n\r\t\v" /* as defined by isspace() */ +#define WHITESPACE " \f\n\r\t\v" /* as defined by isspace() */ /* postgres version ident string */ #define PM_VERSIONSTR "postgres (PostgreSQL) " PG_VERSION "\n" Regards, --- ITAGAKI Takahiro NTT Open Source Software Center
Вложения
ITAGAKI Takahiro <itagaki.takahiro@oss.ntt.co.jp> writes: > I found this bug comes from the definition of WHITESPACE > characters in pg_ctl.c. WHITESPACE is defined as folows: > #define WHITESPACE "\f\n\r\t\v" > In fact, WHITESPACE does not contain whilespace (0x20) :-( Ooops :-( > I attach a patch to fix it. Actually, this coding seems gratuitously ugly/inconsistent/fragile, so I'm inclined to rewrite it to eliminate WHITESPACE altogether. It seems bad style to switch between loops using isspace() and an entirely different type of string searching that (as demonstrated by the bug) isn't easy to keep in sync. Adding an additional loop of the same kind to scan over the parameter value would take a couple more lines, but I think it'll be a lot more readable. In fact there are more bugs here: it won't deal correctly with a quoted port value, and it'd be fooled by '-p' appearing in the argument value for another option type. I'm not sure how tense we should be about getting it to exactly match the backend's behavior for corner cases, but at the very least it probably shouldn't be fooled by -p appearing inside quotes. > BTW, I also found similar definitions in some places. > (Please grep with "\t\n\r".) > They are a bit different from each other. > For example, whitespaces is defined as " \t\n\r" in tzparser.c. > Is it ok in the inconsistency? Or, should we always use " \f\n\r\t\v" ? Not sure. One point is that vertical whitespace shouldn't necessarily be treated the same as horizontal whitespace. regards, tom lane