chomp PQerrorMessage() in backend uses
Started by Peter Eisentrautalmost 9 years ago3 messages
Here is a patch to systematically trim the trailing newlines off
PQerrorMessage() results in backend uses (dblink, postgres_fdw,
libpqwalreceiver).
I noticed that there are some inconsistent assumptions about whether
PQerrorMessage() can ever return NULL. From the code, I think that
should not be possible, but some code appears to be prepared for it.
Other code is not. What is correct?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-chomp-PQerrorMessage-in-backend-uses.patchtext/x-patch; name=0001-chomp-PQerrorMessage-in-backend-uses.patchDownload
From d6835cff36bcc237462febf5ba7d3df7d560329f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Wed, 8 Feb 2017 09:31:35 -0500
Subject: [PATCH] chomp PQerrorMessage() in backend uses
PQerrorMessage() returns an error message with a trailing newline, but
in backend use (dblink, postgres_fdw, libpqwalreceiver), we want to have
the error message without that for emitting via ereport(). To simplify
that, add a function pchomp() that returns a pstrdup'ed string with the
trailing newline characters removed.
---
contrib/dblink/dblink.c | 16 ++++++------
contrib/dblink/expected/dblink.out | 2 --
contrib/postgres_fdw/connection.c | 14 ++--------
.../libpqwalreceiver/libpqwalreceiver.c | 30 +++++++++++-----------
src/backend/utils/mmgr/mcxt.c | 14 ++++++++++
src/include/utils/palloc.h | 2 ++
6 files changed, 41 insertions(+), 37 deletions(-)
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index ac43c458cb..db0a8ba72d 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -166,7 +166,7 @@ typedef struct remoteConnHashEnt
#define DBLINK_RES_INTERNALERROR(p2) \
do { \
- msg = pstrdup(PQerrorMessage(conn)); \
+ msg = pchomp(PQerrorMessage(conn)); \
if (res) \
PQclear(res); \
elog(ERROR, "%s: %s", p2, msg); \
@@ -204,7 +204,7 @@ typedef struct remoteConnHashEnt
conn = PQconnectdb(connstr); \
if (PQstatus(conn) == CONNECTION_BAD) \
{ \
- msg = pstrdup(PQerrorMessage(conn)); \
+ msg = pchomp(PQerrorMessage(conn)); \
PQfinish(conn); \
ereport(ERROR, \
(errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION), \
@@ -278,7 +278,7 @@ dblink_connect(PG_FUNCTION_ARGS)
if (PQstatus(conn) == CONNECTION_BAD)
{
- msg = pstrdup(PQerrorMessage(conn));
+ msg = pchomp(PQerrorMessage(conn));
PQfinish(conn);
if (rconn)
pfree(rconn);
@@ -651,7 +651,7 @@ dblink_send_query(PG_FUNCTION_ARGS)
/* async query send */
retval = PQsendQuery(conn, sql);
if (retval != 1)
- elog(NOTICE, "could not send query: %s", PQerrorMessage(conn));
+ elog(NOTICE, "could not send query: %s", pchomp(PQerrorMessage(conn)));
PG_RETURN_INT32(retval);
}
@@ -1087,7 +1087,7 @@ storeQueryResult(volatile storeInfo *sinfo, PGconn *conn, const char *sql)
PGresult *res;
if (!PQsendQuery(conn, sql))
- elog(ERROR, "could not send query: %s", PQerrorMessage(conn));
+ elog(ERROR, "could not send query: %s", pchomp(PQerrorMessage(conn)));
if (!PQsetSingleRowMode(conn)) /* shouldn't fail */
elog(ERROR, "failed to set single-row mode for dblink query");
@@ -1366,8 +1366,8 @@ dblink_error_message(PG_FUNCTION_ARGS)
DBLINK_INIT;
DBLINK_GET_NAMED_CONN;
- msg = PQerrorMessage(conn);
- if (msg == NULL || msg[0] == '\0')
+ msg = pchomp(PQerrorMessage(conn));
+ if (msg[0] == '\0')
PG_RETURN_TEXT_P(cstring_to_text("OK"));
else
PG_RETURN_TEXT_P(cstring_to_text(msg));
@@ -2709,7 +2709,7 @@ dblink_res_error(PGconn *conn, const char *conname, PGresult *res,
* return NULL, not a PGresult at all.
*/
if (message_primary == NULL)
- message_primary = PQerrorMessage(conn);
+ message_primary = pchomp(PQerrorMessage(conn));
if (res)
PQclear(res);
diff --git a/contrib/dblink/expected/dblink.out b/contrib/dblink/expected/dblink.out
index 5acaaf225a..4b6d26e574 100644
--- a/contrib/dblink/expected/dblink.out
+++ b/contrib/dblink/expected/dblink.out
@@ -377,7 +377,6 @@ FROM dblink('myconn','SELECT * FROM foo') AS t(a int, b text, c text[])
WHERE t.a > 7;
ERROR: could not establish connection
DETAIL: missing "=" after "myconn" in connection info string
-
-- create a named persistent connection
SELECT dblink_connect('myconn',connection_parameters());
dblink_connect
@@ -604,7 +603,6 @@ FROM dblink('myconn','SELECT * FROM foo') AS t(a int, b text, c text[])
WHERE t.a > 7;
ERROR: could not establish connection
DETAIL: missing "=" after "myconn" in connection info string
-
-- create a named persistent connection
SELECT dblink_connect('myconn',connection_parameters());
dblink_connect
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 7f7a744ac0..c6e3d44515 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -226,21 +226,11 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
conn = PQconnectdbParams(keywords, values, false);
if (!conn || PQstatus(conn) != CONNECTION_OK)
- {
- char *connmessage;
- int msglen;
-
- /* libpq typically appends a newline, strip that */
- connmessage = pstrdup(PQerrorMessage(conn));
- msglen = strlen(connmessage);
- if (msglen > 0 && connmessage[msglen - 1] == '\n')
- connmessage[msglen - 1] = '\0';
ereport(ERROR,
(errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
errmsg("could not connect to server \"%s\"",
server->servername),
- errdetail_internal("%s", connmessage)));
- }
+ errdetail_internal("%s", pchomp(PQerrorMessage(conn)))));
/*
* Check that non-superuser has used password to establish connection;
@@ -563,7 +553,7 @@ pgfdw_report_error(int elevel, PGresult *res, PGconn *conn,
* return NULL, not a PGresult at all.
*/
if (message_primary == NULL)
- message_primary = PQerrorMessage(conn);
+ message_primary = pchomp(PQerrorMessage(conn));
ereport(elevel,
(errcode(sqlstate),
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 44a89c73fd..b88760fb68 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -141,7 +141,7 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
conn->streamConn = PQconnectdbParams(keys, vals, /* expand_dbname = */ true);
if (PQstatus(conn->streamConn) != CONNECTION_OK)
{
- *err = pstrdup(PQerrorMessage(conn->streamConn));
+ *err = pchomp(PQerrorMessage(conn->streamConn));
return NULL;
}
@@ -239,7 +239,7 @@ libpqrcv_identify_system(WalReceiverConn *conn, TimeLineID *primary_tli,
ereport(ERROR,
(errmsg("could not receive database system identifier and timeline ID from "
"the primary server: %s",
- PQerrorMessage(conn->streamConn))));
+ pchomp(PQerrorMessage(conn->streamConn)))));
}
if (PQnfields(res) < 3 || PQntuples(res) != 1)
{
@@ -316,13 +316,13 @@ libpqrcv_startstreaming(WalReceiverConn *conn,
if (!pubnames_str)
ereport(ERROR,
(errmsg("could not start WAL streaming: %s",
- PQerrorMessage(conn->streamConn))));
+ pchomp(PQerrorMessage(conn->streamConn)))));
pubnames_literal = PQescapeLiteral(conn->streamConn, pubnames_str,
strlen(pubnames_str));
if (!pubnames_literal)
ereport(ERROR,
(errmsg("could not start WAL streaming: %s",
- PQerrorMessage(conn->streamConn))));
+ pchomp(PQerrorMessage(conn->streamConn)))));
appendStringInfo(&cmd, ", publication_names %s", pubnames_literal);
PQfreemem(pubnames_literal);
pfree(pubnames_str);
@@ -347,7 +347,7 @@ libpqrcv_startstreaming(WalReceiverConn *conn,
PQclear(res);
ereport(ERROR,
(errmsg("could not start WAL streaming: %s",
- PQerrorMessage(conn->streamConn))));
+ pchomp(PQerrorMessage(conn->streamConn)))));
}
PQclear(res);
return true;
@@ -366,7 +366,7 @@ libpqrcv_endstreaming(WalReceiverConn *conn, TimeLineID *next_tli)
PQflush(conn->streamConn))
ereport(ERROR,
(errmsg("could not send end-of-streaming message to primary: %s",
- PQerrorMessage(conn->streamConn))));
+ pchomp(PQerrorMessage(conn->streamConn)))));
*next_tli = 0;
@@ -410,7 +410,7 @@ libpqrcv_endstreaming(WalReceiverConn *conn, TimeLineID *next_tli)
if (PQresultStatus(res) != PGRES_COMMAND_OK)
ereport(ERROR,
(errmsg("error reading result of streaming command: %s",
- PQerrorMessage(conn->streamConn))));
+ pchomp(PQerrorMessage(conn->streamConn)))));
PQclear(res);
/* Verify that there are no more results */
@@ -418,7 +418,7 @@ libpqrcv_endstreaming(WalReceiverConn *conn, TimeLineID *next_tli)
if (res != NULL)
ereport(ERROR,
(errmsg("unexpected result after CommandComplete: %s",
- PQerrorMessage(conn->streamConn))));
+ pchomp(PQerrorMessage(conn->streamConn)))));
}
/*
@@ -445,7 +445,7 @@ libpqrcv_readtimelinehistoryfile(WalReceiverConn *conn,
ereport(ERROR,
(errmsg("could not receive timeline history file from "
"the primary server: %s",
- PQerrorMessage(conn->streamConn))));
+ pchomp(PQerrorMessage(conn->streamConn)))));
}
if (PQnfields(res) != 2 || PQntuples(res) != 1)
{
@@ -603,7 +603,7 @@ libpqrcv_receive(WalReceiverConn *conn, char **buffer,
if (PQconsumeInput(conn->streamConn) == 0)
ereport(ERROR,
(errmsg("could not receive data from WAL stream: %s",
- PQerrorMessage(conn->streamConn))));
+ pchomp(PQerrorMessage(conn->streamConn)))));
/* Now that we've consumed some input, try again */
rawlen = PQgetCopyData(conn->streamConn, &conn->recvBuf, 1);
@@ -630,13 +630,13 @@ libpqrcv_receive(WalReceiverConn *conn, char **buffer,
PQclear(res);
ereport(ERROR,
(errmsg("could not receive data from WAL stream: %s",
- PQerrorMessage(conn->streamConn))));
+ pchomp(PQerrorMessage(conn->streamConn)))));
}
}
if (rawlen < -1)
ereport(ERROR,
(errmsg("could not receive data from WAL stream: %s",
- PQerrorMessage(conn->streamConn))));
+ pchomp(PQerrorMessage(conn->streamConn)))));
/* Return received messages to caller */
*buffer = conn->recvBuf;
@@ -655,7 +655,7 @@ libpqrcv_send(WalReceiverConn *conn, const char *buffer, int nbytes)
PQflush(conn->streamConn))
ereport(ERROR,
(errmsg("could not send data to WAL stream: %s",
- PQerrorMessage(conn->streamConn))));
+ pchomp(PQerrorMessage(conn->streamConn)))));
}
/*
@@ -689,7 +689,7 @@ libpqrcv_create_slot(WalReceiverConn *conn, const char *slotname,
PQclear(res);
ereport(ERROR,
(errmsg("could not create replication slot \"%s\": %s",
- slotname, PQerrorMessage(conn->streamConn))));
+ slotname, pchomp(PQerrorMessage(conn->streamConn)))));
}
*lsn = DatumGetLSN(DirectFunctionCall1Coll(pg_lsn_in, InvalidOid,
@@ -720,7 +720,7 @@ libpqrcv_command(WalReceiverConn *conn, const char *cmd, char **err)
if (PQresultStatus(res) != PGRES_COMMAND_OK)
{
PQclear(res);
- *err = pstrdup(PQerrorMessage(conn->streamConn));
+ *err = pchomp(PQerrorMessage(conn->streamConn));
return false;
}
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 6ad0bb47b6..c21dfcb96b 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -1181,3 +1181,17 @@ pnstrdup(const char *in, Size len)
out[len] = '\0';
return out;
}
+
+/*
+ * Make copy of string with all trailing newline characters removed.
+ */
+char *
+pchomp(const char *in)
+{
+ size_t n;
+
+ n = strlen(in);
+ while (n > 0 && in[n - 1] == '\n')
+ n--;
+ return pnstrdup(in, n);
+}
diff --git a/src/include/utils/palloc.h b/src/include/utils/palloc.h
index b72fe4aee8..2e07bd57ad 100644
--- a/src/include/utils/palloc.h
+++ b/src/include/utils/palloc.h
@@ -127,6 +127,8 @@ extern char *MemoryContextStrdup(MemoryContext context, const char *string);
extern char *pstrdup(const char *in);
extern char *pnstrdup(const char *in, Size len);
+extern char *pchomp(const char *in);
+
/* sprintf into a palloc'd buffer --- these are in psprintf.c */
extern char *psprintf(const char *fmt,...) pg_attribute_printf(1, 2);
extern size_t pvsnprintf(char *buf, size_t len, const char *fmt, va_list args) pg_attribute_printf(3, 0);
--
2.11.1
Re: chomp PQerrorMessage() in backend uses
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
Here is a patch to systematically trim the trailing newlines off
PQerrorMessage() results in backend uses (dblink, postgres_fdw,
libpqwalreceiver).
+1
I noticed that there are some inconsistent assumptions about whether
PQerrorMessage() can ever return NULL. From the code, I think that
should not be possible, but some code appears to be prepared for it.
Other code is not. What is correct?
libpq.sgml doesn't specify, so it's hard to argue that either position
is "correct". I don't mind resolving the ambiguity via a documentation
change though. I'd want to see it also cover other corner cases like
what if there hasn't been an error on the connection.
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
Re: chomp PQerrorMessage() in backend uses
On 2/8/17 11:00, Tom Lane wrote:
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
Here is a patch to systematically trim the trailing newlines off
PQerrorMessage() results in backend uses (dblink, postgres_fdw,
libpqwalreceiver).+1
committed
I noticed that there are some inconsistent assumptions about whether
PQerrorMessage() can ever return NULL. From the code, I think that
should not be possible, but some code appears to be prepared for it.
Other code is not. What is correct?libpq.sgml doesn't specify, so it's hard to argue that either position
is "correct". I don't mind resolving the ambiguity via a documentation
change though. I'd want to see it also cover other corner cases like
what if there hasn't been an error on the connection.
I didn't dare to venture deeper into this right now. The committed
change shouldn't gain or lose anything with respect to this question.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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