In libpq, always append new error messages to conn->errorMessage.

Previously, we had an undisciplined mish-mash of printfPQExpBuffer and
appendPQExpBuffer calls to report errors within libpq.  This commit
establishes a uniform rule that appendPQExpBuffer[Str] should be used.
conn->errorMessage is reset only at the start of an application request,
and then accumulates messages till we're done.  We can remove no less
than three different ad-hoc mechanisms that were used to get the effect
of concatenation of error messages within a sequence of operations.

Although this makes things quite a bit cleaner conceptually, the main
reason to do it is to make the world safer for the multiple-target-host
feature that was added awhile back.  Previously, there were many cases
in which an error occurring during an individual host connection attempt
would wipe out the record of what had happened during previous attempts.
(The reporting is still inadequate, in that it can be hard to tell which
host got the failure, but that seems like a matter for a separate commit.)

Currently, lo_import and lo_export contain exceptions to the "never
use printfPQExpBuffer" rule.  If we changed them, we'd risk reporting
an incidental lo_close failure before the actual read or write
failure, which would be confusing, not least because lo_close happened
after the main failure.  We could improve this by inventing an
internal version of lo_close that doesn't reset the errorMessage; but
we'd also need a version of PQfn() that does that, and it didn't quite
seem worth the trouble for now.

Discussion: https://postgr.es/m/BN6PR05MB3492948E4FD76C156E747E8BC9160@BN6PR05MB3492.namprd05.prod.outlook.com

1 files changed, 102 insertions, 108 deletions

diff --git a/src/interfaces/libpq/fe-lobj.c b/src/interfaces/libpq/fe-lobj.c
index 432935061f0..b9c52eb50c2 100644
--- a/src/interfaces/libpq/fe-lobj.c
+++ b/src/interfaces/libpq/fe-lobj.c
@@ -61,11 +61,8 @@ lo_open(PGconn *conn, Oid lobjId, int mode)
 	PQArgBlock	argv[2];
 	PGresult   *res;
 
-	if (conn == NULL || conn->lobjfuncs == NULL)
-	{
-		if (lo_initialize(conn) < 0)
-			return -1;
-	}
+	if (lo_initialize(conn) < 0)
+		return -1;
 
 	argv[0].isint = 1;
 	argv[0].len = 4;
@@ -103,11 +100,8 @@ lo_close(PGconn *conn, int fd)
 	int			retval;
 	int			result_len;
 
-	if (conn == NULL || conn->lobjfuncs == NULL)
-	{
-		if (lo_initialize(conn) < 0)
-			return -1;
-	}
+	if (lo_initialize(conn) < 0)
+		return -1;
 
 	argv[0].isint = 1;
 	argv[0].len = 4;
@@ -141,17 +135,15 @@ lo_truncate(PGconn *conn, int fd, size_t len)
 	int			retval;
 	int			result_len;
 
-	if (conn == NULL || conn->lobjfuncs == NULL)
-	{
-		if (lo_initialize(conn) < 0)
-			return -1;
-	}
+	if (lo_initialize(conn) < 0)
+		return -1;
 
 	/* Must check this on-the-fly because it's not there pre-8.3 */
 	if (conn->lobjfuncs->fn_lo_truncate == 0)
 	{
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("cannot determine OID of function lo_truncate\n"));
+		appendPQExpBuffer(&conn->errorMessage,
+						  libpq_gettext("cannot determine OID of function %s\n"),
+						  "lo_truncate");
 		return -1;
 	}
 
@@ -166,8 +158,8 @@ lo_truncate(PGconn *conn, int fd, size_t len)
 	 */
 	if (len > (size_t) INT_MAX)
 	{
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("argument of lo_truncate exceeds integer range\n"));
+		appendPQExpBufferStr(&conn->errorMessage,
+							 libpq_gettext("argument of lo_truncate exceeds integer range\n"));
 		return -1;
 	}
 
@@ -209,16 +201,14 @@ lo_truncate64(PGconn *conn, int fd, pg_int64 len)
 	int			retval;
 	int			result_len;
 
-	if (conn == NULL || conn->lobjfuncs == NULL)
-	{
-		if (lo_initialize(conn) < 0)
-			return -1;
-	}
+	if (lo_initialize(conn) < 0)
+		return -1;
 
 	if (conn->lobjfuncs->fn_lo_truncate64 == 0)
 	{
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("cannot determine OID of function lo_truncate64\n"));
+		appendPQExpBuffer(&conn->errorMessage,
+						  libpq_gettext("cannot determine OID of function %s\n"),
+						  "lo_truncate64");
 		return -1;
 	}
 
@@ -261,11 +251,8 @@ lo_read(PGconn *conn, int fd, char *buf, size_t len)
 	PGresult   *res;
 	int			result_len;
 
-	if (conn == NULL || conn->lobjfuncs == NULL)
-	{
-		if (lo_initialize(conn) < 0)
-			return -1;
-	}
+	if (lo_initialize(conn) < 0)
+		return -1;
 
 	/*
 	 * Long ago, somebody thought it'd be a good idea to declare this function
@@ -275,8 +262,8 @@ lo_read(PGconn *conn, int fd, char *buf, size_t len)
 	 */
 	if (len > (size_t) INT_MAX)
 	{
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("argument of lo_read exceeds integer range\n"));
+		appendPQExpBufferStr(&conn->errorMessage,
+							 libpq_gettext("argument of lo_read exceeds integer range\n"));
 		return -1;
 	}
 
@@ -316,11 +303,8 @@ lo_write(PGconn *conn, int fd, const char *buf, size_t len)
 	int			result_len;
 	int			retval;
 
-	if (conn == NULL || conn->lobjfuncs == NULL)
-	{
-		if (lo_initialize(conn) < 0)
-			return -1;
-	}
+	if (lo_initialize(conn) < 0)
+		return -1;
 
 	/*
 	 * Long ago, somebody thought it'd be a good idea to declare this function
@@ -330,8 +314,8 @@ lo_write(PGconn *conn, int fd, const char *buf, size_t len)
 	 */
 	if (len > (size_t) INT_MAX)
 	{
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("argument of lo_write exceeds integer range\n"));
+		appendPQExpBufferStr(&conn->errorMessage,
+							 libpq_gettext("argument of lo_write exceeds integer range\n"));
 		return -1;
 	}
 
@@ -369,11 +353,8 @@ lo_lseek(PGconn *conn, int fd, int offset, int whence)
 	int			retval;
 	int			result_len;
 
-	if (conn == NULL || conn->lobjfuncs == NULL)
-	{
-		if (lo_initialize(conn) < 0)
-			return -1;
-	}
+	if (lo_initialize(conn) < 0)
+		return -1;
 
 	argv[0].isint = 1;
 	argv[0].len = 4;
@@ -413,16 +394,14 @@ lo_lseek64(PGconn *conn, int fd, pg_int64 offset, int whence)
 	pg_int64	retval;
 	int			result_len;
 
-	if (conn == NULL || conn->lobjfuncs == NULL)
-	{
-		if (lo_initialize(conn) < 0)
-			return -1;
-	}
+	if (lo_initialize(conn) < 0)
+		return -1;
 
 	if (conn->lobjfuncs->fn_lo_lseek64 == 0)
 	{
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("cannot determine OID of function lo_lseek64\n"));
+		appendPQExpBuffer(&conn->errorMessage,
+						  libpq_gettext("cannot determine OID of function %s\n"),
+						  "lo_lseek64");
 		return -1;
 	}
 
@@ -469,11 +448,8 @@ lo_creat(PGconn *conn, int mode)
 	int			retval;
 	int			result_len;
 
-	if (conn == NULL || conn->lobjfuncs == NULL)
-	{
-		if (lo_initialize(conn) < 0)
-			return InvalidOid;
-	}
+	if (lo_initialize(conn) < 0)
+		return InvalidOid;
 
 	argv[0].isint = 1;
 	argv[0].len = 4;
@@ -508,17 +484,15 @@ lo_create(PGconn *conn, Oid lobjId)
 	int			retval;
 	int			result_len;
 
-	if (conn == NULL || conn->lobjfuncs == NULL)
-	{
-		if (lo_initialize(conn) < 0)
-			return InvalidOid;
-	}
+	if (lo_initialize(conn) < 0)
+		return InvalidOid;
 
 	/* Must check this on-the-fly because it's not there pre-8.1 */
 	if (conn->lobjfuncs->fn_lo_create == 0)
 	{
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("cannot determine OID of function lo_create\n"));
+		appendPQExpBuffer(&conn->errorMessage,
+						  libpq_gettext("cannot determine OID of function %s\n"),
+						  "lo_create");
 		return InvalidOid;
 	}
 
@@ -552,11 +526,8 @@ lo_tell(PGconn *conn, int fd)
 	PGresult   *res;
 	int			result_len;
 
-	if (conn == NULL || conn->lobjfuncs == NULL)
-	{
-		if (lo_initialize(conn) < 0)
-			return -1;
-	}
+	if (lo_initialize(conn) < 0)
+		return -1;
 
 	argv[0].isint = 1;
 	argv[0].len = 4;
@@ -588,16 +559,14 @@ lo_tell64(PGconn *conn, int fd)
 	PGresult   *res;
 	int			result_len;
 
-	if (conn == NULL || conn->lobjfuncs == NULL)
-	{
-		if (lo_initialize(conn) < 0)
-			return -1;
-	}
+	if (lo_initialize(conn) < 0)
+		return -1;
 
 	if (conn->lobjfuncs->fn_lo_tell64 == 0)
 	{
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("cannot determine OID of function lo_tell64\n"));
+		appendPQExpBuffer(&conn->errorMessage,
+						  libpq_gettext("cannot determine OID of function %s\n"),
+						  "lo_tell64");
 		return -1;
 	}
 
@@ -632,11 +601,8 @@ lo_unlink(PGconn *conn, Oid lobjId)
 	int			result_len;
 	int			retval;
 
-	if (conn == NULL || conn->lobjfuncs == NULL)
-	{
-		if (lo_initialize(conn) < 0)
-			return -1;
-	}
+	if (lo_initialize(conn) < 0)
+		return -1;
 
 	argv[0].isint = 1;
 	argv[0].len = 4;
@@ -696,13 +662,19 @@ lo_import_internal(PGconn *conn, const char *filename, Oid oid)
 	int			lobj;
 	char		sebuf[PG_STRERROR_R_BUFLEN];
 
+	if (conn == NULL)
+		return InvalidOid;
+
+	/* Since this is the beginning of a query cycle, reset the error buffer */
+	resetPQExpBuffer(&conn->errorMessage);
+
 	/*
 	 * open the file to be read in
 	 */
 	fd = open(filename, O_RDONLY | PG_BINARY, 0666);
 	if (fd < 0)
 	{							/* error */
-		printfPQExpBuffer(&conn->errorMessage,
+		appendPQExpBuffer(&conn->errorMessage,
 						  libpq_gettext("could not open file \"%s\": %s\n"),
 						  filename, strerror_r(errno, sebuf, sizeof(sebuf)));
 		return InvalidOid;
@@ -757,6 +729,7 @@ lo_import_internal(PGconn *conn, const char *filename, Oid oid)
 
 		(void) lo_close(conn, lobj);
 		(void) close(fd);
+		/* deliberately overwrite any error from lo_close */
 		printfPQExpBuffer(&conn->errorMessage,
 						  libpq_gettext("could not read from file \"%s\": %s\n"),
 						  filename,
@@ -811,6 +784,7 @@ lo_export(PGconn *conn, Oid lobjId, const char *filename)
 		int			save_errno = errno;
 
 		(void) lo_close(conn, lobj);
+		/* deliberately overwrite any error from lo_close */
 		printfPQExpBuffer(&conn->errorMessage,
 						  libpq_gettext("could not open file \"%s\": %s\n"),
 						  filename,
@@ -831,6 +805,7 @@ lo_export(PGconn *conn, Oid lobjId, const char *filename)
 
 			(void) lo_close(conn, lobj);
 			(void) close(fd);
+			/* deliberately overwrite any error from lo_close */
 			printfPQExpBuffer(&conn->errorMessage,
 							  libpq_gettext("could not write to file \"%s\": %s\n"),
 							  filename,
@@ -855,7 +830,7 @@ lo_export(PGconn *conn, Oid lobjId, const char *filename)
 	/* if we already failed, don't overwrite that msg with a close error */
 	if (close(fd) != 0 && result >= 0)
 	{
-		printfPQExpBuffer(&conn->errorMessage,
+		appendPQExpBuffer(&conn->errorMessage,
 						  libpq_gettext("could not write to file \"%s\": %s\n"),
 						  filename, strerror_r(errno, sebuf, sizeof(sebuf)));
 		result = -1;
@@ -868,9 +843,11 @@ lo_export(PGconn *conn, Oid lobjId, const char *filename)
 /*
  * lo_initialize
  *
- * Initialize the large object interface for an existing connection.
- * We ask the backend about the functions OID's in pg_proc for all
- * functions that are required for large object operations.
+ * Initialize for a new large-object operation on an existing connection.
+ * Return 0 if OK, -1 on failure.
+ *
+ * If we haven't previously done so, we collect the function OIDs from
+ * pg_proc for all functions that are required for large object operations.
  */
 static int
 lo_initialize(PGconn *conn)
@@ -882,17 +859,26 @@ lo_initialize(PGconn *conn)
 	const char *fname;
 	Oid			foid;
 
-	if (!conn)
+	/* Nothing we can do with no connection */
+	if (conn == NULL)
 		return -1;
 
+	/* Since this is the beginning of a query cycle, reset the error buffer */
+	resetPQExpBuffer(&conn->errorMessage);
+
+	/* Nothing else to do if we already collected info */
+	if (conn->lobjfuncs != NULL)
+		return 0;
+
 	/*
-	 * Allocate the structure to hold the functions OID's
+	 * Allocate the structure to hold the function OIDs.  We don't store it
+	 * into the PGconn until it's successfully filled.
 	 */
 	lobjfuncs = (PGlobjfuncs *) malloc(sizeof(PGlobjfuncs));
 	if (lobjfuncs == NULL)
 	{
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("out of memory\n"));
+		appendPQExpBufferStr(&conn->errorMessage,
+							 libpq_gettext("out of memory\n"));
 		return -1;
 	}
 	MemSet((char *) lobjfuncs, 0, sizeof(PGlobjfuncs));
@@ -942,8 +928,8 @@ lo_initialize(PGconn *conn)
 	{
 		free(lobjfuncs);
 		PQclear(res);
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("query to initialize large object functions did not return data\n"));
+		appendPQExpBufferStr(&conn->errorMessage,
+							 libpq_gettext("query to initialize large object functions did not return data\n"));
 		return -1;
 	}
 
@@ -991,57 +977,65 @@ lo_initialize(PGconn *conn)
 	 */
 	if (lobjfuncs->fn_lo_open == 0)
 	{
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("cannot determine OID of function lo_open\n"));
+		appendPQExpBuffer(&conn->errorMessage,
+						  libpq_gettext("cannot determine OID of function %s\n"),
+						  "lo_open");
 		free(lobjfuncs);
 		return -1;
 	}
 	if (lobjfuncs->fn_lo_close == 0)
 	{
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("cannot determine OID of function lo_close\n"));
+		appendPQExpBuffer(&conn->errorMessage,
+						  libpq_gettext("cannot determine OID of function %s\n"),
+						  "lo_close");
 		free(lobjfuncs);
 		return -1;
 	}
 	if (lobjfuncs->fn_lo_creat == 0)
 	{
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("cannot determine OID of function lo_creat\n"));
+		appendPQExpBuffer(&conn->errorMessage,
+						  libpq_gettext("cannot determine OID of function %s\n"),
+						  "lo_creat");
 		free(lobjfuncs);
 		return -1;
 	}
 	if (lobjfuncs->fn_lo_unlink == 0)
 	{
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("cannot determine OID of function lo_unlink\n"));
+		appendPQExpBuffer(&conn->errorMessage,
+						  libpq_gettext("cannot determine OID of function %s\n"),
+						  "lo_unlink");
 		free(lobjfuncs);
 		return -1;
 	}
 	if (lobjfuncs->fn_lo_lseek == 0)
 	{
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("cannot determine OID of function lo_lseek\n"));
+		appendPQExpBuffer(&conn->errorMessage,
+						  libpq_gettext("cannot determine OID of function %s\n"),
+						  "lo_lseek");
 		free(lobjfuncs);
 		return -1;
 	}
 	if (lobjfuncs->fn_lo_tell == 0)
 	{
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("cannot determine OID of function lo_tell\n"));
+		appendPQExpBuffer(&conn->errorMessage,
+						  libpq_gettext("cannot determine OID of function %s\n"),
+						  "lo_tell");
 		free(lobjfuncs);
 		return -1;
 	}
 	if (lobjfuncs->fn_lo_read == 0)
 	{
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("cannot determine OID of function loread\n"));
+		appendPQExpBuffer(&conn->errorMessage,
+						  libpq_gettext("cannot determine OID of function %s\n"),
+						  "loread");
 		free(lobjfuncs);
 		return -1;
 	}
 	if (lobjfuncs->fn_lo_write == 0)
 	{
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("cannot determine OID of function lowrite\n"));
+		appendPQExpBuffer(&conn->errorMessage,
+						  libpq_gettext("cannot determine OID of function %s\n"),
+						  "lowrite");
 		free(lobjfuncs);
 		return -1;
 	}