On Thu, Mar 28, 2013 at 03:06:30PM -0400, Steve Singer wrote:
> So to summarise:
> 
> Plan A: The first patch I attached for pg_upgrade + documentation
> changes, and changing the other places that call PQconndefaults() to
> accept failures on either out of memory, or an invalid PGSERVICE
> 
> Plan B: Create a new function PQconndefaults2(char * errorBuffer) or
> something similar that returned error information to the caller.
> 
> Plan C: PQconndefaults() just ignores an invalid service but
> connection attempts fail because other callers of
> conninfo_add_defaults still pay attention to connection failures.
> This is the second patch I sent.
> 
> Plan D:  Service lookup failures are always ignored by
> conninfo_add_defaults. If you attempt to connect with a bad
> PGSERVICE set it will behave as if no PGSERVICE value was set.   I
> don't think anyone explicitly proposed this yet.
> 
> Plan 'D' is the only option that I'm opposed to, it will effect a
> lot more applications then ones that call PQconndefaults() and I
> feel it will confuse users.
> 
> I'm not convinced plan B is worth the effort of having to maintain
> two versions of PQconndefaults() for a while to fix a corner case.

OK, I am back to looking at this issue from March.  I went with option
C, patch attached, which seems to be the most popular.  It is similar to
Steve's patch, but structured differently and includes a doc patch.

-- 
  Bruce Momjian  <br...@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +
diff --git a/contrib/pg_upgrade/server.c b/contrib/pg_upgrade/server.c
new file mode 100644
index b75f553..7d2aa35
*** a/contrib/pg_upgrade/server.c
--- b/contrib/pg_upgrade/server.c
*************** check_pghost_envvar(void)
*** 325,330 ****
--- 325,333 ----
  
  	start = PQconndefaults();
  
+ 	if (!start)
+ 		pg_fatal("out of memory\n");
+ 		
  	for (option = start; option->keyword != NULL; option++)
  	{
  		if (option->envvar && (strcmp(option->envvar, "PGHOST") == 0 ||
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
new file mode 100644
index 955f248..503a63a
*** a/doc/src/sgml/libpq.sgml
--- b/doc/src/sgml/libpq.sgml
*************** typedef struct
*** 483,489 ****
         with an entry having a null <structfield>keyword</> pointer.  The
         null pointer is returned if memory could not be allocated. Note that
         the current default values (<structfield>val</structfield> fields)
!        will depend on environment variables and other context.  Callers
         must treat the connection options data as read-only.
        </para>
  
--- 483,490 ----
         with an entry having a null <structfield>keyword</> pointer.  The
         null pointer is returned if memory could not be allocated. Note that
         the current default values (<structfield>val</structfield> fields)
!        will depend on environment variables and other context.  A
!        missing or invalid service file will be silently ignored.  Callers
         must treat the connection options data as read-only.
        </para>
  
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
new file mode 100644
index 975f795..9797140
*** a/src/interfaces/libpq/fe-auth.c
--- b/src/interfaces/libpq/fe-auth.c
*************** pg_fe_sendauth(AuthRequest areq, PGconn
*** 982,988 ****
   * if there is an error, return NULL with an error message in errorMessage
   */
  char *
! pg_fe_getauthname(PQExpBuffer errorMessage)
  {
  	const char *name = NULL;
  	char	   *authn;
--- 982,988 ----
   * if there is an error, return NULL with an error message in errorMessage
   */
  char *
! pg_fe_getauthname(void)
  {
  	const char *name = NULL;
  	char	   *authn;
diff --git a/src/interfaces/libpq/fe-auth.h b/src/interfaces/libpq/fe-auth.h
new file mode 100644
index bfddc68..25440b0
*** a/src/interfaces/libpq/fe-auth.h
--- b/src/interfaces/libpq/fe-auth.h
***************
*** 19,24 ****
  
  
  extern int	pg_fe_sendauth(AuthRequest areq, PGconn *conn);
! extern char *pg_fe_getauthname(PQExpBuffer errorMessage);
  
  #endif   /* FE_AUTH_H */
--- 19,24 ----
  
  
  extern int	pg_fe_sendauth(AuthRequest areq, PGconn *conn);
! extern char *pg_fe_getauthname(void);
  
  #endif   /* FE_AUTH_H */
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
new file mode 100644
index 8dd1a59..7ab4a9a
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
*************** PQconndefaults(void)
*** 875,881 ****
  	connOptions = conninfo_init(&errorBuf);
  	if (connOptions != NULL)
  	{
! 		if (!conninfo_add_defaults(connOptions, &errorBuf))
  		{
  			PQconninfoFree(connOptions);
  			connOptions = NULL;
--- 875,882 ----
  	connOptions = conninfo_init(&errorBuf);
  	if (connOptions != NULL)
  	{
! 		/* pass NULL errorBuf to ignore errors */
! 		if (!conninfo_add_defaults(connOptions, NULL))
  		{
  			PQconninfoFree(connOptions);
  			connOptions = NULL;
*************** conninfo_array_parse(const char *const *
*** 4412,4420 ****
   *
   * Defaults are obtained from a service file, environment variables, etc.
   *
!  * Returns TRUE if successful, otherwise FALSE; errorMessage is filled in
!  * upon failure.  Note that failure to locate a default value is not an
!  * error condition here --- we just leave the option's value as NULL.
   */
  static bool
  conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage)
--- 4413,4422 ----
   *
   * Defaults are obtained from a service file, environment variables, etc.
   *
!  * Returns TRUE if successful, otherwise FALSE; errorMessage, if supplied,
!  * is filled in upon failure.  Note that failure to locate a default value
!  * is not an error condition here --- we just leave the option's value as
!  * NULL.
   */
  static bool
  conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage)
*************** conninfo_add_defaults(PQconninfoOption *
*** 4424,4432 ****
  
  	/*
  	 * If there's a service spec, use it to obtain any not-explicitly-given
! 	 * parameters.
  	 */
! 	if (parseServiceInfo(options, errorMessage) != 0)
  		return false;
  
  	/*
--- 4426,4435 ----
  
  	/*
  	 * If there's a service spec, use it to obtain any not-explicitly-given
! 	 * parameters.  Ignore error if no error message buffer is passed
! 	 * because there is no way to pass back the failure message.
  	 */
! 	if (parseServiceInfo(options, errorMessage) != 0 && errorMessage)
  		return false;
  
  	/*
*************** conninfo_add_defaults(PQconninfoOption *
*** 4448,4455 ****
  				option->val = strdup(tmp);
  				if (!option->val)
  				{
! 					printfPQExpBuffer(errorMessage,
! 									  libpq_gettext("out of memory\n"));
  					return false;
  				}
  				continue;
--- 4451,4459 ----
  				option->val = strdup(tmp);
  				if (!option->val)
  				{
! 					if (errorMessage)
! 						printfPQExpBuffer(errorMessage,
! 										  libpq_gettext("out of memory\n"));
  					return false;
  				}
  				continue;
*************** conninfo_add_defaults(PQconninfoOption *
*** 4465,4472 ****
  			option->val = strdup(option->compiled);
  			if (!option->val)
  			{
! 				printfPQExpBuffer(errorMessage,
! 								  libpq_gettext("out of memory\n"));
  				return false;
  			}
  			continue;
--- 4469,4477 ----
  			option->val = strdup(option->compiled);
  			if (!option->val)
  			{
! 				if (errorMessage)
! 					printfPQExpBuffer(errorMessage,
! 									  libpq_gettext("out of memory\n"));
  				return false;
  			}
  			continue;
*************** conninfo_add_defaults(PQconninfoOption *
*** 4477,4483 ****
  		 */
  		if (strcmp(option->keyword, "user") == 0)
  		{
! 			option->val = pg_fe_getauthname(errorMessage);
  			continue;
  		}
  	}
--- 4482,4488 ----
  		 */
  		if (strcmp(option->keyword, "user") == 0)
  		{
! 			option->val = pg_fe_getauthname();
  			continue;
  		}
  	}
-- 
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