PQconninfo function for libpq
I'm breaking this out into it's own thread, for my own sanity if
nothing else :) And it's an isolated feature after all.
I still agree with the previous review at
http://archives.postgresql.org/message-id/1349321071.23971.0.camel@vanquo.pezone.net
about keeping the data in more than one place.
Based on that, I've created a different version of this patch,
attached. This way we keep all the data in one struct.
At this point, the patch is untested beyond compiling and a trivial
psql check, because I ran out of time :) But I figured I'd throw it
out there for comments on which version people prefer. (And yes, it's
quite possible I've made a big think-mistake in it somewhere, but
again, better to get some eyes on it early)
My version also contains a fixed version of the docs that should be
moved back into Zoltans version if that's the one we end up
preferring.
Also, a question was buried in the other review which is - are we OK
to remove the requiressl parameter. Both these patches do so, because
the code becomes much simpler if we can do that. It has been
deprecated since 7.2. Is it OK to remove it, or do we need to put back
in the more complex code to deal with both?
Attached is both Zoltans latest patch (v16) and my slightly different approach.
Comments on which approach is best?
Test results from somebody who has the time to look at it? :)
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Attachments:
01-PQconninfo-v16.patchapplication/octet-stream; name=01-PQconninfo-v16.patchDownload
diff -durpN postgresql/doc/src/sgml/libpq.sgml postgresql.1/doc/src/sgml/libpq.sgml
--- postgresql/doc/src/sgml/libpq.sgml 2012-08-03 09:39:30.114266570 +0200
+++ postgresql.1/doc/src/sgml/libpq.sgml 2012-11-20 11:57:41.359859195 +0100
@@ -496,6 +496,49 @@ typedef struct
</listitem>
</varlistentry>
+ <varlistentry id="libpq-pqconninfo">
+ <term><function>PQconninfo</function><indexterm><primary>PQconninfo</></></term>
+ <listitem>
+ <para>
+ Returns the connection options used by a live connection.
+<synopsis>
+/*
+ * Option flags for PQconninfo
+ */
+#define PG_CONNINFO_NORMAL 0x01
+#define PG_CONNINFO_PASSWORD 0x02
+#define PG_CONNINFO_REPLICATION 0x04
+
+PQconninfoOption *PQconninfo(PGconn *conn, int flags);
+</synopsis>
+ </para>
+
+ <para>
+ Returns a connection options array. This can be used to determine
+ all possible <function>PQconnectdb</function> options and their
+ current values that were used to connect to the server. The return
+ value points to an array of <structname>PQconninfoOption</structname>
+ structures, which ends with an entry having a null <structfield>keyword</>
+ pointer. Every notes above for <function>PQconndefaults</function> also apply.
+ An application may present a dialog using the previous settings by:
+<programlisting>
+ PQconninfoOption *options = PQconninfo(conn, PG_CONNINFO_NORMAL | PG_CONNINFO_PASSWORD);
+</programlisting>
+ </para>
+
+ <para>
+ The array returned by the <literal>PG_CONNINFO_REPLICATION</> flag is
+ a subset of <literal>PG_CONNINFO_NORMAL | PG_CONNINFO_PASSWORD</>.
+ This subset excludes some options from the array which are used by the
+ walreceiver module. <application>pg_basebackup</application> uses this
+ pre-filtered list of options to construct <literal>primary_conninfo</>
+ in the automatically generated recovery.conf file.
+ </para>
+
+ </listitem>
+ </varlistentry>
+
+
<varlistentry id="libpq-pqconninfoparse">
<term><function>PQconninfoParse</function><indexterm><primary>PQconninfoParse</></></term>
<listitem>
diff -durpN postgresql/src/interfaces/libpq/exports.txt postgresql.1/src/interfaces/libpq/exports.txt
--- postgresql/src/interfaces/libpq/exports.txt 2012-10-09 09:58:14.342782974 +0200
+++ postgresql.1/src/interfaces/libpq/exports.txt 2012-11-20 10:55:03.396145749 +0100
@@ -164,3 +164,4 @@ PQsetSingleRowMode 161
lo_lseek64 162
lo_tell64 163
lo_truncate64 164
+PQconninfo 165
diff -durpN postgresql/src/interfaces/libpq/fe-connect.c postgresql.1/src/interfaces/libpq/fe-connect.c
--- postgresql/src/interfaces/libpq/fe-connect.c 2012-09-09 08:11:09.470401480 +0200
+++ postgresql.1/src/interfaces/libpq/fe-connect.c 2012-11-20 16:25:11.722295729 +0100
@@ -137,6 +137,9 @@ static int ldapServiceLookup(const char
* PQconninfoOptions[] *must* be NULL. In a working copy, non-null "val"
* fields point to malloc'd strings that should be freed when the working
* array is freed (see PQconninfoFree).
+ *
+ * If you add a new connection option to this list, remember to add it to
+ * PQconninfoMappings[] below.
* ----------
*/
static const PQconninfoOption PQconninfoOptions[] = {
@@ -203,16 +206,6 @@ static const PQconninfoOption PQconninfo
{"keepalives_count", NULL, NULL, NULL,
"TCP-Keepalives-Count", "", 10}, /* strlen(INT32_MAX) == 10 */
-#ifdef USE_SSL
-
- /*
- * "requiressl" is deprecated, its purpose having been taken over by
- * "sslmode". It remains for backwards compatibility.
- */
- {"requiressl", "PGREQUIRESSL", "0", NULL,
- "Require-SSL", "D", 1},
-#endif
-
/*
* ssl options are allowed even without client SSL support because the
* client can still handle SSL modes "disable" and "allow". Other
@@ -264,6 +257,83 @@ static const PQconninfoOption PQconninfo
NULL, NULL, 0}
};
+/*
+ * We need a mapping between the PQconninfoOptions[] array
+ * and PGconn members. We have to keep it separate from
+ * PQconninfoOptions[] to not leak info about PGconn members
+ * to clients.
+ */
+typedef struct PQconninfoMapping {
+ char *keyword;
+ size_t member_offset;
+ int flags;
+} PQconninfoMapping;
+#define PGCONNMEMBERADDR(conn, mapping) ((char **)((char *)conn + mapping->member_offset))
+
+static const PQconninfoMapping PQconninfoMappings[] =
+{
+ /* "authtype" is not used anymore, there is no mapping to PGconn */
+ /* there is no mapping of "service" to PGconn */
+ { "user", offsetof(struct pg_conn, pguser),
+ PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION },
+ { "password", offsetof(struct pg_conn, pgpass),
+ PG_CONNINFO_PASSWORD | PG_CONNINFO_REPLICATION },
+ { "connect_timeout", offsetof(struct pg_conn, connect_timeout),
+ PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION },
+ { "dbname", offsetof(struct pg_conn, dbName),
+ PG_CONNINFO_NORMAL },
+ { "host", offsetof(struct pg_conn, pghost),
+ PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION },
+ { "hostaddr", offsetof(struct pg_conn, pghostaddr),
+ PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION },
+ { "port", offsetof(struct pg_conn, pgport),
+ PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION },
+ { "client_encoding", offsetof(struct pg_conn, client_encoding_initial),
+ PG_CONNINFO_NORMAL },
+ { "tty", offsetof(struct pg_conn, pgtty),
+ PG_CONNINFO_NORMAL },
+ { "options", offsetof(struct pg_conn, pgoptions),
+ PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION },
+ { "application_name", offsetof(struct pg_conn, appname),
+ PG_CONNINFO_NORMAL },
+ { "fallback_application_name", offsetof(struct pg_conn, fbappname),
+ PG_CONNINFO_NORMAL },
+ { "keepalives", offsetof(struct pg_conn, keepalives),
+ PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION },
+ { "keepalives_idle", offsetof(struct pg_conn, keepalives_idle),
+ PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION },
+ { "keepalives_interval", offsetof(struct pg_conn, keepalives_interval),
+ PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION },
+ { "keepalives_count", offsetof(struct pg_conn, keepalives_count),
+ PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION },
+ { "sslmode", offsetof(struct pg_conn, sslmode),
+ PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION },
+ { "sslcompression", offsetof(struct pg_conn, sslcompression),
+ PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION },
+ { "sslcert", offsetof(struct pg_conn, sslcert),
+ PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION },
+ { "sslkey", offsetof(struct pg_conn, sslkey),
+ PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION },
+ { "sslrootcert", offsetof(struct pg_conn, sslrootcert),
+ PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION },
+ { "sslcrl", offsetof(struct pg_conn, sslcrl),
+ PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION },
+ { "requirepeer", offsetof(struct pg_conn, requirepeer),
+ PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION },
+#if defined(KRB5) || defined(ENABLE_GSS) || defined(ENABLE_SSPI)
+ { "krbsrvname", offsetof(struct pg_conn, krbsrvname),
+ PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION },
+#endif
+#if defined(ENABLE_GSS) && defined(ENABLE_SSPI)
+ { "gsslib", offsetof(struct pg_conn, requirepeer),
+ PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION },
+#endif
+ { "replication", offsetof(struct pg_conn, replication),
+ PG_CONNINFO_NORMAL },
+ /* Terminating entry --- MUST BE LAST */
+ { NULL, 0, 0 }
+};
+
static const PQEnvironmentOption EnvironmentOptions[] =
{
/* common user-interface settings */
@@ -295,7 +365,8 @@ static PGconn *makeEmptyPGconn(void);
static void fillPGconn(PGconn *conn, PQconninfoOption *connOptions);
static void freePGconn(PGconn *conn);
static void closePGconn(PGconn *conn);
-static PQconninfoOption *conninfo_init(PQExpBuffer errorMessage);
+static PQconninfoOption *conninfo_init(PQExpBuffer errorMessage,
+ int flags);
static PQconninfoOption *parse_connection_string(const char *conninfo,
PQExpBuffer errorMessage, bool use_defaults);
static int uri_prefix_length(const char *connstr);
@@ -627,7 +698,7 @@ PQconnectStart(const char *conninfo)
static void
fillPGconn(PGconn *conn, PQconninfoOption *connOptions)
{
- const char *tmp;
+ const PQconninfoMapping *mapping;
/*
* Move option values into conn structure
@@ -637,72 +708,19 @@ fillPGconn(PGconn *conn, PQconninfoOptio
*
* XXX: probably worth checking strdup() return value here...
*/
- tmp = conninfo_getval(connOptions, "hostaddr");
- conn->pghostaddr = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "host");
- conn->pghost = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "port");
- conn->pgport = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "tty");
- conn->pgtty = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "options");
- conn->pgoptions = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "application_name");
- conn->appname = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "fallback_application_name");
- conn->fbappname = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "dbname");
- conn->dbName = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "user");
- conn->pguser = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "password");
- conn->pgpass = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "connect_timeout");
- conn->connect_timeout = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "client_encoding");
- conn->client_encoding_initial = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "keepalives");
- conn->keepalives = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "keepalives_idle");
- conn->keepalives_idle = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "keepalives_interval");
- conn->keepalives_interval = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "keepalives_count");
- conn->keepalives_count = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "sslmode");
- conn->sslmode = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "sslcompression");
- conn->sslcompression = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "sslkey");
- conn->sslkey = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "sslcert");
- conn->sslcert = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "sslrootcert");
- conn->sslrootcert = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "sslcrl");
- conn->sslcrl = tmp ? strdup(tmp) : NULL;
-#ifdef USE_SSL
- tmp = conninfo_getval(connOptions, "requiressl");
- if (tmp && tmp[0] == '1')
+ for (mapping = PQconninfoMappings; mapping->keyword; mapping++)
{
- /* here warn that the requiressl option is deprecated? */
- if (conn->sslmode)
- free(conn->sslmode);
- conn->sslmode = strdup("require");
+ const char *tmp = conninfo_getval(connOptions, mapping->keyword);
+
+ if (tmp)
+ {
+ char **connmember = PGCONNMEMBERADDR(conn, mapping);
+
+ if (*connmember)
+ free(*connmember);
+ *connmember = tmp ? strdup(tmp) : NULL;
+ }
}
-#endif
- tmp = conninfo_getval(connOptions, "requirepeer");
- conn->requirepeer = tmp ? strdup(tmp) : NULL;
-#if defined(KRB5) || defined(ENABLE_GSS) || defined(ENABLE_SSPI)
- tmp = conninfo_getval(connOptions, "krbsrvname");
- conn->krbsrvname = tmp ? strdup(tmp) : NULL;
-#endif
-#if defined(ENABLE_GSS) && defined(ENABLE_SSPI)
- tmp = conninfo_getval(connOptions, "gsslib");
- conn->gsslib = tmp ? strdup(tmp) : NULL;
-#endif
- tmp = conninfo_getval(connOptions, "replication");
- conn->replication = tmp ? strdup(tmp) : NULL;
}
/*
@@ -884,7 +902,7 @@ PQconndefaults(void)
if (PQExpBufferDataBroken(errorBuf))
return NULL; /* out of memory already :-( */
- connOptions = conninfo_init(&errorBuf);
+ connOptions = conninfo_init(&errorBuf, PG_CONNINFO_NORMAL | PG_CONNINFO_PASSWORD);
if (connOptions != NULL)
{
if (!conninfo_add_defaults(connOptions, &errorBuf))
@@ -4008,9 +4026,11 @@ PQconninfoParse(const char *conninfo, ch
* Build a working copy of the constant PQconninfoOptions array.
*/
static PQconninfoOption *
-conninfo_init(PQExpBuffer errorMessage)
+conninfo_init(PQExpBuffer errorMessage, int flags)
{
+ const PQconninfoMapping *mapping;
PQconninfoOption *options;
+ PQconninfoOption *opt_dest;
options = (PQconninfoOption *) malloc(sizeof(PQconninfoOptions));
if (options == NULL)
@@ -4019,7 +4039,23 @@ conninfo_init(PQExpBuffer errorMessage)
libpq_gettext("out of memory\n"));
return NULL;
}
- memcpy(options, PQconninfoOptions, sizeof(PQconninfoOptions));
+
+ opt_dest = options;
+ for (mapping = PQconninfoMappings; mapping->keyword; mapping++)
+ {
+ PQconninfoOption *opt_src;
+
+ if (!(mapping->flags & flags))
+ continue;
+
+ opt_src = conninfo_find((PQconninfoOption *)PQconninfoOptions, mapping->keyword);
+ if (opt_src)
+ {
+ memcpy(opt_dest, opt_src, sizeof(PQconninfoOption));
+ opt_dest++;
+ }
+ }
+ MemSet(opt_dest, 0, sizeof(PQconninfoOption));
return options;
}
@@ -4095,7 +4131,7 @@ conninfo_parse(const char *conninfo, PQE
PQconninfoOption *options;
/* Make a working copy of PQconninfoOptions */
- options = conninfo_init(errorMessage);
+ options = conninfo_init(errorMessage, PG_CONNINFO_NORMAL | PG_CONNINFO_PASSWORD);
if (options == NULL)
return NULL;
@@ -4295,7 +4331,7 @@ conninfo_array_parse(const char *const *
}
/* Make a working copy of PQconninfoOptions */
- options = conninfo_init(errorMessage);
+ options = conninfo_init(errorMessage, PG_CONNINFO_NORMAL | PG_CONNINFO_PASSWORD);
if (options == NULL)
{
PQconninfoFree(dbname_options);
@@ -4485,7 +4521,7 @@ conninfo_uri_parse(const char *uri, PQEx
PQconninfoOption *options;
/* Make a working copy of PQconninfoOptions */
- options = conninfo_init(errorMessage);
+ options = conninfo_init(errorMessage, PG_CONNINFO_NORMAL | PG_CONNINFO_PASSWORD);
if (options == NULL)
return NULL;
@@ -5066,6 +5102,45 @@ conninfo_find(PQconninfoOption *connOpti
}
+/*
+ * Return the connection options used for the connections
+ */
+PQconninfoOption *
+PQconninfo(PGconn *conn, int flags)
+{
+ PQExpBufferData errorBuf;
+ PQconninfoOption *connOptions;
+
+ if (conn == NULL)
+ return NULL;
+
+ /* We don't actually report any errors here, but callees want a buffer */
+ initPQExpBuffer(&errorBuf);
+ if (PQExpBufferDataBroken(errorBuf))
+ return NULL; /* out of memory already :-( */
+
+ connOptions = conninfo_init(&errorBuf, flags);
+
+ if (connOptions != NULL)
+ {
+ const PQconninfoMapping *mapping;
+
+ for (mapping = PQconninfoMappings; mapping->keyword; mapping++)
+ {
+ char **connmember = PGCONNMEMBERADDR(conn, mapping);
+
+ if (*connmember)
+ conninfo_storeval(connOptions, mapping->keyword, *connmember,
+ &errorBuf, false, false);
+ }
+ }
+
+ termPQExpBuffer(&errorBuf);
+
+ return connOptions;
+}
+
+
void
PQconninfoFree(PQconninfoOption *connOptions)
{
diff -durpN postgresql/src/interfaces/libpq/libpq-fe.h postgresql.1/src/interfaces/libpq/libpq-fe.h
--- postgresql/src/interfaces/libpq/libpq-fe.h 2012-10-09 09:58:14.343782980 +0200
+++ postgresql.1/src/interfaces/libpq/libpq-fe.h 2012-11-20 11:49:36.747692399 +0100
@@ -36,6 +36,13 @@ extern "C"
#define PG_COPYRES_EVENTS 0x04
#define PG_COPYRES_NOTICEHOOKS 0x08
+/*
+ * Option flags for PQconninfo
+ */
+#define PG_CONNINFO_NORMAL 0x01
+#define PG_CONNINFO_PASSWORD 0x02
+#define PG_CONNINFO_REPLICATION 0x04
+
/* Application-visible enum types */
/*
@@ -262,6 +269,9 @@ extern PQconninfoOption *PQconndefaults(
/* parse connection options in same way as PQconnectdb */
extern PQconninfoOption *PQconninfoParse(const char *conninfo, char **errmsg);
+/* return the connection options used by a live connection */
+extern PQconninfoOption *PQconninfo(PGconn *conn, int flags);
+
/* free the data structure returned by PQconndefaults() or PQconninfoParse() */
extern void PQconninfoFree(PQconninfoOption *connOptions);
PQconninfo-mha.patchapplication/octet-stream; name=PQconninfo-mha.patchDownload
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 255c5c1..e811672 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -496,6 +496,69 @@ typedef struct
</listitem>
</varlistentry>
+ <varlistentry id="libpq-pqconninfo">
+ <term><function>PQconninfo</function><indexterm><primary>PQconninfo</></></term>
+ <listitem>
+ <para>
+ Returns the connection options used by a live connection.
+<synopsis>
+PQconninfoOption *PQconninfo(PGconn *conn, int flags);
+</synopsis>
+ </para>
+
+ <para>
+ Returns a connection options array. This can be used to determine
+ all possible <function>PQconnectdb</function> options and the
+ values that were used to connect to the server. The return
+ value points to an array of <structname>PQconninfoOption</structname>
+ structures, which ends with an entry having a null <structfield>keyword</>
+ pointer. All notes above for <function>PQconndefaults</function> also
+ apply to the result of <function>PQconninfo</function>.
+ </para>
+
+ <para>
+ The parameter <literal>flags</literal> is used to filter which
+ connection options are returned. The flags are a bitmask that can be
+ ORed together to return parameters of multiple types at the same time.
+
+ <variablelist>
+ <varlistentry>
+ <term><symbol>PG_CONNINFO_NORMAL</symbol></term>
+ <listitem>
+ <para>
+ All normal connection options.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><symbol>PG_CONNINFO_PASSWORD</symbol></term>
+ <listitem>
+ <para>
+ All connection options that include passwords or other
+ sensitive information.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><symbol>PG_CONNINFO_REPLICATION</symbol></term>
+ <listitem>
+ <para>
+ All connection options only used for replication
+ connections.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
+
+ It is also possible to specify <literal>PG_CONNINFO_ALL</literal>,
+ which will return all options, regardless of type.
+ </para>
+ </listitem>
+ </varlistentry>
+
+
<varlistentry id="libpq-pqconninfoparse">
<term><function>PQconninfoParse</function><indexterm><primary>PQconninfoParse</></></term>
<listitem>
@@ -1178,28 +1241,6 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
</listitem>
</varlistentry>
- <varlistentry id="libpq-connect-requiressl" xreflabel="requiressl">
- <term><literal>requiressl</literal></term>
- <listitem>
- <para>
- This option is deprecated in favor of the <literal>sslmode</>
- setting.
- </para>
-
- <para>
- If set to 1, an <acronym>SSL</acronym> connection to the server
- is required (this is equivalent to <literal>sslmode</>
- <literal>require</>). <application>libpq</> will then refuse
- to connect if the server does not accept an
- <acronym>SSL</acronym> connection. If set to 0 (default),
- <application>libpq</> will negotiate the connection type with
- the server (equivalent to <literal>sslmode</>
- <literal>prefer</>). This option is only available if
- <productname>PostgreSQL</> is compiled with SSL support.
- </para>
- </listitem>
- </varlistentry>
-
<varlistentry id="libpq-connect-sslcompression" xreflabel="sslcompression">
<term><literal>sslcompression</literal></term>
<listitem>
@@ -6574,16 +6615,6 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough)
<listitem>
<para>
<indexterm>
- <primary><envar>PGREQUIRESSL</envar></primary>
- </indexterm>
- <envar>PGREQUIRESSL</envar> behaves the same as the <xref
- linkend="libpq-connect-requiressl"> connection parameter.
- </para>
- </listitem>
-
- <listitem>
- <para>
- <indexterm>
<primary><envar>PGSSLCOMPRESSION</envar></primary>
</indexterm>
<envar>PGSSLCOMPRESSION</envar> behaves the same as the <xref
diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt
index 56d0bb8..93da50d 100644
--- a/src/interfaces/libpq/exports.txt
+++ b/src/interfaces/libpq/exports.txt
@@ -164,3 +164,4 @@ PQsetSingleRowMode 161
lo_lseek64 162
lo_tell64 163
lo_truncate64 164
+PQconninfo 165
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 9eaf410..b66c6ee 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -137,81 +137,113 @@ static int ldapServiceLookup(const char *purl, PQconninfoOption *options,
* PQconninfoOptions[] *must* be NULL. In a working copy, non-null "val"
* fields point to malloc'd strings that should be freed when the working
* array is freed (see PQconninfoFree).
+ *
+ * The first part of each struct is identical to the one in libpq-fe.h,
+ * which is required since we memcpy() data between the two!
* ----------
*/
-static const PQconninfoOption PQconninfoOptions[] = {
+typedef struct _internalPQconninfoOption
+{
+ char *keyword; /* The keyword of the option */
+ char *envvar; /* Fallback environment variable name */
+ char *compiled; /* Fallback compiled in default value */
+ char *val; /* Option's current value, or NULL */
+ char *label; /* Label for field in connect dialog */
+ char *dispchar; /* Indicates how to display this field in a
+ * connect dialog. Values are: "" Display
+ * entered value as is "*" Password field -
+ * hide value "D" Debug option - don't show
+ * by default */
+ int dispsize; /* Field size in characters for dialog */
+ /* ---
+ * Anything above this comment must be synchronized with
+ * PQconninfoOption in libpq-fe.h, since we memcpy() data
+ * between them!
+ * ---
+ */
+ off_t connofs; /* Offset into PGconn struct, -1 if not there */
+ unsigned int flags; /* type of option */
+} internalPQconninfoOption;
+
+static const internalPQconninfoOption PQconninfoOptions[] = {
/*
* "authtype" is no longer used, so mark it "don't show". We keep it in
* the array so as not to reject conninfo strings from old apps that might
* still try to set it.
*/
{"authtype", "PGAUTHTYPE", DefaultAuthtype, NULL,
- "Database-Authtype", "D", 20},
+ "Database-Authtype", "D", 20, -1, 0},
{"service", "PGSERVICE", NULL, NULL,
- "Database-Service", "", 20},
+ "Database-Service", "", 20, -1, 0},
{"user", "PGUSER", NULL, NULL,
- "Database-User", "", 20},
+ "Database-User", "", 20,
+ offsetof(struct pg_conn, pguser), PG_CONNINFO_NORMAL},
{"password", "PGPASSWORD", NULL, NULL,
- "Database-Password", "*", 20},
+ "Database-Password", "*", 20,
+ offsetof(struct pg_conn, pgpass), PG_CONNINFO_PASSWORD},
{"connect_timeout", "PGCONNECT_TIMEOUT", NULL, NULL,
- "Connect-timeout", "", 10}, /* strlen(INT32_MAX) == 10 */
+ "Connect-timeout", "", 10, /* strlen(INT32_MAX) == 10 */
+ offsetof(struct pg_conn, connect_timeout), PG_CONNINFO_NORMAL},
{"dbname", "PGDATABASE", NULL, NULL,
- "Database-Name", "", 20},
+ "Database-Name", "", 20,
+ offsetof(struct pg_conn, dbName), PG_CONNINFO_NORMAL},
{"host", "PGHOST", NULL, NULL,
- "Database-Host", "", 40},
+ "Database-Host", "", 40,
+ offsetof(struct pg_conn, pghost), PG_CONNINFO_NORMAL},
{"hostaddr", "PGHOSTADDR", NULL, NULL,
- "Database-Host-IP-Address", "", 45},
+ "Database-Host-IP-Address", "", 45,
+ offsetof(struct pg_conn, pghostaddr), PG_CONNINFO_NORMAL},
{"port", "PGPORT", DEF_PGPORT_STR, NULL,
- "Database-Port", "", 6},
+ "Database-Port", "", 6,
+ offsetof(struct pg_conn, pgport), PG_CONNINFO_NORMAL},
{"client_encoding", "PGCLIENTENCODING", NULL, NULL,
- "Client-Encoding", "", 10},
+ "Client-Encoding", "", 10,
+ offsetof(struct pg_conn, client_encoding_initial), PG_CONNINFO_NORMAL},
/*
* "tty" is no longer used either, but keep it present for backwards
* compatibility.
*/
{"tty", "PGTTY", DefaultTty, NULL,
- "Backend-Debug-TTY", "D", 40},
+ "Backend-Debug-TTY", "D", 40,
+ offsetof(struct pg_conn, pgtty), PG_CONNINFO_NORMAL},
{"options", "PGOPTIONS", DefaultOption, NULL,
- "Backend-Debug-Options", "D", 40},
+ "Backend-Debug-Options", "D", 40,
+ offsetof(struct pg_conn, pgoptions), PG_CONNINFO_NORMAL},
{"application_name", "PGAPPNAME", NULL, NULL,
- "Application-Name", "", 64},
+ "Application-Name", "", 64,
+ offsetof(struct pg_conn, appname), PG_CONNINFO_NORMAL},
{"fallback_application_name", NULL, NULL, NULL,
- "Fallback-Application-Name", "", 64},
+ "Fallback-Application-Name", "", 64,
+ offsetof(struct pg_conn, fbappname), PG_CONNINFO_NORMAL},
{"keepalives", NULL, NULL, NULL,
- "TCP-Keepalives", "", 1}, /* should be just '0' or '1' */
+ "TCP-Keepalives", "", 1, /* should be just '0' or '1' */
+ offsetof(struct pg_conn, keepalives), PG_CONNINFO_NORMAL},
{"keepalives_idle", NULL, NULL, NULL,
- "TCP-Keepalives-Idle", "", 10}, /* strlen(INT32_MAX) == 10 */
+ "TCP-Keepalives-Idle", "", 10, /* strlen(INT32_MAX) == 10 */
+ offsetof(struct pg_conn, keepalives_idle), PG_CONNINFO_NORMAL},
{"keepalives_interval", NULL, NULL, NULL,
- "TCP-Keepalives-Interval", "", 10}, /* strlen(INT32_MAX) == 10 */
+ "TCP-Keepalives-Interval", "", 10, /* strlen(INT32_MAX) == 10 */
+ offsetof(struct pg_conn, keepalives_interval), PG_CONNINFO_NORMAL},
{"keepalives_count", NULL, NULL, NULL,
- "TCP-Keepalives-Count", "", 10}, /* strlen(INT32_MAX) == 10 */
-
-#ifdef USE_SSL
-
- /*
- * "requiressl" is deprecated, its purpose having been taken over by
- * "sslmode". It remains for backwards compatibility.
- */
- {"requiressl", "PGREQUIRESSL", "0", NULL,
- "Require-SSL", "D", 1},
-#endif
+ "TCP-Keepalives-Count", "", 10, /* strlen(INT32_MAX) == 10 */
+ offsetof(struct pg_conn, keepalives_count), PG_CONNINFO_NORMAL},
/*
* ssl options are allowed even without client SSL support because the
@@ -220,30 +252,38 @@ static const PQconninfoOption PQconninfoOptions[] = {
* to exclude them since none of them are mandatory.
*/
{"sslmode", "PGSSLMODE", DefaultSSLMode, NULL,
- "SSL-Mode", "", 8}, /* sizeof("disable") == 8 */
+ "SSL-Mode", "", 8, /* sizeof("disable") == 8 */
+ offsetof(struct pg_conn, sslmode), PG_CONNINFO_NORMAL},
{"sslcompression", "PGSSLCOMPRESSION", "1", NULL,
- "SSL-Compression", "", 1},
+ "SSL-Compression", "", 1,
+ offsetof(struct pg_conn, sslcompression), PG_CONNINFO_NORMAL},
{"sslcert", "PGSSLCERT", NULL, NULL,
- "SSL-Client-Cert", "", 64},
+ "SSL-Client-Cert", "", 64,
+ offsetof(struct pg_conn, sslcert), PG_CONNINFO_NORMAL},
{"sslkey", "PGSSLKEY", NULL, NULL,
- "SSL-Client-Key", "", 64},
+ "SSL-Client-Key", "", 64,
+ offsetof(struct pg_conn, sslkey), PG_CONNINFO_NORMAL},
{"sslrootcert", "PGSSLROOTCERT", NULL, NULL,
- "SSL-Root-Certificate", "", 64},
+ "SSL-Root-Certificate", "", 64,
+ offsetof(struct pg_conn, sslrootcert), PG_CONNINFO_NORMAL},
{"sslcrl", "PGSSLCRL", NULL, NULL,
- "SSL-Revocation-List", "", 64},
+ "SSL-Revocation-List", "", 64,
+ offsetof(struct pg_conn, sslcrl), PG_CONNINFO_NORMAL},
{"requirepeer", "PGREQUIREPEER", NULL, NULL,
- "Require-Peer", "", 10},
+ "Require-Peer", "", 10,
+ offsetof(struct pg_conn, requirepeer), PG_CONNINFO_NORMAL},
#if defined(KRB5) || defined(ENABLE_GSS) || defined(ENABLE_SSPI)
/* Kerberos and GSSAPI authentication support specifying the service name */
{"krbsrvname", "PGKRBSRVNAME", PG_KRB_SRVNAM, NULL,
- "Kerberos-service-name", "", 20},
+ "Kerberos-service-name", "", 20,
+ offsetof(struct pg_conn, krbsrvname), PG_CONNINFO_NORMAL},
#endif
#if defined(ENABLE_GSS) && defined(ENABLE_SSPI)
@@ -253,11 +293,13 @@ static const PQconninfoOption PQconninfoOptions[] = {
* default
*/
{"gsslib", "PGGSSLIB", NULL, NULL,
- "GSS-library", "", 7}, /* sizeof("gssapi") = 7 */
+ "GSS-library", "", 7, /* sizeof("gssapi") = 7 */
+ offsetof(struct pg_conn, gsslib), PG_CONNINFO_NORMAL},
#endif
{"replication", NULL, NULL, NULL,
- "Replication", "D", 5},
+ "Replication", "D", 5,
+ offsetof(struct pg_conn, replication), PG_CONNINFO_REPLICATION},
/* Terminating entry --- MUST BE LAST */
{NULL, NULL, NULL, NULL,
@@ -295,7 +337,8 @@ static PGconn *makeEmptyPGconn(void);
static void fillPGconn(PGconn *conn, PQconninfoOption *connOptions);
static void freePGconn(PGconn *conn);
static void closePGconn(PGconn *conn);
-static PQconninfoOption *conninfo_init(PQExpBuffer errorMessage);
+static PQconninfoOption *conninfo_init(PQExpBuffer errorMessage,
+ int flags);
static PQconninfoOption *parse_connection_string(const char *conninfo,
PQExpBuffer errorMessage, bool use_defaults);
static int uri_prefix_length(const char *connstr);
@@ -627,7 +670,7 @@ PQconnectStart(const char *conninfo)
static void
fillPGconn(PGconn *conn, PQconninfoOption *connOptions)
{
- const char *tmp;
+ const internalPQconninfoOption *option;
/*
* Move option values into conn structure
@@ -637,72 +680,19 @@ fillPGconn(PGconn *conn, PQconninfoOption *connOptions)
*
* XXX: probably worth checking strdup() return value here...
*/
- tmp = conninfo_getval(connOptions, "hostaddr");
- conn->pghostaddr = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "host");
- conn->pghost = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "port");
- conn->pgport = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "tty");
- conn->pgtty = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "options");
- conn->pgoptions = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "application_name");
- conn->appname = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "fallback_application_name");
- conn->fbappname = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "dbname");
- conn->dbName = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "user");
- conn->pguser = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "password");
- conn->pgpass = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "connect_timeout");
- conn->connect_timeout = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "client_encoding");
- conn->client_encoding_initial = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "keepalives");
- conn->keepalives = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "keepalives_idle");
- conn->keepalives_idle = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "keepalives_interval");
- conn->keepalives_interval = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "keepalives_count");
- conn->keepalives_count = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "sslmode");
- conn->sslmode = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "sslcompression");
- conn->sslcompression = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "sslkey");
- conn->sslkey = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "sslcert");
- conn->sslcert = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "sslrootcert");
- conn->sslrootcert = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "sslcrl");
- conn->sslcrl = tmp ? strdup(tmp) : NULL;
-#ifdef USE_SSL
- tmp = conninfo_getval(connOptions, "requiressl");
- if (tmp && tmp[0] == '1')
+ for (option = PQconninfoOptions; option->keyword; option++)
{
- /* here warn that the requiressl option is deprecated? */
- if (conn->sslmode)
- free(conn->sslmode);
- conn->sslmode = strdup("require");
+ const char *tmp = conninfo_getval(connOptions, option->keyword);
+
+ if (tmp && option->connofs >= 0)
+ {
+ char **connmember = (char **)((char *)conn + option->connofs);
+
+ if (*connmember)
+ free(*connmember);
+ *connmember = tmp ? strdup(tmp) : NULL;
+ }
}
-#endif
- tmp = conninfo_getval(connOptions, "requirepeer");
- conn->requirepeer = tmp ? strdup(tmp) : NULL;
-#if defined(KRB5) || defined(ENABLE_GSS) || defined(ENABLE_SSPI)
- tmp = conninfo_getval(connOptions, "krbsrvname");
- conn->krbsrvname = tmp ? strdup(tmp) : NULL;
-#endif
-#if defined(ENABLE_GSS) && defined(ENABLE_SSPI)
- tmp = conninfo_getval(connOptions, "gsslib");
- conn->gsslib = tmp ? strdup(tmp) : NULL;
-#endif
- tmp = conninfo_getval(connOptions, "replication");
- conn->replication = tmp ? strdup(tmp) : NULL;
}
/*
@@ -884,7 +874,7 @@ PQconndefaults(void)
if (PQExpBufferDataBroken(errorBuf))
return NULL; /* out of memory already :-( */
- connOptions = conninfo_init(&errorBuf);
+ connOptions = conninfo_init(&errorBuf, PG_CONNINFO_ALL);
if (connOptions != NULL)
{
if (!conninfo_add_defaults(connOptions, &errorBuf))
@@ -4008,18 +3998,35 @@ PQconninfoParse(const char *conninfo, char **errmsg)
* Build a working copy of the constant PQconninfoOptions array.
*/
static PQconninfoOption *
-conninfo_init(PQExpBuffer errorMessage)
+conninfo_init(PQExpBuffer errorMessage, int flags)
{
PQconninfoOption *options;
+ PQconninfoOption *opt_dest;
+ const internalPQconninfoOption *cur_opt;
- options = (PQconninfoOption *) malloc(sizeof(PQconninfoOptions));
+ /*
+ * Get enough memory for all optoins in PQconninfoOptions, even if some
+ * end up being filtered out.
+ */
+ options = (PQconninfoOption *) malloc(sizeof(PQconninfoOption) * sizeof(PQconninfoOptions)/sizeof(PQconninfoOptions[0]));
if (options == NULL)
{
printfPQExpBuffer(errorMessage,
libpq_gettext("out of memory\n"));
return NULL;
}
- memcpy(options, PQconninfoOptions, sizeof(PQconninfoOptions));
+ opt_dest = options;
+
+ for (cur_opt = PQconninfoOptions; cur_opt->keyword; cur_opt++)
+ {
+ if (!(cur_opt->flags & flags))
+ continue;
+
+ /* Only copy the public part of the struct, not the full internal */
+ memcpy(opt_dest, cur_opt, sizeof(PQconninfoOption));
+ opt_dest++;
+ }
+ MemSet(opt_dest, 0, sizeof(PQconninfoOption));
return options;
}
@@ -4095,7 +4102,7 @@ conninfo_parse(const char *conninfo, PQExpBuffer errorMessage,
PQconninfoOption *options;
/* Make a working copy of PQconninfoOptions */
- options = conninfo_init(errorMessage);
+ options = conninfo_init(errorMessage, PG_CONNINFO_ALL);
if (options == NULL)
return NULL;
@@ -4295,7 +4302,7 @@ conninfo_array_parse(const char *const * keywords, const char *const * values,
}
/* Make a working copy of PQconninfoOptions */
- options = conninfo_init(errorMessage);
+ options = conninfo_init(errorMessage, PG_CONNINFO_ALL);
if (options == NULL)
{
PQconninfoFree(dbname_options);
@@ -4485,7 +4492,7 @@ conninfo_uri_parse(const char *uri, PQExpBuffer errorMessage,
PQconninfoOption *options;
/* Make a working copy of PQconninfoOptions */
- options = conninfo_init(errorMessage);
+ options = conninfo_init(errorMessage, PG_CONNINFO_ALL);
if (options == NULL)
return NULL;
@@ -5066,6 +5073,50 @@ conninfo_find(PQconninfoOption *connOptions, const char *keyword)
}
+/*
+ * Return the connection options used for the connections
+ */
+PQconninfoOption *
+PQconninfo(PGconn *conn, unsigned int flags)
+{
+ PQExpBufferData errorBuf;
+ PQconninfoOption *connOptions;
+
+ if (conn == NULL)
+ return NULL;
+
+ /* We don't actually report any errors here, but callees want a buffer */
+ initPQExpBuffer(&errorBuf);
+ if (PQExpBufferDataBroken(errorBuf))
+ return NULL; /* out of memory already :-( */
+
+ connOptions = conninfo_init(&errorBuf, flags);
+
+ if (connOptions != NULL)
+ {
+ const internalPQconninfoOption *option;
+
+ for (option = PQconninfoOptions; option->keyword; option++)
+ {
+ char **connmember;
+
+ if (option->connofs < 0)
+ continue;
+
+ connmember = (char **)((char *)conn + option->connofs);
+
+ if (*connmember)
+ conninfo_storeval(connOptions, option->keyword, *connmember,
+ &errorBuf, false, false);
+ }
+ }
+
+ termPQExpBuffer(&errorBuf);
+
+ return connOptions;
+}
+
+
void
PQconninfoFree(PQconninfoOption *connOptions)
{
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index 0b8d9a6..5038fce 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -36,6 +36,14 @@ extern "C"
#define PG_COPYRES_EVENTS 0x04
#define PG_COPYRES_NOTICEHOOKS 0x08
+/*
+ * Option flags for PQconninfo
+ */
+#define PG_CONNINFO_NORMAL 0x01
+#define PG_CONNINFO_PASSWORD 0x02
+#define PG_CONNINFO_REPLICATION 0x04
+#define PG_CONNINFO_ALL 0xFFFFFFFF
+
/* Application-visible enum types */
/*
@@ -262,6 +270,9 @@ extern PQconninfoOption *PQconndefaults(void);
/* parse connection options in same way as PQconnectdb */
extern PQconninfoOption *PQconninfoParse(const char *conninfo, char **errmsg);
+/* return the connection options used by a live connection */
+extern PQconninfoOption *PQconninfo(PGconn *conn, unsigned int flags);
+
/* free the data structure returned by PQconndefaults() or PQconninfoParse() */
extern void PQconninfoFree(PQconninfoOption *connOptions);
Hi,
2012-11-21 19:19 keltezéssel, Magnus Hagander írta:
I'm breaking this out into it's own thread, for my own sanity if
nothing else :) And it's an isolated feature after all.I still agree with the previous review at
http://archives.postgresql.org/message-id/1349321071.23971.0.camel@vanquo.pezone.net
about keeping the data in more than one place.
OK, it seems I completely missed that comment.
(Or forgot about it if I happened to answer it.)
Based on that, I've created a different version of this patch,
attached. This way we keep all the data in one struct.
I like this single structure but not the way you handle the
options' classes. In your patch, each option belongs to only
one class. These classes are:
PG_CONNINFO_REPLICATION = "replication" only
PG_CONNINFO_PASSWORD = "password" only
PG_CONNINFO_NORMAL = everything else
How does it help pg_basebackup to filter out e.g. dbname and replication?
These are added by the walreceiver module anyway and adding them
to the primary_conninfo line should even be discouraged by the documentation.
In my view, the classes should be inclusive:
PG_CONNINFO_NORMAL = Everything that's usable for a regular client
connection. This mean everything, maybe including "password" but
excluding "replication".
PG_CONNINFO_PASSWORD = "password" only.
PG_CONNINFO_REPLICATION = Everything usable for a replication client
not added by walreceiver. Maybe including/excluding "password".
Maybe there should be two flags for replication usage:
PG_CONNINFO_WALRECEIVER = everything except those not added
by walreceiver (and maybe "password" too)
PG_CONNINFO_REPLICATION = "replication" only
And every option can belong to more than one class, just as in my patch.
At this point, the patch is untested beyond compiling and a trivial
psql check, because I ran out of time :) But I figured I'd throw it
out there for comments on which version people prefer. (And yes, it's
quite possible I've made a big think-mistake in it somewhere, but
again, better to get some eyes on it early)My version also contains a fixed version of the docs that should be
moved back into Zoltans version if that's the one we end up
preferring.
I also liked your version of the documentation better,
I am not too good at writing docs.
Also, a question was buried in the other review which is - are we OK
to remove the requiressl parameter. Both these patches do so, because
the code becomes much simpler if we can do that. It has been
deprecated since 7.2. Is it OK to remove it, or do we need to put back
in the more complex code to deal with both?Attached is both Zoltans latest patch (v16) and my slightly different approach.
Comments on which approach is best?
Test results from somebody who has the time to look at it? :)
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Thanks for four work on this.
Best regards,
Zoltán Böszörményi
--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/
On Wed, Nov 21, 2012 at 10:01 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
Hi,
2012-11-21 19:19 keltezéssel, Magnus Hagander írta:
I'm breaking this out into it's own thread, for my own sanity if
nothing else :) And it's an isolated feature after all.I still agree with the previous review at
http://archives.postgresql.org/message-id/1349321071.23971.0.camel@vanquo.pezone.net
about keeping the data in more than one place.OK, it seems I completely missed that comment.
(Or forgot about it if I happened to answer it.)Based on that, I've created a different version of this patch,
attached. This way we keep all the data in one struct.I like this single structure but not the way you handle the
options' classes. In your patch, each option belongs to only
one class. These classes are:PG_CONNINFO_REPLICATION = "replication" only
PG_CONNINFO_PASSWORD = "password" only
PG_CONNINFO_NORMAL = everything elseHow does it help pg_basebackup to filter out e.g. dbname and replication?
PG_CONNINFO_ALL should give pg_basebackup all it needs, no? Or
actually, it shouldn't have the replication=1 part, right? So it
should just use PG_CONNINFO_NORMAL|PG_CONNINFO_PASSWORD?
These are added by the walreceiver module anyway and adding them
to the primary_conninfo line should even be discouraged by the
documentation.
Hmm. I wasn't actually thinking about the dbname part here, I admit that.
In my view, the classes should be inclusive:
PG_CONNINFO_NORMAL = Everything that's usable for a regular client
connection. This mean everything, maybe including "password" but
excluding "replication".PG_CONNINFO_PASSWORD = "password" only.
PG_CONNINFO_REPLICATION = Everything usable for a replication client
not added by walreceiver. Maybe including/excluding "password".Maybe there should be two flags for replication usage:
PG_CONNINFO_WALRECEIVER = everything except those not added
by walreceiver (and maybe "password" too)PG_CONNINFO_REPLICATION = "replication" only
And every option can belong to more than one class, just as in my patch.
Hmm. I kind of liked having each option in just one class, but I see
the problem. Looking at the ones you suggest, all the non-password
ones *have* to be without password, otherwise there i no way to get
the conninfo without password - which is the whole reason for that
parameter to exist. So the PASSWORD one has to be additional - which
means that not making the other ones additional makes them
inconsistent. But maybe we don't really have a choice there.
At this point, the patch is untested beyond compiling and a trivial
psql check, because I ran out of time :) But I figured I'd throw it
out there for comments on which version people prefer. (And yes, it's
quite possible I've made a big think-mistake in it somewhere, but
again, better to get some eyes on it early)My version also contains a fixed version of the docs that should be
moved back into Zoltans version if that's the one we end up
preferring.I also liked your version of the documentation better,
I am not too good at writing docs.
np.
Also, a question was buried in the other review which is - are we OK
to remove the requiressl parameter. Both these patches do so, because
the code becomes much simpler if we can do that. It has been
deprecated since 7.2. Is it OK to remove it, or do we need to put back
in the more complex code to deal with both?
Just going to highlight that we're looking for at least one third
party to comment on this :)
Attached is both Zoltans latest patch (v16) and my slightly different
approach.Comments on which approach is best?
Test results from somebody who has the time to look at it? :)
Do you happen to have a set of tests you've been running on your
patches? Can you try them again this one?
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
2012-11-21 22:15 keltezéssel, Magnus Hagander írta:
On Wed, Nov 21, 2012 at 10:01 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
Hi,
2012-11-21 19:19 keltezéssel, Magnus Hagander írta:
I'm breaking this out into it's own thread, for my own sanity if
nothing else :) And it's an isolated feature after all.I still agree with the previous review at
http://archives.postgresql.org/message-id/1349321071.23971.0.camel@vanquo.pezone.net
about keeping the data in more than one place.OK, it seems I completely missed that comment.
(Or forgot about it if I happened to answer it.)Based on that, I've created a different version of this patch,
attached. This way we keep all the data in one struct.I like this single structure but not the way you handle the
options' classes. In your patch, each option belongs to only
one class. These classes are:PG_CONNINFO_REPLICATION = "replication" only
PG_CONNINFO_PASSWORD = "password" only
PG_CONNINFO_NORMAL = everything elseHow does it help pg_basebackup to filter out e.g. dbname and replication?
PG_CONNINFO_ALL should give pg_basebackup all it needs, no? Or
actually, it shouldn't have the replication=1 part, right? So it
should just use PG_CONNINFO_NORMAL|PG_CONNINFO_PASSWORD?These are added by the walreceiver module anyway and adding them
to the primary_conninfo line should even be discouraged by the
documentation.Hmm. I wasn't actually thinking about the dbname part here, I admit that.
And not only the dbname, libpqwalreceiver adds these three:
[zozo@localhost libpqwalreceiver]$ grep dbname *
libpqwalreceiver.c: "%s dbname=replication replication=true
fallback_application_name=walreceiver",
I also excluded "application_name" from PG_CONNINFO_REPLICATION
by this reasoning:
- for async replication or single standby, it doesn't matter,
the connection will show up as "walreceiver"
- for sync replication, the administrator has to add the node name
manually via application_name.
In my view, the classes should be inclusive:
PG_CONNINFO_NORMAL = Everything that's usable for a regular client
connection. This mean everything, maybe including "password" but
excluding "replication".PG_CONNINFO_PASSWORD = "password" only.
PG_CONNINFO_REPLICATION = Everything usable for a replication client
not added by walreceiver. Maybe including/excluding "password".Maybe there should be two flags for replication usage:
PG_CONNINFO_WALRECEIVER = everything except those not added
by walreceiver (and maybe "password" too)PG_CONNINFO_REPLICATION = "replication" only
And every option can belong to more than one class, just as in my patch.
Hmm. I kind of liked having each option in just one class, but I see
the problem. Looking at the ones you suggest, all the non-password
ones *have* to be without password, otherwise there i no way to get
the conninfo without password - which is the whole reason for that
parameter to exist. So the PASSWORD one has to be additional - which
means that not making the other ones additional makes them
inconsistent. But maybe we don't really have a choice there.
Yes, the PASSWORD part can be on its own, this is what I meant above
but wanted a different opinion about having it completely separate
is better or not.
But the NORMAL and REPLICATION (or WALRECEIVER) classes
need to overlap.
At this point, the patch is untested beyond compiling and a trivial
psql check, because I ran out of time :) But I figured I'd throw it
out there for comments on which version people prefer. (And yes, it's
quite possible I've made a big think-mistake in it somewhere, but
again, better to get some eyes on it early)My version also contains a fixed version of the docs that should be
moved back into Zoltans version if that's the one we end up
preferring.I also liked your version of the documentation better,
I am not too good at writing docs.np.
Also, a question was buried in the other review which is - are we OK
to remove the requiressl parameter. Both these patches do so, because
the code becomes much simpler if we can do that. It has been
deprecated since 7.2. Is it OK to remove it, or do we need to put back
in the more complex code to deal with both?Just going to highlight that we're looking for at least one third
party to comment on this :)
Yes, me too. A +1 for removing wouldn't count from me. ;-)
Attached is both Zoltans latest patch (v16) and my slightly different
approach.Comments on which approach is best?
Test results from somebody who has the time to look at it? :)
Do you happen to have a set of tests you've been running on your
patches? Can you try them again this one?
My set of tests are:
1. initdb the master
2. pg_basebackup -R the first standby from the master
3. pg_basebackup -R the second standby from the master
4. pg_basebackup -R the third standby from the first standby
and diff -durpN the different data directories while there is no load on either.
I will test it today after fixing the classes in your patch. ;-)
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/
2012-11-22 08:35 keltezéssel, Boszormenyi Zoltan írta:
2012-11-21 22:15 keltezéssel, Magnus Hagander írta:
On Wed, Nov 21, 2012 at 10:01 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
Hi,
2012-11-21 19:19 keltezéssel, Magnus Hagander írta:
I'm breaking this out into it's own thread, for my own sanity if
nothing else :) And it's an isolated feature after all.I still agree with the previous review at
http://archives.postgresql.org/message-id/1349321071.23971.0.camel@vanquo.pezone.net
about keeping the data in more than one place.OK, it seems I completely missed that comment.
(Or forgot about it if I happened to answer it.)Based on that, I've created a different version of this patch,
attached. This way we keep all the data in one struct.I like this single structure but not the way you handle the
options' classes. In your patch, each option belongs to only
one class. These classes are:PG_CONNINFO_REPLICATION = "replication" only
PG_CONNINFO_PASSWORD = "password" only
PG_CONNINFO_NORMAL = everything elseHow does it help pg_basebackup to filter out e.g. dbname and replication?
PG_CONNINFO_ALL should give pg_basebackup all it needs, no? Or
actually, it shouldn't have the replication=1 part, right? So it
should just use PG_CONNINFO_NORMAL|PG_CONNINFO_PASSWORD?These are added by the walreceiver module anyway and adding them
to the primary_conninfo line should even be discouraged by the
documentation.Hmm. I wasn't actually thinking about the dbname part here, I admit that.
And not only the dbname, libpqwalreceiver adds these three:
[zozo@localhost libpqwalreceiver]$ grep dbname *
libpqwalreceiver.c: "%s dbname=replication replication=true
fallback_application_name=walreceiver",I also excluded "application_name" from PG_CONNINFO_REPLICATION
by this reasoning:- for async replication or single standby, it doesn't matter,
the connection will show up as "walreceiver"
- for sync replication, the administrator has to add the node name
manually via application_name.In my view, the classes should be inclusive:
PG_CONNINFO_NORMAL = Everything that's usable for a regular client
connection. This mean everything, maybe including "password" but
excluding "replication".PG_CONNINFO_PASSWORD = "password" only.
PG_CONNINFO_REPLICATION = Everything usable for a replication client
not added by walreceiver. Maybe including/excluding "password".Maybe there should be two flags for replication usage:
PG_CONNINFO_WALRECEIVER = everything except those not added
by walreceiver (and maybe "password" too)PG_CONNINFO_REPLICATION = "replication" only
And every option can belong to more than one class, just as in my patch.
Hmm. I kind of liked having each option in just one class, but I see
the problem. Looking at the ones you suggest, all the non-password
ones *have* to be without password, otherwise there i no way to get
the conninfo without password - which is the whole reason for that
parameter to exist. So the PASSWORD one has to be additional - which
means that not making the other ones additional makes them
inconsistent. But maybe we don't really have a choice there.Yes, the PASSWORD part can be on its own, this is what I meant above
but wanted a different opinion about having it completely separate
is better or not.But the NORMAL and REPLICATION (or WALRECEIVER) classes
need to overlap.At this point, the patch is untested beyond compiling and a trivial
psql check, because I ran out of time :) But I figured I'd throw it
out there for comments on which version people prefer. (And yes, it's
quite possible I've made a big think-mistake in it somewhere, but
again, better to get some eyes on it early)My version also contains a fixed version of the docs that should be
moved back into Zoltans version if that's the one we end up
preferring.I also liked your version of the documentation better,
I am not too good at writing docs.np.
Also, a question was buried in the other review which is - are we OK
to remove the requiressl parameter. Both these patches do so, because
the code becomes much simpler if we can do that. It has been
deprecated since 7.2. Is it OK to remove it, or do we need to put back
in the more complex code to deal with both?Just going to highlight that we're looking for at least one third
party to comment on this :)Yes, me too. A +1 for removing wouldn't count from me. ;-)
Attached is both Zoltans latest patch (v16) and my slightly different
approach.Comments on which approach is best?
Test results from somebody who has the time to look at it? :)
Do you happen to have a set of tests you've been running on your
patches? Can you try them again this one?My set of tests are:
1. initdb the master
2. pg_basebackup -R the first standby from the master
3. pg_basebackup -R the second standby from the master
4. pg_basebackup -R the third standby from the first standbyand diff -durpN the different data directories while there is no load on either.
I will test it today after fixing the classes in your patch. ;-)
Attached is my v17 patch based on your version with the classes fixed.
I introduced 2 new classes: PG_CONNINFO_REPLICATION_HIDDEN
which contains "dbname" and "fallback_application_name, and
PG_CONNINFO_REPLICATION_USER which contains everything else
except "replication".
Since pg_basebackup sets fallback_application_name and not
application_name, this latter can be part of PG_CONNINFO_REPLICATION_USER.
The REPLICATION and REPLICATION_HIDDEN options may be united,
I don't have a strong opinion on it.
I also flipped the "ignoreMissing" argument to conninfo_storeval()
from false to true in PQconninfo() since we don't want error
reporting there.
This patch works with my pg_basebackup patch after fixing the flags value.
Best regards,
Zoltán Böszörményi
--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/
Attachments:
01-PQconninfo-v17.patchtext/x-patch; name=01-PQconninfo-v17.patchDownload
diff -durpN postgresql/doc/src/sgml/libpq.sgml postgresql.1/doc/src/sgml/libpq.sgml
--- postgresql/doc/src/sgml/libpq.sgml 2012-08-03 09:39:30.114266570 +0200
+++ postgresql.1/doc/src/sgml/libpq.sgml 2012-11-22 09:16:29.476791009 +0100
@@ -496,6 +496,89 @@ typedef struct
</listitem>
</varlistentry>
+ <varlistentry id="libpq-pqconninfo">
+ <term><function>PQconninfo</function><indexterm><primary>PQconninfo</></></term>
+ <listitem>
+ <para>
+ Returns the connection options used by a live connection.
+<synopsis>
+PQconninfoOption *PQconninfo(PGconn *conn, int flags);
+</synopsis>
+ </para>
+
+ <para>
+ Returns a connection options array. This can be used to determine
+ all possible <function>PQconnectdb</function> options and the
+ values that were used to connect to the server. The return
+ value points to an array of <structname>PQconninfoOption</structname>
+ structures, which ends with an entry having a null <structfield>keyword</>
+ pointer. All notes above for <function>PQconndefaults</function> also
+ apply to the result of <function>PQconninfo</function>.
+ </para>
+
+ <para>
+ The parameter <literal>flags</literal> is used to filter which
+ connection options are returned. The flags are a bitmask that can be
+ ORed together to return parameters of multiple types at the same time.
+
+ <variablelist>
+ <varlistentry>
+ <term><symbol>PG_CONNINFO_NORMAL</symbol></term>
+ <listitem>
+ <para>
+ All normal connection options.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><symbol>PG_CONNINFO_PASSWORD</symbol></term>
+ <listitem>
+ <para>
+ All connection options that include passwords or other
+ sensitive information.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><symbol>PG_CONNINFO_REPLICATION</symbol></term>
+ <listitem>
+ <para>
+ The "replication" connection option that is used for replication
+ connections.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><symbol>PG_CONNINFO_REPLICATION_HIDDEN</symbol></term>
+ <listitem>
+ <para>
+ All connection options that are used internally by the
+ libpqwalreceiver module.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><symbol>PG_CONNINFO_REPLICATION_USER</symbol></term>
+ <listitem>
+ <para>
+ All connection options that are user-settable for replication
+ connections.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
+
+ It is also possible to specify <literal>PG_CONNINFO_ALL</literal>,
+ which will return all options, regardless of type.
+ </para>
+ </listitem>
+ </varlistentry>
+
+
<varlistentry id="libpq-pqconninfoparse">
<term><function>PQconninfoParse</function><indexterm><primary>PQconninfoParse</></></term>
<listitem>
@@ -1178,28 +1261,6 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/d
</listitem>
</varlistentry>
- <varlistentry id="libpq-connect-requiressl" xreflabel="requiressl">
- <term><literal>requiressl</literal></term>
- <listitem>
- <para>
- This option is deprecated in favor of the <literal>sslmode</>
- setting.
- </para>
-
- <para>
- If set to 1, an <acronym>SSL</acronym> connection to the server
- is required (this is equivalent to <literal>sslmode</>
- <literal>require</>). <application>libpq</> will then refuse
- to connect if the server does not accept an
- <acronym>SSL</acronym> connection. If set to 0 (default),
- <application>libpq</> will negotiate the connection type with
- the server (equivalent to <literal>sslmode</>
- <literal>prefer</>). This option is only available if
- <productname>PostgreSQL</> is compiled with SSL support.
- </para>
- </listitem>
- </varlistentry>
-
<varlistentry id="libpq-connect-sslcompression" xreflabel="sslcompression">
<term><literal>sslcompression</literal></term>
<listitem>
@@ -6571,16 +6632,6 @@ myEventProc(PGEventId evtId, void *evtIn
</para>
</listitem>
- <listitem>
- <para>
- <indexterm>
- <primary><envar>PGREQUIRESSL</envar></primary>
- </indexterm>
- <envar>PGREQUIRESSL</envar> behaves the same as the <xref
- linkend="libpq-connect-requiressl"> connection parameter.
- </para>
- </listitem>
-
<listitem>
<para>
<indexterm>
diff -durpN postgresql/src/interfaces/libpq/exports.txt postgresql.1/src/interfaces/libpq/exports.txt
--- postgresql/src/interfaces/libpq/exports.txt 2012-10-09 09:58:14.342782974 +0200
+++ postgresql.1/src/interfaces/libpq/exports.txt 2012-11-22 08:44:17.228315509 +0100
@@ -164,3 +164,4 @@ PQsetSingleRowMode 161
lo_lseek64 162
lo_tell64 163
lo_truncate64 164
+PQconninfo 165
diff -durpN postgresql/src/interfaces/libpq/fe-connect.c postgresql.1/src/interfaces/libpq/fe-connect.c
--- postgresql/src/interfaces/libpq/fe-connect.c 2012-09-09 08:11:09.470401480 +0200
+++ postgresql.1/src/interfaces/libpq/fe-connect.c 2012-11-22 10:02:59.733581698 +0100
@@ -137,81 +137,113 @@ static int ldapServiceLookup(const char
* PQconninfoOptions[] *must* be NULL. In a working copy, non-null "val"
* fields point to malloc'd strings that should be freed when the working
* array is freed (see PQconninfoFree).
+ *
+ * The first part of each struct is identical to the one in libpq-fe.h,
+ * which is required since we memcpy() data between the two!
* ----------
*/
-static const PQconninfoOption PQconninfoOptions[] = {
+typedef struct _internalPQconninfoOption
+{
+ char *keyword; /* The keyword of the option */
+ char *envvar; /* Fallback environment variable name */
+ char *compiled; /* Fallback compiled in default value */
+ char *val; /* Option's current value, or NULL */
+ char *label; /* Label for field in connect dialog */
+ char *dispchar; /* Indicates how to display this field in a
+ * connect dialog. Values are: "" Display
+ * entered value as is "*" Password field -
+ * hide value "D" Debug option - don't show
+ * by default */
+ int dispsize; /* Field size in characters for dialog */
+ /* ---
+ * Anything above this comment must be synchronized with
+ * PQconninfoOption in libpq-fe.h, since we memcpy() data
+ * between them!
+ * ---
+ */
+ off_t connofs; /* Offset into PGconn struct, -1 if not there */
+ unsigned int flags; /* type of option */
+} internalPQconninfoOption;
+
+static const internalPQconninfoOption PQconninfoOptions[] = {
/*
* "authtype" is no longer used, so mark it "don't show". We keep it in
* the array so as not to reject conninfo strings from old apps that might
* still try to set it.
*/
{"authtype", "PGAUTHTYPE", DefaultAuthtype, NULL,
- "Database-Authtype", "D", 20},
+ "Database-Authtype", "D", 20, -1, 0},
{"service", "PGSERVICE", NULL, NULL,
- "Database-Service", "", 20},
+ "Database-Service", "", 20, -1, 0},
{"user", "PGUSER", NULL, NULL,
- "Database-User", "", 20},
+ "Database-User", "", 20,
+ offsetof(struct pg_conn, pguser), PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION_USER},
{"password", "PGPASSWORD", NULL, NULL,
- "Database-Password", "*", 20},
+ "Database-Password", "*", 20,
+ offsetof(struct pg_conn, pgpass), PG_CONNINFO_PASSWORD},
{"connect_timeout", "PGCONNECT_TIMEOUT", NULL, NULL,
- "Connect-timeout", "", 10}, /* strlen(INT32_MAX) == 10 */
+ "Connect-timeout", "", 10, /* strlen(INT32_MAX) == 10 */
+ offsetof(struct pg_conn, connect_timeout), PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION_USER},
{"dbname", "PGDATABASE", NULL, NULL,
- "Database-Name", "", 20},
+ "Database-Name", "", 20,
+ offsetof(struct pg_conn, dbName), PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION_HIDDEN},
{"host", "PGHOST", NULL, NULL,
- "Database-Host", "", 40},
+ "Database-Host", "", 40,
+ offsetof(struct pg_conn, pghost), PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION_USER},
{"hostaddr", "PGHOSTADDR", NULL, NULL,
- "Database-Host-IP-Address", "", 45},
+ "Database-Host-IP-Address", "", 45,
+ offsetof(struct pg_conn, pghostaddr), PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION_USER},
{"port", "PGPORT", DEF_PGPORT_STR, NULL,
- "Database-Port", "", 6},
+ "Database-Port", "", 6,
+ offsetof(struct pg_conn, pgport), PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION_USER},
{"client_encoding", "PGCLIENTENCODING", NULL, NULL,
- "Client-Encoding", "", 10},
+ "Client-Encoding", "", 10,
+ offsetof(struct pg_conn, client_encoding_initial), PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION_USER},
/*
* "tty" is no longer used either, but keep it present for backwards
- * compatibility.
+ * compatibility. Leave it out from the REPLICATION classes.
*/
{"tty", "PGTTY", DefaultTty, NULL,
- "Backend-Debug-TTY", "D", 40},
+ "Backend-Debug-TTY", "D", 40,
+ offsetof(struct pg_conn, pgtty), PG_CONNINFO_NORMAL},
{"options", "PGOPTIONS", DefaultOption, NULL,
- "Backend-Debug-Options", "D", 40},
+ "Backend-Debug-Options", "D", 40,
+ offsetof(struct pg_conn, pgoptions), PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION_USER},
{"application_name", "PGAPPNAME", NULL, NULL,
- "Application-Name", "", 64},
+ "Application-Name", "", 64,
+ offsetof(struct pg_conn, appname), PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION_USER},
{"fallback_application_name", NULL, NULL, NULL,
- "Fallback-Application-Name", "", 64},
+ "Fallback-Application-Name", "", 64,
+ offsetof(struct pg_conn, fbappname), PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION_HIDDEN},
{"keepalives", NULL, NULL, NULL,
- "TCP-Keepalives", "", 1}, /* should be just '0' or '1' */
+ "TCP-Keepalives", "", 1, /* should be just '0' or '1' */
+ offsetof(struct pg_conn, keepalives), PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION_USER},
{"keepalives_idle", NULL, NULL, NULL,
- "TCP-Keepalives-Idle", "", 10}, /* strlen(INT32_MAX) == 10 */
+ "TCP-Keepalives-Idle", "", 10, /* strlen(INT32_MAX) == 10 */
+ offsetof(struct pg_conn, keepalives_idle), PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION_USER},
{"keepalives_interval", NULL, NULL, NULL,
- "TCP-Keepalives-Interval", "", 10}, /* strlen(INT32_MAX) == 10 */
+ "TCP-Keepalives-Interval", "", 10, /* strlen(INT32_MAX) == 10 */
+ offsetof(struct pg_conn, keepalives_interval), PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION_USER},
{"keepalives_count", NULL, NULL, NULL,
- "TCP-Keepalives-Count", "", 10}, /* strlen(INT32_MAX) == 10 */
-
-#ifdef USE_SSL
-
- /*
- * "requiressl" is deprecated, its purpose having been taken over by
- * "sslmode". It remains for backwards compatibility.
- */
- {"requiressl", "PGREQUIRESSL", "0", NULL,
- "Require-SSL", "D", 1},
-#endif
+ "TCP-Keepalives-Count", "", 10, /* strlen(INT32_MAX) == 10 */
+ offsetof(struct pg_conn, keepalives_count), PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION_USER},
/*
* ssl options are allowed even without client SSL support because the
@@ -220,30 +252,38 @@ static const PQconninfoOption PQconninfo
* to exclude them since none of them are mandatory.
*/
{"sslmode", "PGSSLMODE", DefaultSSLMode, NULL,
- "SSL-Mode", "", 8}, /* sizeof("disable") == 8 */
+ "SSL-Mode", "", 8, /* sizeof("disable") == 8 */
+ offsetof(struct pg_conn, sslmode), PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION_USER},
{"sslcompression", "PGSSLCOMPRESSION", "1", NULL,
- "SSL-Compression", "", 1},
+ "SSL-Compression", "", 1,
+ offsetof(struct pg_conn, sslcompression), PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION_USER},
{"sslcert", "PGSSLCERT", NULL, NULL,
- "SSL-Client-Cert", "", 64},
+ "SSL-Client-Cert", "", 64,
+ offsetof(struct pg_conn, sslcert), PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION_USER},
{"sslkey", "PGSSLKEY", NULL, NULL,
- "SSL-Client-Key", "", 64},
+ "SSL-Client-Key", "", 64,
+ offsetof(struct pg_conn, sslkey), PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION_USER},
{"sslrootcert", "PGSSLROOTCERT", NULL, NULL,
- "SSL-Root-Certificate", "", 64},
+ "SSL-Root-Certificate", "", 64,
+ offsetof(struct pg_conn, sslrootcert), PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION_USER},
{"sslcrl", "PGSSLCRL", NULL, NULL,
- "SSL-Revocation-List", "", 64},
+ "SSL-Revocation-List", "", 64,
+ offsetof(struct pg_conn, sslcrl), PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION_USER},
{"requirepeer", "PGREQUIREPEER", NULL, NULL,
- "Require-Peer", "", 10},
+ "Require-Peer", "", 10,
+ offsetof(struct pg_conn, requirepeer), PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION_USER},
#if defined(KRB5) || defined(ENABLE_GSS) || defined(ENABLE_SSPI)
/* Kerberos and GSSAPI authentication support specifying the service name */
{"krbsrvname", "PGKRBSRVNAME", PG_KRB_SRVNAM, NULL,
- "Kerberos-service-name", "", 20},
+ "Kerberos-service-name", "", 20,
+ offsetof(struct pg_conn, krbsrvname), PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION_USER},
#endif
#if defined(ENABLE_GSS) && defined(ENABLE_SSPI)
@@ -253,11 +293,13 @@ static const PQconninfoOption PQconninfo
* default
*/
{"gsslib", "PGGSSLIB", NULL, NULL,
- "GSS-library", "", 7}, /* sizeof("gssapi") = 7 */
+ "GSS-library", "", 7, /* sizeof("gssapi") = 7 */
+ offsetof(struct pg_conn, gsslib), PG_CONNINFO_NORMAL | PG_CONNINFO_REPLICATION_USER},
#endif
{"replication", NULL, NULL, NULL,
- "Replication", "D", 5},
+ "Replication", "D", 5,
+ offsetof(struct pg_conn, replication), PG_CONNINFO_REPLICATION},
/* Terminating entry --- MUST BE LAST */
{NULL, NULL, NULL, NULL,
@@ -295,7 +337,8 @@ static PGconn *makeEmptyPGconn(void);
static void fillPGconn(PGconn *conn, PQconninfoOption *connOptions);
static void freePGconn(PGconn *conn);
static void closePGconn(PGconn *conn);
-static PQconninfoOption *conninfo_init(PQExpBuffer errorMessage);
+static PQconninfoOption *conninfo_init(PQExpBuffer errorMessage,
+ int flags);
static PQconninfoOption *parse_connection_string(const char *conninfo,
PQExpBuffer errorMessage, bool use_defaults);
static int uri_prefix_length(const char *connstr);
@@ -627,7 +670,7 @@ PQconnectStart(const char *conninfo)
static void
fillPGconn(PGconn *conn, PQconninfoOption *connOptions)
{
- const char *tmp;
+ const internalPQconninfoOption *option;
/*
* Move option values into conn structure
@@ -637,72 +680,19 @@ fillPGconn(PGconn *conn, PQconninfoOptio
*
* XXX: probably worth checking strdup() return value here...
*/
- tmp = conninfo_getval(connOptions, "hostaddr");
- conn->pghostaddr = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "host");
- conn->pghost = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "port");
- conn->pgport = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "tty");
- conn->pgtty = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "options");
- conn->pgoptions = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "application_name");
- conn->appname = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "fallback_application_name");
- conn->fbappname = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "dbname");
- conn->dbName = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "user");
- conn->pguser = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "password");
- conn->pgpass = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "connect_timeout");
- conn->connect_timeout = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "client_encoding");
- conn->client_encoding_initial = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "keepalives");
- conn->keepalives = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "keepalives_idle");
- conn->keepalives_idle = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "keepalives_interval");
- conn->keepalives_interval = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "keepalives_count");
- conn->keepalives_count = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "sslmode");
- conn->sslmode = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "sslcompression");
- conn->sslcompression = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "sslkey");
- conn->sslkey = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "sslcert");
- conn->sslcert = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "sslrootcert");
- conn->sslrootcert = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "sslcrl");
- conn->sslcrl = tmp ? strdup(tmp) : NULL;
-#ifdef USE_SSL
- tmp = conninfo_getval(connOptions, "requiressl");
- if (tmp && tmp[0] == '1')
+ for (option = PQconninfoOptions; option->keyword; option++)
{
- /* here warn that the requiressl option is deprecated? */
- if (conn->sslmode)
- free(conn->sslmode);
- conn->sslmode = strdup("require");
+ const char *tmp = conninfo_getval(connOptions, option->keyword);
+
+ if (tmp && option->connofs >= 0)
+ {
+ char **connmember = (char **)((char *)conn + option->connofs);
+
+ if (*connmember)
+ free(*connmember);
+ *connmember = tmp ? strdup(tmp) : NULL;
+ }
}
-#endif
- tmp = conninfo_getval(connOptions, "requirepeer");
- conn->requirepeer = tmp ? strdup(tmp) : NULL;
-#if defined(KRB5) || defined(ENABLE_GSS) || defined(ENABLE_SSPI)
- tmp = conninfo_getval(connOptions, "krbsrvname");
- conn->krbsrvname = tmp ? strdup(tmp) : NULL;
-#endif
-#if defined(ENABLE_GSS) && defined(ENABLE_SSPI)
- tmp = conninfo_getval(connOptions, "gsslib");
- conn->gsslib = tmp ? strdup(tmp) : NULL;
-#endif
- tmp = conninfo_getval(connOptions, "replication");
- conn->replication = tmp ? strdup(tmp) : NULL;
}
/*
@@ -884,7 +874,7 @@ PQconndefaults(void)
if (PQExpBufferDataBroken(errorBuf))
return NULL; /* out of memory already :-( */
- connOptions = conninfo_init(&errorBuf);
+ connOptions = conninfo_init(&errorBuf, PG_CONNINFO_ALL);
if (connOptions != NULL)
{
if (!conninfo_add_defaults(connOptions, &errorBuf))
@@ -4008,18 +3998,35 @@ PQconninfoParse(const char *conninfo, ch
* Build a working copy of the constant PQconninfoOptions array.
*/
static PQconninfoOption *
-conninfo_init(PQExpBuffer errorMessage)
+conninfo_init(PQExpBuffer errorMessage, int flags)
{
PQconninfoOption *options;
+ PQconninfoOption *opt_dest;
+ const internalPQconninfoOption *cur_opt;
- options = (PQconninfoOption *) malloc(sizeof(PQconninfoOptions));
+ /*
+ * Get enough memory for all options in PQconninfoOptions, even if some
+ * end up being filtered out.
+ */
+ options = (PQconninfoOption *) malloc(sizeof(PQconninfoOption) * sizeof(PQconninfoOptions)/sizeof(PQconninfoOptions[0]));
if (options == NULL)
{
printfPQExpBuffer(errorMessage,
libpq_gettext("out of memory\n"));
return NULL;
}
- memcpy(options, PQconninfoOptions, sizeof(PQconninfoOptions));
+ opt_dest = options;
+
+ for (cur_opt = PQconninfoOptions; cur_opt->keyword; cur_opt++)
+ {
+ if (!(cur_opt->flags & flags))
+ continue;
+
+ /* Only copy the public part of the struct, not the full internal */
+ memcpy(opt_dest, cur_opt, sizeof(PQconninfoOption));
+ opt_dest++;
+ }
+ MemSet(opt_dest, 0, sizeof(PQconninfoOption));
return options;
}
@@ -4095,7 +4102,7 @@ conninfo_parse(const char *conninfo, PQE
PQconninfoOption *options;
/* Make a working copy of PQconninfoOptions */
- options = conninfo_init(errorMessage);
+ options = conninfo_init(errorMessage, PG_CONNINFO_ALL);
if (options == NULL)
return NULL;
@@ -4295,7 +4302,7 @@ conninfo_array_parse(const char *const *
}
/* Make a working copy of PQconninfoOptions */
- options = conninfo_init(errorMessage);
+ options = conninfo_init(errorMessage, PG_CONNINFO_ALL);
if (options == NULL)
{
PQconninfoFree(dbname_options);
@@ -4485,7 +4492,7 @@ conninfo_uri_parse(const char *uri, PQEx
PQconninfoOption *options;
/* Make a working copy of PQconninfoOptions */
- options = conninfo_init(errorMessage);
+ options = conninfo_init(errorMessage, PG_CONNINFO_ALL);
if (options == NULL)
return NULL;
@@ -5066,6 +5073,50 @@ conninfo_find(PQconninfoOption *connOpti
}
+/*
+ * Return the connection options used for the connections
+ */
+PQconninfoOption *
+PQconninfo(PGconn *conn, unsigned int flags)
+{
+ PQExpBufferData errorBuf;
+ PQconninfoOption *connOptions;
+
+ if (conn == NULL)
+ return NULL;
+
+ /* We don't actually report any errors here, but callees want a buffer */
+ initPQExpBuffer(&errorBuf);
+ if (PQExpBufferDataBroken(errorBuf))
+ return NULL; /* out of memory already :-( */
+
+ connOptions = conninfo_init(&errorBuf, flags);
+
+ if (connOptions != NULL)
+ {
+ const internalPQconninfoOption *option;
+
+ for (option = PQconninfoOptions; option->keyword; option++)
+ {
+ char **connmember;
+
+ if (option->connofs < 0)
+ continue;
+
+ connmember = (char **)((char *)conn + option->connofs);
+
+ if (*connmember)
+ conninfo_storeval(connOptions, option->keyword, *connmember,
+ &errorBuf, true, false);
+ }
+ }
+
+ termPQExpBuffer(&errorBuf);
+
+ return connOptions;
+}
+
+
void
PQconninfoFree(PQconninfoOption *connOptions)
{
diff -durpN postgresql/src/interfaces/libpq/libpq-fe.h postgresql.1/src/interfaces/libpq/libpq-fe.h
--- postgresql/src/interfaces/libpq/libpq-fe.h 2012-10-09 09:58:14.343782980 +0200
+++ postgresql.1/src/interfaces/libpq/libpq-fe.h 2012-11-22 09:14:13.144868898 +0100
@@ -36,6 +36,16 @@ extern "C"
#define PG_COPYRES_EVENTS 0x04
#define PG_COPYRES_NOTICEHOOKS 0x08
+/*
+ * Option flags for PQconninfo
+ */
+#define PG_CONNINFO_NORMAL 0x01
+#define PG_CONNINFO_PASSWORD 0x02
+#define PG_CONNINFO_REPLICATION 0x04
+#define PG_CONNINFO_REPLICATION_HIDDEN 0x08
+#define PG_CONNINFO_REPLICATION_USER 0x10
+#define PG_CONNINFO_ALL 0xFFFFFFFF
+
/* Application-visible enum types */
/*
@@ -262,6 +272,9 @@ extern PQconninfoOption *PQconndefaults(
/* parse connection options in same way as PQconnectdb */
extern PQconninfoOption *PQconninfoParse(const char *conninfo, char **errmsg);
+/* return the connection options used by a live connection */
+extern PQconninfoOption *PQconninfo(PGconn *conn, unsigned int flags);
+
/* free the data structure returned by PQconndefaults() or PQconninfoParse() */
extern void PQconninfoFree(PQconninfoOption *connOptions);
On Thu, Nov 22, 2012 at 10:16 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
2012-11-22 08:35 keltezéssel, Boszormenyi Zoltan írta:
2012-11-21 22:15 keltezéssel, Magnus Hagander írta:
On Wed, Nov 21, 2012 at 10:01 PM, Boszormenyi Zoltan <zb@cybertec.at>
wrote:Hi,
2012-11-21 19:19 keltezéssel, Magnus Hagander írta:
I'm breaking this out into it's own thread, for my own sanity if
nothing else :) And it's an isolated feature after all.I still agree with the previous review at
http://archives.postgresql.org/message-id/1349321071.23971.0.camel@vanquo.pezone.net
about keeping the data in more than one place.OK, it seems I completely missed that comment.
(Or forgot about it if I happened to answer it.)Based on that, I've created a different version of this patch,
attached. This way we keep all the data in one struct.I like this single structure but not the way you handle the
options' classes. In your patch, each option belongs to only
one class. These classes are:PG_CONNINFO_REPLICATION = "replication" only
PG_CONNINFO_PASSWORD = "password" only
PG_CONNINFO_NORMAL = everything elseHow does it help pg_basebackup to filter out e.g. dbname and
replication?PG_CONNINFO_ALL should give pg_basebackup all it needs, no? Or
actually, it shouldn't have the replication=1 part, right? So it
should just use PG_CONNINFO_NORMAL|PG_CONNINFO_PASSWORD?These are added by the walreceiver module anyway and adding them
to the primary_conninfo line should even be discouraged by the
documentation.Hmm. I wasn't actually thinking about the dbname part here, I admit that.
And not only the dbname, libpqwalreceiver adds these three:
[zozo@localhost libpqwalreceiver]$ grep dbname *
libpqwalreceiver.c: "%s dbname=replication replication=true
fallback_application_name=walreceiver",I also excluded "application_name" from PG_CONNINFO_REPLICATION
by this reasoning:- for async replication or single standby, it doesn't matter,
the connection will show up as "walreceiver"
- for sync replication, the administrator has to add the node name
manually via application_name.In my view, the classes should be inclusive:
PG_CONNINFO_NORMAL = Everything that's usable for a regular client
connection. This mean everything, maybe including "password" but
excluding "replication".PG_CONNINFO_PASSWORD = "password" only.
PG_CONNINFO_REPLICATION = Everything usable for a replication client
not added by walreceiver. Maybe including/excluding "password".Maybe there should be two flags for replication usage:
PG_CONNINFO_WALRECEIVER = everything except those not added
by walreceiver (and maybe "password" too)PG_CONNINFO_REPLICATION = "replication" only
And every option can belong to more than one class, just as in my patch.
Hmm. I kind of liked having each option in just one class, but I see
the problem. Looking at the ones you suggest, all the non-password
ones *have* to be without password, otherwise there i no way to get
the conninfo without password - which is the whole reason for that
parameter to exist. So the PASSWORD one has to be additional - which
means that not making the other ones additional makes them
inconsistent. But maybe we don't really have a choice there.Yes, the PASSWORD part can be on its own, this is what I meant above
but wanted a different opinion about having it completely separate
is better or not.But the NORMAL and REPLICATION (or WALRECEIVER) classes
need to overlap.At this point, the patch is untested beyond compiling and a trivial
psql check, because I ran out of time :) But I figured I'd throw it
out there for comments on which version people prefer. (And yes, it's
quite possible I've made a big think-mistake in it somewhere, but
again, better to get some eyes on it early)My version also contains a fixed version of the docs that should be
moved back into Zoltans version if that's the one we end up
preferring.I also liked your version of the documentation better,
I am not too good at writing docs.np.
Also, a question was buried in the other review which is - are we OK
to remove the requiressl parameter. Both these patches do so, because
the code becomes much simpler if we can do that. It has been
deprecated since 7.2. Is it OK to remove it, or do we need to put back
in the more complex code to deal with both?Just going to highlight that we're looking for at least one third
party to comment on this :)Yes, me too. A +1 for removing wouldn't count from me. ;-)
Attached is both Zoltans latest patch (v16) and my slightly different
approach.Comments on which approach is best?
Test results from somebody who has the time to look at it? :)
Do you happen to have a set of tests you've been running on your
patches? Can you try them again this one?My set of tests are:
1. initdb the master
2. pg_basebackup -R the first standby from the master
3. pg_basebackup -R the second standby from the master
4. pg_basebackup -R the third standby from the first standbyand diff -durpN the different data directories while there is no load on
either.I will test it today after fixing the classes in your patch. ;-)
Attached is my v17 patch based on your version with the classes fixed.
I introduced 2 new classes: PG_CONNINFO_REPLICATION_HIDDEN
which contains "dbname" and "fallback_application_name, and
PG_CONNINFO_REPLICATION_USER which contains everything else
except "replication".Since pg_basebackup sets fallback_application_name and not
application_name, this latter can be part of PG_CONNINFO_REPLICATION_USER.
I'm still not sure I like this. The "since pg_basebackup sets" part
really points to the problem - we're designing a public API for just
internal options here, aren't we? Is there *any* other application
that would ever use this? And if we do it like this, changing how
pg_basebackup works will make a change to our public libpq APIs in
worst case.
fallback_application_name certainly has *nothing* to do with
replication, in principle.
I'm thinking it might be a better idea to just have PG_CONNINFO_NORMAL
and PG_CONNINFO_PASSWORD (which still make sense to separate), and
then plug in the exclusion of specific parameters in pg_basebackup, in
the CreateRecoveryConf() function. pg_basebackup will of course always
know what pg_basebackup is doing with these things.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
2012-11-22 12:44 keltezéssel, Magnus Hagander írta:
On Thu, Nov 22, 2012 at 10:16 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
2012-11-22 08:35 keltezéssel, Boszormenyi Zoltan írta:
2012-11-21 22:15 keltezéssel, Magnus Hagander írta:
On Wed, Nov 21, 2012 at 10:01 PM, Boszormenyi Zoltan <zb@cybertec.at>
wrote:Hi,
2012-11-21 19:19 keltezéssel, Magnus Hagander írta:
I'm breaking this out into it's own thread, for my own sanity if
nothing else :) And it's an isolated feature after all.I still agree with the previous review at
http://archives.postgresql.org/message-id/1349321071.23971.0.camel@vanquo.pezone.net
about keeping the data in more than one place.OK, it seems I completely missed that comment.
(Or forgot about it if I happened to answer it.)Based on that, I've created a different version of this patch,
attached. This way we keep all the data in one struct.I like this single structure but not the way you handle the
options' classes. In your patch, each option belongs to only
one class. These classes are:PG_CONNINFO_REPLICATION = "replication" only
PG_CONNINFO_PASSWORD = "password" only
PG_CONNINFO_NORMAL = everything elseHow does it help pg_basebackup to filter out e.g. dbname and
replication?PG_CONNINFO_ALL should give pg_basebackup all it needs, no? Or
actually, it shouldn't have the replication=1 part, right? So it
should just use PG_CONNINFO_NORMAL|PG_CONNINFO_PASSWORD?These are added by the walreceiver module anyway and adding them
to the primary_conninfo line should even be discouraged by the
documentation.Hmm. I wasn't actually thinking about the dbname part here, I admit that.
And not only the dbname, libpqwalreceiver adds these three:
[zozo@localhost libpqwalreceiver]$ grep dbname *
libpqwalreceiver.c: "%s dbname=replication replication=true
fallback_application_name=walreceiver",I also excluded "application_name" from PG_CONNINFO_REPLICATION
by this reasoning:- for async replication or single standby, it doesn't matter,
the connection will show up as "walreceiver"
- for sync replication, the administrator has to add the node name
manually via application_name.In my view, the classes should be inclusive:
PG_CONNINFO_NORMAL = Everything that's usable for a regular client
connection. This mean everything, maybe including "password" but
excluding "replication".PG_CONNINFO_PASSWORD = "password" only.
PG_CONNINFO_REPLICATION = Everything usable for a replication client
not added by walreceiver. Maybe including/excluding "password".Maybe there should be two flags for replication usage:
PG_CONNINFO_WALRECEIVER = everything except those not added
by walreceiver (and maybe "password" too)PG_CONNINFO_REPLICATION = "replication" only
And every option can belong to more than one class, just as in my patch.
Hmm. I kind of liked having each option in just one class, but I see
the problem. Looking at the ones you suggest, all the non-password
ones *have* to be without password, otherwise there i no way to get
the conninfo without password - which is the whole reason for that
parameter to exist. So the PASSWORD one has to be additional - which
means that not making the other ones additional makes them
inconsistent. But maybe we don't really have a choice there.Yes, the PASSWORD part can be on its own, this is what I meant above
but wanted a different opinion about having it completely separate
is better or not.But the NORMAL and REPLICATION (or WALRECEIVER) classes
need to overlap.At this point, the patch is untested beyond compiling and a trivial
psql check, because I ran out of time :) But I figured I'd throw it
out there for comments on which version people prefer. (And yes, it's
quite possible I've made a big think-mistake in it somewhere, but
again, better to get some eyes on it early)My version also contains a fixed version of the docs that should be
moved back into Zoltans version if that's the one we end up
preferring.I also liked your version of the documentation better,
I am not too good at writing docs.np.
Also, a question was buried in the other review which is - are we OK
to remove the requiressl parameter. Both these patches do so, because
the code becomes much simpler if we can do that. It has been
deprecated since 7.2. Is it OK to remove it, or do we need to put back
in the more complex code to deal with both?Just going to highlight that we're looking for at least one third
party to comment on this :)Yes, me too. A +1 for removing wouldn't count from me. ;-)
Attached is both Zoltans latest patch (v16) and my slightly different
approach.Comments on which approach is best?
Test results from somebody who has the time to look at it? :)
Do you happen to have a set of tests you've been running on your
patches? Can you try them again this one?My set of tests are:
1. initdb the master
2. pg_basebackup -R the first standby from the master
3. pg_basebackup -R the second standby from the master
4. pg_basebackup -R the third standby from the first standbyand diff -durpN the different data directories while there is no load on
either.I will test it today after fixing the classes in your patch. ;-)
Attached is my v17 patch based on your version with the classes fixed.
I introduced 2 new classes: PG_CONNINFO_REPLICATION_HIDDEN
which contains "dbname" and "fallback_application_name, and
PG_CONNINFO_REPLICATION_USER which contains everything else
except "replication".Since pg_basebackup sets fallback_application_name and not
application_name, this latter can be part of PG_CONNINFO_REPLICATION_USER.I'm still not sure I like this. The "since pg_basebackup sets" part
really points to the problem - we're designing a public API for just
internal options here, aren't we? Is there *any* other application
that would ever use this?
I was thinking about this pg_receivexlog application but PQconninfo
is not needed there.
It would be a nice libpq extension to be able to login using a
PQconninfoOption array instead of the keyword/value arrays.
But the replication/normal distinction is not needed there either.
And if we do it like this, changing how
pg_basebackup works will make a change to our public libpq APIs in
worst case.fallback_application_name certainly has *nothing* to do with
replication, in principle.I'm thinking it might be a better idea to just have PG_CONNINFO_NORMAL
and PG_CONNINFO_PASSWORD (which still make sense to separate), and
then plug in the exclusion of specific parameters in pg_basebackup, in
the CreateRecoveryConf() function. pg_basebackup will of course always
know what pg_basebackup is doing with these things.
OK, I will post a new pg_basebackup patch with this in mind
in its own thread.
Anyway, here are two small patches, the first is against your
previous patch to fix a typo and and the ignoreMissing argument
to conninfo_storeval().
The second one is the product of what caught my attention while
I was looking at pg_receivexlog. The current coding may write
beyond the end of the allocated arrays and the password may
overwrite a previously set keyword/value pair.
Best regards,
Zoltán Böszörményi
--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/
Attachments:
PQconninfo-mha-fixes.patchtext/x-patch; name=PQconninfo-mha-fixes.patchDownload
diff -durpN postgresql.1/src/interfaces/libpq/fe-connect.c postgresql.1.1/src/interfaces/libpq/fe-connect.c
--- postgresql.1/src/interfaces/libpq/fe-connect.c 2012-11-22 13:42:42.820738413 +0100
+++ postgresql.1.1/src/interfaces/libpq/fe-connect.c 2012-11-22 13:49:00.828604651 +0100
@@ -4005,7 +4005,7 @@ conninfo_init(PQExpBuffer errorMessage,
const internalPQconninfoOption *cur_opt;
/*
- * Get enough memory for all optoins in PQconninfoOptions, even if some
+ * Get enough memory for all options in PQconninfoOptions, even if some
* end up being filtered out.
*/
options = (PQconninfoOption *) malloc(sizeof(PQconninfoOption) * sizeof(PQconninfoOptions)/sizeof(PQconninfoOptions[0]));
@@ -5107,7 +5107,7 @@ PQconninfo(PGconn *conn, unsigned int fl
if (*connmember)
conninfo_storeval(connOptions, option->keyword, *connmember,
- &errorBuf, false, false);
+ &errorBuf, true, false);
}
}
pg_basebackup-streamutil-fix.patchtext/x-patch; name=pg_basebackup-streamutil-fix.patchDownload
--- postgresql.1.1/src/bin/pg_basebackup/streamutil.c.old 2012-11-22 13:57:09.954235023 +0100
+++ postgresql.1.1/src/bin/pg_basebackup/streamutil.c 2012-11-22 14:02:19.712243537 +0100
@@ -77,8 +77,8 @@
GetConnection(void)
{
PGconn *tmpconn;
- int argcount = 4; /* dbname, replication, fallback_app_name,
- * password */
+ int argcount = 7; /* dbname, replication, fallback_app_name,
+ * host, user, port, password */
int i;
const char **keywords;
const char **values;
@@ -133,15 +133,15 @@
* meaning this is the call for a second session to the same
* database, so just forcibly reuse that password.
*/
- keywords[argcount - 1] = "password";
- values[argcount - 1] = dbpassword;
+ keywords[i] = "password";
+ values[i] = dbpassword;
dbgetpassword = -1; /* Don't try again if this fails */
}
else if (dbgetpassword == 1)
{
password = simple_prompt(_("Password: "), 100, false);
- keywords[argcount - 1] = "password";
- values[argcount - 1] = password;
+ keywords[i] = "password";
+ values[i] = password;
}
tmpconn = PQconnectdbParams(keywords, values, true);
When I was testing pg_resetxlog with relative database path,
The pg_resetxlog is doing the transaction log reset even when the server is
running.
Steps to reproduce:
1. ./pg_ctl -D ../../data start
2. ./pg_resetxlog -f ../../data -- is not able to detect as server
is already running.
Please find the patch for the same:
*** a/src/bin/pg_resetxlog/pg_resetxlog.c
--- b/src/bin/pg_resetxlog/pg_resetxlog.c
***************
*** 254,260 **** main(int argc, char *argv[])
*/
snprintf(path, MAXPGPATH, "%s/postmaster.pid", DataDir);
! if ((fd = open(path, O_RDONLY, 0)) < 0)
{
if (errno != ENOENT)
{
--- 254,260 ----
*/
snprintf(path, MAXPGPATH, "%s/postmaster.pid", DataDir);
! if ((fd = open("postmaster.pid", O_RDONLY, 0)) < 0)
{
if (errno != ENOENT)
{
Any suggestions/comments?
Regards,
Hari babu.
On Thu, Nov 22, 2012 at 10:22 PM, Hari Babu <haribabu.kommi@huawei.com> wrote:
When I was testing pg_resetxlog with relative database path,
The pg_resetxlog is doing the transaction log reset even when the server is
running.Steps to reproduce:
1. ./pg_ctl -D ../../data start
2. ./pg_resetxlog -f ../../data -- is not able to detect as server
is already running.Please find the patch for the same:
*** a/src/bin/pg_resetxlog/pg_resetxlog.c --- b/src/bin/pg_resetxlog/pg_resetxlog.c *************** *** 254,260 **** main(int argc, char *argv[]) */ snprintf(path, MAXPGPATH, "%s/postmaster.pid", DataDir);! if ((fd = open(path, O_RDONLY, 0)) < 0) { if (errno != ENOENT) { --- 254,260 ---- */ snprintf(path, MAXPGPATH, "%s/postmaster.pid", DataDir);! if ((fd = open("postmaster.pid", O_RDONLY, 0)) < 0)
{
if (errno != ENOENT)
{Any suggestions/comments?
Looks good to me.
Regards,
--
Fujii Masao
Import Notes
Reply to msg id not found: 50ae2761.828bd80a.6252.ffffc573SMTPIN_ADDED_BROKEN@mx.google.com
Hari Babu <haribabu.kommi@huawei.com> writes:
When I was testing pg_resetxlog with relative database path,
The pg_resetxlog is doing the transaction log reset even when the server is
running.
Ooops. Fixed, thanks for the report!
regards, tom lane
On Thu, Nov 22, 2012 at 10:05 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
2012-11-22 12:44 keltezéssel, Magnus Hagander írta:
Also, a question was buried in the other review which is - are we OK
to remove the requiressl parameter. Both these patches do so, because
the code becomes much simpler if we can do that. It has been
deprecated since 7.2. Is it OK to remove it, or do we need to put
back
in the more complex code to deal with both?Just going to highlight that we're looking for at least one third
party to comment on this :)Yes, me too. A +1 for removing wouldn't count from me. ;-)
+1
The second one is the product of what caught my attention while
I was looking at pg_receivexlog. The current coding may write
beyond the end of the allocated arrays and the password may
overwrite a previously set keyword/value pair.
ISTM that such problem doesn't happen at all because argcount is
incremented as follows.
if (dbhost)
argcount++;
if (dbuser)
argcount++;
if (dbport)
argcount++;
Regards,
--
Fujii Masao
2012-11-23 06:30 keltezéssel, Fujii Masao írta:
On Thu, Nov 22, 2012 at 10:05 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
2012-11-22 12:44 keltezéssel, Magnus Hagander írta:
Also, a question was buried in the other review which is - are we OK
to remove the requiressl parameter. Both these patches do so, because
the code becomes much simpler if we can do that. It has been
deprecated since 7.2. Is it OK to remove it, or do we need to put
back
in the more complex code to deal with both?Just going to highlight that we're looking for at least one third
party to comment on this :)Yes, me too. A +1 for removing wouldn't count from me. ;-)
+1
Thanks. :-)
The second one is the product of what caught my attention while
I was looking at pg_receivexlog. The current coding may write
beyond the end of the allocated arrays and the password may
overwrite a previously set keyword/value pair.ISTM that such problem doesn't happen at all because argcount is
incremented as follows.if (dbhost)
argcount++;
if (dbuser)
argcount++;
if (dbport)
argcount++;
Right, forget about the second patch.
Regards,
--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/
On Fri, Nov 23, 2012 at 6:30 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Thu, Nov 22, 2012 at 10:05 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
2012-11-22 12:44 keltezéssel, Magnus Hagander írta:
Also, a question was buried in the other review which is - are we OK
to remove the requiressl parameter. Both these patches do so, because
the code becomes much simpler if we can do that. It has been
deprecated since 7.2. Is it OK to remove it, or do we need to put
back
in the more complex code to deal with both?Just going to highlight that we're looking for at least one third
party to comment on this :)Yes, me too. A +1 for removing wouldn't count from me. ;-)
+1
Actually, with the cleaner code that resulted from the rewrite,
reintroducing it turned out to be pretty darn simple - if I'm not
missing something. PFA a patch that comes *with* requiressl=<n>
support, without making the code that much more complex.
It also fixes the fact that pg_service.conf was broken by the previous one.
It also includes the small fixes from Zoltans latest round (the one
that was for libpq, not the one for pg_receivexlog that turned out to
be wrong).
And a pgindent run :)
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Attachments:
PQconninfo-mha-2.patchapplication/octet-stream; name=PQconninfo-mha-2.patchDownload
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 255c5c1..db46cf1 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -496,6 +496,79 @@ typedef struct
</listitem>
</varlistentry>
+ <varlistentry id="libpq-pqconninfo">
+ <term><function>PQconninfo</function><indexterm><primary>PQconninfo</></></term>
+ <listitem>
+ <para>
+ Returns the connection options used by a live connection.
+<synopsis>
+PQconninfoOption *PQconninfo(PGconn *conn, int flags);
+</synopsis>
+ </para>
+
+ <para>
+ Returns a connection options array. This can be used to determine
+ all possible <function>PQconnectdb</function> options and the
+ values that were used to connect to the server. The return
+ value points to an array of <structname>PQconninfoOption</structname>
+ structures, which ends with an entry having a null <structfield>keyword</>
+ pointer. All notes above for <function>PQconndefaults</function> also
+ apply to the result of <function>PQconninfo</function>.
+ </para>
+
+ <para>
+ The parameter <literal>flags</literal> is used to filter which
+ connection options are returned. The flags are a bitmask that can be
+ ORed together to return parameters of multiple types at the same time.
+
+ <variablelist>
+ <varlistentry>
+ <term><symbol>PG_CONNINFO_NORMAL</symbol></term>
+ <listitem>
+ <para>
+ All normal connection options.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><symbol>PG_CONNINFO_PASSWORD</symbol></term>
+ <listitem>
+ <para>
+ All connection options that include passwords or other
+ sensitive information.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><symbol>PG_CONNINFO_REPLICATION</symbol></term>
+ <listitem>
+ <para>
+ All connection options only used for replication
+ connections.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><symbol>PG_CONNINFO_INTERNAL</symbol></term>
+ <listitem>
+ <para>
+ Internal or backwards compatible connection options.
+ Should not be used by regular applications.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
+
+ It is also possible to specify <literal>PG_CONNINFO_ALL</literal>,
+ which will return all options, regardless of type.
+ </para>
+ </listitem>
+ </varlistentry>
+
+
<varlistentry id="libpq-pqconninfoparse">
<term><function>PQconninfoParse</function><indexterm><primary>PQconninfoParse</></></term>
<listitem>
diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt
index 56d0bb8..93da50d 100644
--- a/src/interfaces/libpq/exports.txt
+++ b/src/interfaces/libpq/exports.txt
@@ -164,3 +164,4 @@ PQsetSingleRowMode 161
lo_lseek64 162
lo_tell64 163
lo_truncate64 164
+PQconninfo 165
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 9eaf410..7f498bb 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -137,81 +137,113 @@ static int ldapServiceLookup(const char *purl, PQconninfoOption *options,
* PQconninfoOptions[] *must* be NULL. In a working copy, non-null "val"
* fields point to malloc'd strings that should be freed when the working
* array is freed (see PQconninfoFree).
+ *
+ * The first part of each struct is identical to the one in libpq-fe.h,
+ * which is required since we memcpy() data between the two!
* ----------
*/
-static const PQconninfoOption PQconninfoOptions[] = {
+typedef struct _internalPQconninfoOption
+{
+ char *keyword; /* The keyword of the option */
+ char *envvar; /* Fallback environment variable name */
+ char *compiled; /* Fallback compiled in default value */
+ char *val; /* Option's current value, or NULL */
+ char *label; /* Label for field in connect dialog */
+ char *dispchar; /* Indicates how to display this field in a
+ * connect dialog. Values are: "" Display
+ * entered value as is "*" Password field -
+ * hide value "D" Debug option - don't show
+ * by default */
+ int dispsize; /* Field size in characters for dialog */
+ /* ---
+ * Anything above this comment must be synchronized with
+ * PQconninfoOption in libpq-fe.h, since we memcpy() data
+ * between them!
+ * ---
+ */
+ off_t connofs; /* Offset into PGconn struct, -1 if not there */
+ unsigned int flags; /* type of option */
+} internalPQconninfoOption;
+
+static const internalPQconninfoOption PQconninfoOptions[] = {
/*
* "authtype" is no longer used, so mark it "don't show". We keep it in
* the array so as not to reject conninfo strings from old apps that might
* still try to set it.
*/
{"authtype", "PGAUTHTYPE", DefaultAuthtype, NULL,
- "Database-Authtype", "D", 20},
+ "Database-Authtype", "D", 20, -1, PG_CONNINFO_INTERNAL},
{"service", "PGSERVICE", NULL, NULL,
- "Database-Service", "", 20},
+ "Database-Service", "", 20, -1, PG_CONNINFO_NORMAL},
{"user", "PGUSER", NULL, NULL,
- "Database-User", "", 20},
+ "Database-User", "", 20,
+ offsetof(struct pg_conn, pguser), PG_CONNINFO_NORMAL},
{"password", "PGPASSWORD", NULL, NULL,
- "Database-Password", "*", 20},
+ "Database-Password", "*", 20,
+ offsetof(struct pg_conn, pgpass), PG_CONNINFO_PASSWORD},
{"connect_timeout", "PGCONNECT_TIMEOUT", NULL, NULL,
- "Connect-timeout", "", 10}, /* strlen(INT32_MAX) == 10 */
+ "Connect-timeout", "", 10, /* strlen(INT32_MAX) == 10 */
+ offsetof(struct pg_conn, connect_timeout), PG_CONNINFO_NORMAL},
{"dbname", "PGDATABASE", NULL, NULL,
- "Database-Name", "", 20},
+ "Database-Name", "", 20,
+ offsetof(struct pg_conn, dbName), PG_CONNINFO_NORMAL},
{"host", "PGHOST", NULL, NULL,
- "Database-Host", "", 40},
+ "Database-Host", "", 40,
+ offsetof(struct pg_conn, pghost), PG_CONNINFO_NORMAL},
{"hostaddr", "PGHOSTADDR", NULL, NULL,
- "Database-Host-IP-Address", "", 45},
+ "Database-Host-IP-Address", "", 45,
+ offsetof(struct pg_conn, pghostaddr), PG_CONNINFO_NORMAL},
{"port", "PGPORT", DEF_PGPORT_STR, NULL,
- "Database-Port", "", 6},
+ "Database-Port", "", 6,
+ offsetof(struct pg_conn, pgport), PG_CONNINFO_NORMAL},
{"client_encoding", "PGCLIENTENCODING", NULL, NULL,
- "Client-Encoding", "", 10},
+ "Client-Encoding", "", 10,
+ offsetof(struct pg_conn, client_encoding_initial), PG_CONNINFO_NORMAL},
/*
* "tty" is no longer used either, but keep it present for backwards
* compatibility.
*/
{"tty", "PGTTY", DefaultTty, NULL,
- "Backend-Debug-TTY", "D", 40},
+ "Backend-Debug-TTY", "D", 40,
+ offsetof(struct pg_conn, pgtty), PG_CONNINFO_NORMAL},
{"options", "PGOPTIONS", DefaultOption, NULL,
- "Backend-Debug-Options", "D", 40},
+ "Backend-Debug-Options", "D", 40,
+ offsetof(struct pg_conn, pgoptions), PG_CONNINFO_NORMAL},
{"application_name", "PGAPPNAME", NULL, NULL,
- "Application-Name", "", 64},
+ "Application-Name", "", 64,
+ offsetof(struct pg_conn, appname), PG_CONNINFO_NORMAL},
{"fallback_application_name", NULL, NULL, NULL,
- "Fallback-Application-Name", "", 64},
+ "Fallback-Application-Name", "", 64,
+ offsetof(struct pg_conn, fbappname), PG_CONNINFO_NORMAL},
{"keepalives", NULL, NULL, NULL,
- "TCP-Keepalives", "", 1}, /* should be just '0' or '1' */
+ "TCP-Keepalives", "", 1, /* should be just '0' or '1' */
+ offsetof(struct pg_conn, keepalives), PG_CONNINFO_NORMAL},
{"keepalives_idle", NULL, NULL, NULL,
- "TCP-Keepalives-Idle", "", 10}, /* strlen(INT32_MAX) == 10 */
+ "TCP-Keepalives-Idle", "", 10, /* strlen(INT32_MAX) == 10 */
+ offsetof(struct pg_conn, keepalives_idle), PG_CONNINFO_NORMAL},
{"keepalives_interval", NULL, NULL, NULL,
- "TCP-Keepalives-Interval", "", 10}, /* strlen(INT32_MAX) == 10 */
+ "TCP-Keepalives-Interval", "", 10, /* strlen(INT32_MAX) == 10 */
+ offsetof(struct pg_conn, keepalives_interval), PG_CONNINFO_NORMAL},
{"keepalives_count", NULL, NULL, NULL,
- "TCP-Keepalives-Count", "", 10}, /* strlen(INT32_MAX) == 10 */
-
-#ifdef USE_SSL
-
- /*
- * "requiressl" is deprecated, its purpose having been taken over by
- * "sslmode". It remains for backwards compatibility.
- */
- {"requiressl", "PGREQUIRESSL", "0", NULL,
- "Require-SSL", "D", 1},
-#endif
+ "TCP-Keepalives-Count", "", 10, /* strlen(INT32_MAX) == 10 */
+ offsetof(struct pg_conn, keepalives_count), PG_CONNINFO_NORMAL},
/*
* ssl options are allowed even without client SSL support because the
@@ -220,30 +252,38 @@ static const PQconninfoOption PQconninfoOptions[] = {
* to exclude them since none of them are mandatory.
*/
{"sslmode", "PGSSLMODE", DefaultSSLMode, NULL,
- "SSL-Mode", "", 8}, /* sizeof("disable") == 8 */
+ "SSL-Mode", "", 8, /* sizeof("disable") == 8 */
+ offsetof(struct pg_conn, sslmode), PG_CONNINFO_NORMAL},
{"sslcompression", "PGSSLCOMPRESSION", "1", NULL,
- "SSL-Compression", "", 1},
+ "SSL-Compression", "", 1,
+ offsetof(struct pg_conn, sslcompression), PG_CONNINFO_NORMAL},
{"sslcert", "PGSSLCERT", NULL, NULL,
- "SSL-Client-Cert", "", 64},
+ "SSL-Client-Cert", "", 64,
+ offsetof(struct pg_conn, sslcert), PG_CONNINFO_NORMAL},
{"sslkey", "PGSSLKEY", NULL, NULL,
- "SSL-Client-Key", "", 64},
+ "SSL-Client-Key", "", 64,
+ offsetof(struct pg_conn, sslkey), PG_CONNINFO_NORMAL},
{"sslrootcert", "PGSSLROOTCERT", NULL, NULL,
- "SSL-Root-Certificate", "", 64},
+ "SSL-Root-Certificate", "", 64,
+ offsetof(struct pg_conn, sslrootcert), PG_CONNINFO_NORMAL},
{"sslcrl", "PGSSLCRL", NULL, NULL,
- "SSL-Revocation-List", "", 64},
+ "SSL-Revocation-List", "", 64,
+ offsetof(struct pg_conn, sslcrl), PG_CONNINFO_NORMAL},
{"requirepeer", "PGREQUIREPEER", NULL, NULL,
- "Require-Peer", "", 10},
+ "Require-Peer", "", 10,
+ offsetof(struct pg_conn, requirepeer), PG_CONNINFO_NORMAL},
#if defined(KRB5) || defined(ENABLE_GSS) || defined(ENABLE_SSPI)
/* Kerberos and GSSAPI authentication support specifying the service name */
{"krbsrvname", "PGKRBSRVNAME", PG_KRB_SRVNAM, NULL,
- "Kerberos-service-name", "", 20},
+ "Kerberos-service-name", "", 20,
+ offsetof(struct pg_conn, krbsrvname), PG_CONNINFO_NORMAL},
#endif
#if defined(ENABLE_GSS) && defined(ENABLE_SSPI)
@@ -253,11 +293,13 @@ static const PQconninfoOption PQconninfoOptions[] = {
* default
*/
{"gsslib", "PGGSSLIB", NULL, NULL,
- "GSS-library", "", 7}, /* sizeof("gssapi") = 7 */
+ "GSS-library", "", 7, /* sizeof("gssapi") = 7 */
+ offsetof(struct pg_conn, gsslib), PG_CONNINFO_NORMAL},
#endif
{"replication", NULL, NULL, NULL,
- "Replication", "D", 5},
+ "Replication", "D", 5,
+ offsetof(struct pg_conn, replication), PG_CONNINFO_REPLICATION},
/* Terminating entry --- MUST BE LAST */
{NULL, NULL, NULL, NULL,
@@ -295,7 +337,8 @@ static PGconn *makeEmptyPGconn(void);
static void fillPGconn(PGconn *conn, PQconninfoOption *connOptions);
static void freePGconn(PGconn *conn);
static void closePGconn(PGconn *conn);
-static PQconninfoOption *conninfo_init(PQExpBuffer errorMessage);
+static PQconninfoOption *conninfo_init(PQExpBuffer errorMessage,
+ int flags);
static PQconninfoOption *parse_connection_string(const char *conninfo,
PQExpBuffer errorMessage, bool use_defaults);
static int uri_prefix_length(const char *connstr);
@@ -627,7 +670,7 @@ PQconnectStart(const char *conninfo)
static void
fillPGconn(PGconn *conn, PQconninfoOption *connOptions)
{
- const char *tmp;
+ const internalPQconninfoOption *option;
/*
* Move option values into conn structure
@@ -637,72 +680,19 @@ fillPGconn(PGconn *conn, PQconninfoOption *connOptions)
*
* XXX: probably worth checking strdup() return value here...
*/
- tmp = conninfo_getval(connOptions, "hostaddr");
- conn->pghostaddr = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "host");
- conn->pghost = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "port");
- conn->pgport = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "tty");
- conn->pgtty = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "options");
- conn->pgoptions = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "application_name");
- conn->appname = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "fallback_application_name");
- conn->fbappname = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "dbname");
- conn->dbName = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "user");
- conn->pguser = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "password");
- conn->pgpass = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "connect_timeout");
- conn->connect_timeout = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "client_encoding");
- conn->client_encoding_initial = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "keepalives");
- conn->keepalives = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "keepalives_idle");
- conn->keepalives_idle = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "keepalives_interval");
- conn->keepalives_interval = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "keepalives_count");
- conn->keepalives_count = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "sslmode");
- conn->sslmode = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "sslcompression");
- conn->sslcompression = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "sslkey");
- conn->sslkey = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "sslcert");
- conn->sslcert = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "sslrootcert");
- conn->sslrootcert = tmp ? strdup(tmp) : NULL;
- tmp = conninfo_getval(connOptions, "sslcrl");
- conn->sslcrl = tmp ? strdup(tmp) : NULL;
-#ifdef USE_SSL
- tmp = conninfo_getval(connOptions, "requiressl");
- if (tmp && tmp[0] == '1')
+ for (option = PQconninfoOptions; option->keyword; option++)
{
- /* here warn that the requiressl option is deprecated? */
- if (conn->sslmode)
- free(conn->sslmode);
- conn->sslmode = strdup("require");
+ const char *tmp = conninfo_getval(connOptions, option->keyword);
+
+ if (tmp && option->connofs >= 0)
+ {
+ char **connmember = (char **) ((char *) conn + option->connofs);
+
+ if (*connmember)
+ free(*connmember);
+ *connmember = tmp ? strdup(tmp) : NULL;
+ }
}
-#endif
- tmp = conninfo_getval(connOptions, "requirepeer");
- conn->requirepeer = tmp ? strdup(tmp) : NULL;
-#if defined(KRB5) || defined(ENABLE_GSS) || defined(ENABLE_SSPI)
- tmp = conninfo_getval(connOptions, "krbsrvname");
- conn->krbsrvname = tmp ? strdup(tmp) : NULL;
-#endif
-#if defined(ENABLE_GSS) && defined(ENABLE_SSPI)
- tmp = conninfo_getval(connOptions, "gsslib");
- conn->gsslib = tmp ? strdup(tmp) : NULL;
-#endif
- tmp = conninfo_getval(connOptions, "replication");
- conn->replication = tmp ? strdup(tmp) : NULL;
}
/*
@@ -884,7 +874,7 @@ PQconndefaults(void)
if (PQExpBufferDataBroken(errorBuf))
return NULL; /* out of memory already :-( */
- connOptions = conninfo_init(&errorBuf);
+ connOptions = conninfo_init(&errorBuf, PG_CONNINFO_ALL);
if (connOptions != NULL)
{
if (!conninfo_add_defaults(connOptions, &errorBuf))
@@ -4008,18 +3998,35 @@ PQconninfoParse(const char *conninfo, char **errmsg)
* Build a working copy of the constant PQconninfoOptions array.
*/
static PQconninfoOption *
-conninfo_init(PQExpBuffer errorMessage)
+conninfo_init(PQExpBuffer errorMessage, int flags)
{
PQconninfoOption *options;
+ PQconninfoOption *opt_dest;
+ const internalPQconninfoOption *cur_opt;
- options = (PQconninfoOption *) malloc(sizeof(PQconninfoOptions));
+ /*
+ * Get enough memory for all options in PQconninfoOptions, even if some
+ * end up being filtered out.
+ */
+ options = (PQconninfoOption *) malloc(sizeof(PQconninfoOption) * sizeof(PQconninfoOptions) / sizeof(PQconninfoOptions[0]));
if (options == NULL)
{
printfPQExpBuffer(errorMessage,
libpq_gettext("out of memory\n"));
return NULL;
}
- memcpy(options, PQconninfoOptions, sizeof(PQconninfoOptions));
+ opt_dest = options;
+
+ for (cur_opt = PQconninfoOptions; cur_opt->keyword; cur_opt++)
+ {
+ if (!(cur_opt->flags & flags))
+ continue;
+
+ /* Only copy the public part of the struct, not the full internal */
+ memcpy(opt_dest, cur_opt, sizeof(PQconninfoOption));
+ opt_dest++;
+ }
+ MemSet(opt_dest, 0, sizeof(PQconninfoOption));
return options;
}
@@ -4095,7 +4102,7 @@ conninfo_parse(const char *conninfo, PQExpBuffer errorMessage,
PQconninfoOption *options;
/* Make a working copy of PQconninfoOptions */
- options = conninfo_init(errorMessage);
+ options = conninfo_init(errorMessage, PG_CONNINFO_ALL);
if (options == NULL)
return NULL;
@@ -4295,7 +4302,7 @@ conninfo_array_parse(const char *const * keywords, const char *const * values,
}
/* Make a working copy of PQconninfoOptions */
- options = conninfo_init(errorMessage);
+ options = conninfo_init(errorMessage, PG_CONNINFO_ALL);
if (options == NULL)
{
PQconninfoFree(dbname_options);
@@ -4485,7 +4492,7 @@ conninfo_uri_parse(const char *uri, PQExpBuffer errorMessage,
PQconninfoOption *options;
/* Make a working copy of PQconninfoOptions */
- options = conninfo_init(errorMessage);
+ options = conninfo_init(errorMessage, PG_CONNINFO_ALL);
if (options == NULL)
return NULL;
@@ -5008,6 +5015,20 @@ conninfo_storeval(PQconninfoOption *connOptions,
PQconninfoOption *option;
char *value_copy;
+ /*
+ * For backwards compatibility, requiressl=1 gets translated to
+ * sslmode=require, and requiressl=0 gets translated to sslmode=prefer
+ * (which is the default for sslmode).
+ */
+ if (strcmp(keyword, "requiressl") == 0)
+ {
+ keyword = "sslmode";
+ if (value[0] == '1')
+ value = "require";
+ else
+ value = "prefer";
+ }
+
option = conninfo_find(connOptions, keyword);
if (option == NULL)
{
@@ -5066,6 +5087,50 @@ conninfo_find(PQconninfoOption *connOptions, const char *keyword)
}
+/*
+ * Return the connection options used for the connections
+ */
+PQconninfoOption *
+PQconninfo(PGconn *conn, unsigned int flags)
+{
+ PQExpBufferData errorBuf;
+ PQconninfoOption *connOptions;
+
+ if (conn == NULL)
+ return NULL;
+
+ /* We don't actually report any errors here, but callees want a buffer */
+ initPQExpBuffer(&errorBuf);
+ if (PQExpBufferDataBroken(errorBuf))
+ return NULL; /* out of memory already :-( */
+
+ connOptions = conninfo_init(&errorBuf, flags);
+
+ if (connOptions != NULL)
+ {
+ const internalPQconninfoOption *option;
+
+ for (option = PQconninfoOptions; option->keyword; option++)
+ {
+ char **connmember;
+
+ if (option->connofs < 0)
+ continue;
+
+ connmember = (char **) ((char *) conn + option->connofs);
+
+ if (*connmember)
+ conninfo_storeval(connOptions, option->keyword, *connmember,
+ &errorBuf, true, false);
+ }
+ }
+
+ termPQExpBuffer(&errorBuf);
+
+ return connOptions;
+}
+
+
void
PQconninfoFree(PQconninfoOption *connOptions)
{
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index 0b8d9a6..911fd64 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -36,6 +36,15 @@ extern "C"
#define PG_COPYRES_EVENTS 0x04
#define PG_COPYRES_NOTICEHOOKS 0x08
+/*
+ * Option flags for PQconninfo
+ */
+#define PG_CONNINFO_NORMAL 0x01
+#define PG_CONNINFO_PASSWORD 0x02
+#define PG_CONNINFO_REPLICATION 0x04
+#define PG_CONNINFO_INTERNAL 0x80000000
+#define PG_CONNINFO_ALL 0xFFFFFFFF
+
/* Application-visible enum types */
/*
@@ -262,6 +271,9 @@ extern PQconninfoOption *PQconndefaults(void);
/* parse connection options in same way as PQconnectdb */
extern PQconninfoOption *PQconninfoParse(const char *conninfo, char **errmsg);
+/* return the connection options used by a live connection */
+extern PQconninfoOption *PQconninfo(PGconn *conn, unsigned int flags);
+
/* free the data structure returned by PQconndefaults() or PQconninfoParse() */
extern void PQconninfoFree(PQconninfoOption *connOptions);
On 11/22/12 6:44 AM, Magnus Hagander wrote:
I'm thinking it might be a better idea to just have PG_CONNINFO_NORMAL
and PG_CONNINFO_PASSWORD (which still make sense to separate),
What is the use case for separating them?
and
then plug in the exclusion of specific parameters in pg_basebackup, in
the CreateRecoveryConf() function. pg_basebackup will of course always
know what pg_basebackup is doing with these things.
Agreed on that in principle.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Nov 27, 2012 at 10:13 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
On 11/22/12 6:44 AM, Magnus Hagander wrote:
I'm thinking it might be a better idea to just have PG_CONNINFO_NORMAL
and PG_CONNINFO_PASSWORD (which still make sense to separate),What is the use case for separating them?
Getting a connection string that doesn't contain sensitive information
like a password. In order to show it in a dialog box, for example.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 11/27/12 4:20 PM, Magnus Hagander wrote:
On Tue, Nov 27, 2012 at 10:13 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
On 11/22/12 6:44 AM, Magnus Hagander wrote:
I'm thinking it might be a better idea to just have PG_CONNINFO_NORMAL
and PG_CONNINFO_PASSWORD (which still make sense to separate),What is the use case for separating them?
Getting a connection string that doesn't contain sensitive information
like a password. In order to show it in a dialog box, for example.
There is already the PQconninfoOption.dispchar field for this purpose.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Peter Eisentraut wrote:
On 11/27/12 4:20 PM, Magnus Hagander wrote:
On Tue, Nov 27, 2012 at 10:13 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
On 11/22/12 6:44 AM, Magnus Hagander wrote:
I'm thinking it might be a better idea to just have PG_CONNINFO_NORMAL
and PG_CONNINFO_PASSWORD (which still make sense to separate),What is the use case for separating them?
Getting a connection string that doesn't contain sensitive information
like a password. In order to show it in a dialog box, for example.There is already the PQconninfoOption.dispchar field for this purpose.
I had the impression that that field would go away with this patch, but
then again it might not be worth the compatibility break. I find the
dispchar thingy somewhat unsightly.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
Peter Eisentraut wrote:
There is already the PQconninfoOption.dispchar field for this purpose.
I had the impression that that field would go away with this patch, but
then again it might not be worth the compatibility break. I find the
dispchar thingy somewhat unsightly.
It is that, without a doubt, but that API has been that way longer than
any of us have been working on the code. I'm not excited about changing
it just to have an allegedly-cleaner API. And we cannot have the field
simply "go away", because it's been exposed to applications for lo these
many years, and surely some of them are using it. So in practice we'd
be maintaining both the old API and the new one.
I think we should leave it as-is until there are more reasons to change
it than seem to be provided in this thread.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Nov 28, 2012 1:54 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
Peter Eisentraut wrote:
There is already the PQconninfoOption.dispchar field for this purpose.
I had the impression that that field would go away with this patch, but
then again it might not be worth the compatibility break. I find the
dispchar thingy somewhat unsightly.It is that, without a doubt, but that API has been that way longer than
any of us have been working on the code. I'm not excited about changing
it just to have an allegedly-cleaner API. And we cannot have the field
simply "go away", because it's been exposed to applications for lo these
many years, and surely some of them are using it. So in practice we'd
be maintaining both the old API and the new one.I think we should leave it as-is until there are more reasons to change
it than seem to be provided in this thread.
I think removing that would be a really bad idea. I'm not sure anybody is
actually relying on it, but it would also change the size of the struct and
thus break things for anybody using those functions.
If people prefer we remove the password classifier for the new function
since it at least partially duplicates that field we can certainly do that,
but I think leaving it in allows those who write new code to use a slightly
neater api, at pretty much no cost in maintenance for us.
/Magnus
On Wed, Nov 28, 2012 at 7:01 AM, Magnus Hagander <magnus@hagander.net> wrote:
On Nov 28, 2012 1:54 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
Peter Eisentraut wrote:
There is already the PQconninfoOption.dispchar field for this purpose.
I had the impression that that field would go away with this patch, but
then again it might not be worth the compatibility break. I find the
dispchar thingy somewhat unsightly.It is that, without a doubt, but that API has been that way longer than
any of us have been working on the code. I'm not excited about changing
it just to have an allegedly-cleaner API. And we cannot have the field
simply "go away", because it's been exposed to applications for lo these
many years, and surely some of them are using it. So in practice we'd
be maintaining both the old API and the new one.I think we should leave it as-is until there are more reasons to change
it than seem to be provided in this thread.I think removing that would be a really bad idea. I'm not sure anybody is
actually relying on it, but it would also change the size of the struct and
thus break things for anybody using those functions.If people prefer we remove the password classifier for the new function
since it at least partially duplicates that field we can certainly do that,
but I think leaving it in allows those who write new code to use a slightly
neater api, at pretty much no cost in maintenance for us.
In the interest of moving things along, I'll remove these for now at
least, and commit the rest of the patch, so we can keep working on the
basebacku part of things.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Nov 30, 2012 at 1:02 AM, Magnus Hagander <magnus@hagander.net> wrote:
In the interest of moving things along, I'll remove these for now at
least, and commit the rest of the patch, so we can keep working on the
basebacku part of things.
I see you committed this - does this possibly provide infrastructure
that could be used to lift the restriction imposed by the following
commit?
commit fe21fcaf8d91a71c15ff25276f9fa81e0cd1dba9
Author: Bruce Momjian <bruce@momjian.us>
Date: Wed Aug 15 19:04:52 2012 -0400
In psql, if the is no connection object, e.g. due to a server crash,
require all parameters for \c, rather than using the defaults, which
might be wrong.
Because personally, I find the new behavior no end of annoying.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Dec 3, 2012 at 3:20 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Fri, Nov 30, 2012 at 1:02 AM, Magnus Hagander <magnus@hagander.net> wrote:
In the interest of moving things along, I'll remove these for now at
least, and commit the rest of the patch, so we can keep working on the
basebacku part of things.I see you committed this - does this possibly provide infrastructure
that could be used to lift the restriction imposed by the following
commit?commit fe21fcaf8d91a71c15ff25276f9fa81e0cd1dba9
Author: Bruce Momjian <bruce@momjian.us>
Date: Wed Aug 15 19:04:52 2012 -0400In psql, if the is no connection object, e.g. due to a server crash,
require all parameters for \c, rather than using the defaults, which
might be wrong.Because personally, I find the new behavior no end of annoying.
It certainly sounds like it could do that. Though it might be useful
to actually add a connection funciton to libpq that takes such an
array of options directly - AFAICS, right now you'd have to
deconstruct the return value into a string, and then pass it into a
connection function that immediately parses it right back out as
conninfooptions.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers