Обсуждение: Re: [BUGS] BUG #3969: pg_ctl cannot detect server startup

"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