On Tue, Nov 19, 2013 at 11:51 PM, Robert Haas <robertmh...@gmail.com> wrote:
> On Fri, Nov 15, 2013 at 9:01 PM, Fabrízio de Royes Mello
> <fabriziome...@gmail.com> wrote:
>> On Wed, Nov 13, 2013 at 9:37 PM, Josh Berkus <j...@agliodbs.com> wrote:
>>> handyrep@john:~/handyrep$ pg_isready --version
>>> pg_isready (PostgreSQL) 9.3.1
>>>
>>> handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U postgres -d
>>> postgres -q
>>> pg_isready: missing "=" after "postgres" in connection info string
>>>
>>> handyrep@john:~/handyrep$ pg_isready --host=john --port=5432
>>> --user=postgres --dbname=postgres
>>> pg_isready: missing "=" after "postgres" in connection info string
>>>
>>> handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U postgres
>>> john:5432 - accepting connections
>>>
>>> so apparently the -d option:
>>>
>>> a) doesn't work, and
>>> b) doesn't do anything
>>>
>>> I suggest simply removing it from the utility.
>>>
>>> I'll note that the -U option doesn't appear to do anything relevant
>>> either, but at least it doesn't error unnecessarily:
>>>
>>> handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U no_such_user
>>> john:5432 - accepting connections
>>>
>>
>> The attached patch fix it.
>
> Well, there still seem to be some problems.
>
> [rhaas pgsql]$ src/bin/scripts/pg_isready -d host=sgdasasg
> /tmp:5432 - no attempt
>
> I added some debugging instrumentation and it looks like there's a
> problem with this code:
>
>         if (strcmp(def->keyword, "hostaddr") == 0 ||
>             strcmp(def->keyword, "host") == 0)
>
> The problem is that we end up executing the code contained in that
> block twice, once for host and once for hostaddr.  Thus:
>
> [rhaas pgsql]$ src/bin/scripts/pg_isready -d host=sgdasasg
> def->keyword=host pghost_str=sgdasasg
> def->keyword=hostaddr pghost_str=/tmp
> def->keyword=port pgport_str=5432
> /tmp:5432 - no attempt
>
> Oops.
>
> Nevertheless, that's kind of a separate problem, so we could address
> in a separate commit.  But even ignoring that, this still isn't right:
> according to the fine documentation, -d will be expanded not only if
> it contains an =, but also if it starts with a valid URI prefix.  This
> would break that documented behavior.
>
> http://www.postgresql.org/docs/9.3/static/app-pg-isready.html

Attached is the updated version of the patch. Is there any other bug?

Regards,

-- 
Fujii Masao
*** a/src/bin/scripts/pg_isready.c
--- b/src/bin/scripts/pg_isready.c
***************
*** 31,36 **** main(int argc, char **argv)
--- 31,37 ----
  	const char *connect_timeout = DEFAULT_CONNECT_TIMEOUT;
  
  	const char *pghost_str = NULL;
+ 	const char *pghostaddr_str = NULL;
  	const char *pgport_str = NULL;
  
  #define PARAMS_ARRAY_SIZE	7
***************
*** 130,136 **** main(int argc, char **argv)
  	/*
  	 * Get the host and port so we can display them in our output
  	 */
! 	if (pgdbname)
  	{
  		opts = PQconninfoParse(pgdbname, &errmsg);
  		if (opts == NULL)
--- 131,140 ----
  	/*
  	 * Get the host and port so we can display them in our output
  	 */
! 	if (pgdbname &&
! 		(strncmp(pgdbname, "postgresql://", 13) == 0 ||
! 		 strncmp(pgdbname, "postgres://", 11) == 0 ||
! 		 strchr(pgdbname, '=') != NULL))
  	{
  		opts = PQconninfoParse(pgdbname, &errmsg);
  		if (opts == NULL)
***************
*** 149,156 **** main(int argc, char **argv)
  
  	for (opt = opts, def = defs; def->keyword; def++)
  	{
! 		if (strcmp(def->keyword, "hostaddr") == 0 ||
! 			strcmp(def->keyword, "host") == 0)
  		{
  			if (opt && opt->val)
  				pghost_str = opt->val;
--- 153,159 ----
  
  	for (opt = opts, def = defs; def->keyword; def++)
  	{
! 		if (strcmp(def->keyword, "host") == 0)
  		{
  			if (opt && opt->val)
  				pghost_str = opt->val;
***************
*** 161,166 **** main(int argc, char **argv)
--- 164,176 ----
  			else
  				pghost_str = DEFAULT_PGSOCKET_DIR;
  		}
+ 		else if (strcmp(def->keyword, "hostaddr") == 0)
+ 		{
+ 			if (opt && opt->val)
+ 				pghostaddr_str = opt->val;
+ 			else if (def->val)
+ 				pghostaddr_str = def->val;
+ 		}
  		else if (strcmp(def->keyword, "port") == 0)
  		{
  			if (opt && opt->val)
***************
*** 179,185 **** main(int argc, char **argv)
  
  	if (!quiet)
  	{
! 		printf("%s:%s - ", pghost_str, pgport_str);
  
  		switch (rv)
  		{
--- 189,197 ----
  
  	if (!quiet)
  	{
! 		printf("%s:%s - ",
! 			   pghostaddr_str != NULL ? pghostaddr_str : pghost_str,
! 			   pgport_str);
  
  		switch (rv)
  		{
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to