I'm reviewing the patch posted here: http://archives.postgresql.org/pgsql-hackers/2010-01/msg01579.php for this commitfest item: https://commitfest.postgresql.org/action/patch_view?id=259
Patch attached - a few minor changes: ------------------------------------- 1) Updated to apply cleanly against cvs tip 2) Improved comments 3) Moved much of what was in PQconnectStartParams() to a new conninfo_array_parse() to be more consistent with existing code Questions/comments: ------------------- a) Do we want an analog to PQconninfoParse(), e.g. PQconninfoParseParams()? If not, it isn't worth keeping use_defaults as an argument to conninfo_array_parse(). b) I refrained from further consolidation even though there is room. For example, I considered leaving only the real parsing code in conninfo_parse(), and having it return keywords and values arrays. If we did that, the rest of the code could be modified to accept keywords and values instead of conninfo, and therefore shared. I was concerned about the probably small performance hit to the existing code path. Thoughts? c) Obviously I liked the "two-arrays approach" better -- any objections to that? Thanks, Joe
Index: src/bin/psql/startup.c
===================================================================
RCS file: /opt/src/cvs/pgsql/src/bin/psql/startup.c,v
retrieving revision 1.158
diff -c -r1.158 startup.c
*** src/bin/psql/startup.c 2 Jan 2010 16:57:59 -0000 1.158
--- src/bin/psql/startup.c 25 Jan 2010 18:09:55 -0000
***************
*** 168,181 ****
if (pset.getPassword == TRI_YES)
password = simple_prompt(password_prompt, 100, false);
/* loop until we have a password if requested by backend */
do
{
! new_pass = false;
! pset.db = PQsetdbLogin(options.host, options.port, NULL, NULL,
! options.action == ACT_LIST_DB && options.dbname == NULL ?
! "postgres" : options.dbname,
! options.username, password);
if (PQstatus(pset.db) == CONNECTION_BAD &&
PQconnectionNeedsPassword(pset.db) &&
--- 168,200 ----
if (pset.getPassword == TRI_YES)
password = simple_prompt(password_prompt, 100, false);
+ const char *keywords[] = {
+ "host",
+ "port",
+ "dbname",
+ "user",
+ "password",
+ "application_name",
+ NULL
+ };
+
/* loop until we have a password if requested by backend */
do
{
! const char *values[] = {
! options.host,
! options.port,
! (options.action == ACT_LIST_DB &&
! options.dbname == NULL) ? "postgres" : options.dbname,
! options.username,
! password,
! pset.progname,
! NULL
! };
!
! new_pass = false;
!
! pset.db = PQconnectdbParams(keywords, values);
if (PQstatus(pset.db) == CONNECTION_BAD &&
PQconnectionNeedsPassword(pset.db) &&
Index: src/interfaces/libpq/exports.txt
===================================================================
RCS file: /opt/src/cvs/pgsql/src/interfaces/libpq/exports.txt,v
retrieving revision 1.24
diff -c -r1.24 exports.txt
*** src/interfaces/libpq/exports.txt 21 Jan 2010 14:58:53 -0000 1.24
--- src/interfaces/libpq/exports.txt 25 Jan 2010 18:09:55 -0000
***************
*** 155,157 ****
--- 155,159 ----
PQinitOpenSSL 153
PQescapeLiteral 154
PQescapeIdentifier 155
+ PQconnectdbParams 156
+ PQconnectStartParams 157
Index: src/interfaces/libpq/fe-connect.c
===================================================================
RCS file: /opt/src/cvs/pgsql/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.384
diff -c -r1.384 fe-connect.c
*** src/interfaces/libpq/fe-connect.c 20 Jan 2010 21:15:21 -0000 1.384
--- src/interfaces/libpq/fe-connect.c 25 Jan 2010 22:39:45 -0000
***************
*** 262,271 ****
--- 262,275 ----
static int connectDBStart(PGconn *conn);
static int connectDBComplete(PGconn *conn);
static PGconn *makeEmptyPGconn(void);
+ static void fillPGconn(PGconn *conn, PQconninfoOption *connOptions);
static void freePGconn(PGconn *conn);
static void closePGconn(PGconn *conn);
static PQconninfoOption *conninfo_parse(const char *conninfo,
PQExpBuffer errorMessage, bool use_defaults);
+ static PQconninfoOption *conninfo_array_parse(const char **keywords,
+ const char **values, PQExpBuffer errorMessage,
+ bool use_defaults);
static char *conninfo_getval(PQconninfoOption *connOptions,
const char *keyword);
static void defaultNoticeReceiver(void *arg, const PGresult *res);
***************
*** 290,312 ****
/*
* Connecting to a Database
*
! * There are now four different ways a user of this API can connect to the
* database. Two are not recommended for use in new code, because of their
* lack of extensibility with respect to the passing of options to the
* backend. These are PQsetdb and PQsetdbLogin (the former now being a macro
* to the latter).
*
* If it is desired to connect in a synchronous (blocking) manner, use the
! * function PQconnectdb.
*
* To connect in an asynchronous (non-blocking) manner, use the functions
! * PQconnectStart, and PQconnectPoll.
*
* Internally, the static functions connectDBStart, connectDBComplete
* are part of the connection procedure.
*/
/*
* PQconnectdb
*
* establishes a connection to a postgres backend through the postmaster
--- 294,353 ----
/*
* Connecting to a Database
*
! * There are now six different ways a user of this API can connect to the
* database. Two are not recommended for use in new code, because of their
* lack of extensibility with respect to the passing of options to the
* backend. These are PQsetdb and PQsetdbLogin (the former now being a macro
* to the latter).
*
* If it is desired to connect in a synchronous (blocking) manner, use the
! * function PQconnectdb or PQconnectdbParams. The former accepts a string
! * of option = value pairs which must be parsed; the latter takes two NULL
! * terminated arrays instead.
*
* To connect in an asynchronous (non-blocking) manner, use the functions
! * PQconnectStart or PQconnectStartParams (which differ in the same way as
! * PQconnectdb and PQconnectdbParams) and PQconnectPoll.
*
* Internally, the static functions connectDBStart, connectDBComplete
* are part of the connection procedure.
*/
/*
+ * PQconnectdbParams
+ *
+ * establishes a connection to a postgres backend through the postmaster
+ * using connection information in two arrays.
+ *
+ * The keywords array is defined as
+ *
+ * const char *params[] = {"option1", "option2", NULL}
+ *
+ * The values array is defined as
+ *
+ * const char *values[] = {"value1", "value2", NULL}
+ *
+ * Returns a PGconn* which is needed for all subsequent libpq calls, or NULL
+ * if a memory allocation failed.
+ * If the status field of the connection returned is CONNECTION_BAD,
+ * then some fields may be null'ed out instead of having valid values.
+ *
+ * You should call PQfinish (if conn is not NULL) regardless of whether this
+ * call succeeded.
+ */
+ PGconn *
+ PQconnectdbParams(const char **keywords, const char **values)
+ {
+ PGconn *conn = PQconnectStartParams(keywords, values);
+
+ if (conn && conn->status != CONNECTION_BAD)
+ (void) connectDBComplete(conn);
+
+ return conn;
+
+ }
+
+ /*
* PQconnectdb
*
* establishes a connection to a postgres backend through the postmaster
***************
*** 340,351 ****
}
/*
! * PQconnectStart
*
* Begins the establishment of a connection to a postgres backend through the
! * postmaster using connection information in a string.
*
! * See comment for PQconnectdb for the definition of the string format.
*
* Returns a PGconn*. If NULL is returned, a malloc error has occurred, and
* you should not attempt to proceed with this connection. If the status
--- 381,392 ----
}
/*
! * PQconnectStartParams
*
* Begins the establishment of a connection to a postgres backend through the
! * postmaster using connection information in a struct.
*
! * See comment for PQconnectdbParams for the definition of the string format.
*
* Returns a PGconn*. If NULL is returned, a malloc error has occurred, and
* you should not attempt to proceed with this connection. If the status
***************
*** 359,367 ****
* See PQconnectPoll for more info.
*/
PGconn *
! PQconnectStart(const char *conninfo)
{
! PGconn *conn;
/*
* Allocate memory for the conn structure
--- 400,409 ----
* See PQconnectPoll for more info.
*/
PGconn *
! PQconnectStartParams(const char **keywords, const char **values)
{
! PGconn *conn;
! PQconninfoOption *connOptions;
/*
* Allocate memory for the conn structure
***************
*** 371,380 ****
return NULL;
/*
! * Parse the conninfo string
*/
! if (!connectOptions1(conn, conninfo))
! return conn;
/*
* Compute derived options
--- 413,438 ----
return NULL;
/*
! * Parse the conninfo arrays
*/
! connOptions = conninfo_array_parse(keywords, values,
! &conn->errorMessage, true);
! if (connOptions == NULL)
! {
! conn->status = CONNECTION_BAD;
! /* errorMessage is already set */
! return false;
! }
!
! /*
! * Move option values into conn structure
! */
! fillPGconn(conn, connOptions);
!
! /*
! * Free the option info - all is in conn now
! */
! PQconninfoFree(connOptions);
/*
* Compute derived options
***************
*** 395,427 ****
}
/*
! * connectOptions1
*
! * Internal subroutine to set up connection parameters given an already-
! * created PGconn and a conninfo string. Derived settings should be
! * processed by calling connectOptions2 next. (We split them because
! * PQsetdbLogin overrides defaults in between.)
*
! * Returns true if OK, false if trouble (in which case errorMessage is set
! * and so is conn->status).
*/
! static bool
! connectOptions1(PGconn *conn, const char *conninfo)
{
! PQconninfoOption *connOptions;
! char *tmp;
/*
* Parse the conninfo string
*/
! connOptions = conninfo_parse(conninfo, &conn->errorMessage, true);
! if (connOptions == NULL)
{
conn->status = CONNECTION_BAD;
- /* errorMessage is already set */
- return false;
}
/*
* Move option values into conn structure
*
--- 453,517 ----
}
/*
! * PQconnectStart
*
! * Begins the establishment of a connection to a postgres backend through the
! * postmaster using connection information in a string.
*
! * See comment for PQconnectdb for the definition of the string format.
! *
! * Returns a PGconn*. If NULL is returned, a malloc error has occurred, and
! * you should not attempt to proceed with this connection. If the status
! * field of the connection returned is CONNECTION_BAD, an error has
! * occurred. In this case you should call PQfinish on the result, (perhaps
! * inspecting the error message first). Other fields of the structure may not
! * be valid if that occurs. If the status field is not CONNECTION_BAD, then
! * this stage has succeeded - call PQconnectPoll, using select(2) to see when
! * this is necessary.
! *
! * See PQconnectPoll for more info.
*/
! PGconn *
! PQconnectStart(const char *conninfo)
{
! PGconn *conn;
!
! /*
! * Allocate memory for the conn structure
! */
! conn = makeEmptyPGconn();
! if (conn == NULL)
! return NULL;
/*
* Parse the conninfo string
*/
! if (!connectOptions1(conn, conninfo))
! return conn;
!
! /*
! * Compute derived options
! */
! if (!connectOptions2(conn))
! return conn;
!
! /*
! * Connect to the database
! */
! if (!connectDBStart(conn))
{
+ /* Just in case we failed to set it in connectDBStart */
conn->status = CONNECTION_BAD;
}
+ return conn;
+ }
+
+ static void
+ fillPGconn(PGconn *conn, PQconninfoOption *connOptions)
+ {
+ char *tmp;
+
/*
* Move option values into conn structure
*
***************
*** 482,487 ****
--- 572,610 ----
#endif
tmp = conninfo_getval(connOptions, "replication");
conn->replication = tmp ? strdup(tmp) : NULL;
+ }
+
+ /*
+ * connectOptions1
+ *
+ * Internal subroutine to set up connection parameters given an already-
+ * created PGconn and a conninfo string. Derived settings should be
+ * processed by calling connectOptions2 next. (We split them because
+ * PQsetdbLogin overrides defaults in between.)
+ *
+ * Returns true if OK, false if trouble (in which case errorMessage is set
+ * and so is conn->status).
+ */
+ static bool
+ connectOptions1(PGconn *conn, const char *conninfo)
+ {
+ PQconninfoOption *connOptions;
+
+ /*
+ * Parse the conninfo string
+ */
+ connOptions = conninfo_parse(conninfo, &conn->errorMessage, true);
+ if (connOptions == NULL)
+ {
+ conn->status = CONNECTION_BAD;
+ /* errorMessage is already set */
+ return false;
+ }
+
+ /*
+ * Move option values into conn structure
+ */
+ fillPGconn(conn, connOptions);
/*
* Free the option info - all is in conn now
***************
*** 3598,3603 ****
--- 3721,3869 ----
return options;
}
+ /*
+ * Conninfo array parser routine
+ *
+ * If successful, a malloc'd PQconninfoOption array is returned.
+ * If not successful, NULL is returned and an error message is
+ * left in errorMessage.
+ * Defaults are supplied (from a service file, environment variables, etc)
+ * for unspecified options, but only if use_defaults is TRUE.
+ */
+ static PQconninfoOption *
+ conninfo_array_parse(const char **keywords, const char **values,
+ PQExpBuffer errorMessage, bool use_defaults)
+ {
+ char *tmp;
+ PQconninfoOption *options;
+ PQconninfoOption *option;
+ int i = 0;
+
+ /* Make a working copy of PQconninfoOptions */
+ options = malloc(sizeof(PQconninfoOptions));
+ if (options == NULL)
+ {
+ printfPQExpBuffer(errorMessage,
+ libpq_gettext("out of memory\n"));
+ return NULL;
+ }
+ memcpy(options, PQconninfoOptions, sizeof(PQconninfoOptions));
+
+ /* Parse the keywords/values arrays */
+ while(keywords[i])
+ {
+ const char *pname = keywords[i];
+ const char *pvalue = values[i];
+
+ if (pvalue != NULL)
+ {
+ /* Search for the param record */
+ for (option = options; option->keyword != NULL; option++)
+ {
+ if (strcmp(option->keyword, pname) == 0)
+ break;
+ }
+
+ /* Check for invalid connection option */
+ if (option->keyword == NULL)
+ {
+ printfPQExpBuffer(errorMessage,
+ libpq_gettext("invalid connection option \"%s\"\n"),
+ pname);
+ PQconninfoFree(options);
+ return NULL;
+ }
+
+ /*
+ * Store the value
+ */
+ if (option->val)
+ free(option->val);
+ option->val = strdup(pvalue);
+ if (!option->val)
+ {
+ printfPQExpBuffer(errorMessage,
+ libpq_gettext("out of memory\n"));
+ PQconninfoFree(options);
+ return NULL;
+ }
+ }
+ ++i;
+ }
+
+ /*
+ * Stop here if caller doesn't want defaults filled in.
+ */
+ if (!use_defaults)
+ return options;
+
+ /*
+ * If there's a service spec, use it to obtain any not-explicitly-given
+ * parameters.
+ */
+ if (parseServiceInfo(options, errorMessage))
+ {
+ PQconninfoFree(options);
+ return NULL;
+ }
+
+ /*
+ * Get the fallback resources for parameters not specified in the conninfo
+ * string nor the service.
+ */
+ for (option = options; option->keyword != NULL; option++)
+ {
+ if (option->val != NULL)
+ continue; /* Value was in conninfo or service */
+
+ /*
+ * Try to get the environment variable fallback
+ */
+ if (option->envvar != NULL)
+ {
+ if ((tmp = getenv(option->envvar)) != NULL)
+ {
+ option->val = strdup(tmp);
+ if (!option->val)
+ {
+ printfPQExpBuffer(errorMessage,
+ libpq_gettext("out of memory\n"));
+ PQconninfoFree(options);
+ return NULL;
+ }
+ continue;
+ }
+ }
+
+ /*
+ * No environment variable specified or this one isn't set - try
+ * compiled in
+ */
+ if (option->compiled != NULL)
+ {
+ option->val = strdup(option->compiled);
+ if (!option->val)
+ {
+ printfPQExpBuffer(errorMessage,
+ libpq_gettext("out of memory\n"));
+ PQconninfoFree(options);
+ return NULL;
+ }
+ continue;
+ }
+
+ /*
+ * Special handling for user
+ */
+ if (strcmp(option->keyword, "user") == 0)
+ {
+ option->val = pg_fe_getauthname(errorMessage);
+ continue;
+ }
+ }
+
+ return options;
+ }
static char *
conninfo_getval(PQconninfoOption *connOptions,
Index: src/interfaces/libpq/libpq-fe.h
===================================================================
RCS file: /opt/src/cvs/pgsql/src/interfaces/libpq/libpq-fe.h,v
retrieving revision 1.149
diff -c -r1.149 libpq-fe.h
*** src/interfaces/libpq/libpq-fe.h 21 Jan 2010 14:58:53 -0000 1.149
--- src/interfaces/libpq/libpq-fe.h 25 Jan 2010 18:09:55 -0000
***************
*** 226,235 ****
--- 226,237 ----
/* make a new client connection to the backend */
/* Asynchronous (non-blocking) */
extern PGconn *PQconnectStart(const char *conninfo);
+ extern PGconn *PQconnectStartParams(const char **keywords, const char **values);
extern PostgresPollingStatusType PQconnectPoll(PGconn *conn);
/* Synchronous (blocking) */
extern PGconn *PQconnectdb(const char *conninfo);
+ extern PGconn *PQconnectdbParams(const char **keywords, const char **values);
extern PGconn *PQsetdbLogin(const char *pghost, const char *pgport,
const char *pgoptions, const char *pgtty,
const char *dbName,
signature.asc
Description: OpenPGP digital signature
