Improve style guideline compliance of assorted error-report messages.

Per the project style guide, details and hints should have leading
capitalization and end with a period.  On the other hand, errcontext should
not be capitalized and should not end with a period.  To support well
formatted error contexts in dblink, extend dblink_res_error() to take a
format+arguments rather than a hardcoded string.

Daniel Gustafsson

Discussion: https://postgr.es/m/B3C002C8-21A0-4F53-A06E-8CAB29FCF295@yesql.se

Author: tglsfdc

contrib/dblink/dblink.c

@@ -113,7 +113,7 @@ static char *generate_relation_name(Relation rel);
 static void dblink_connstr_check(const char *connstr);
 static void dblink_security_check(PGconn *conn, remoteConn *rconn);
 static void dblink_res_error(PGconn *conn, const char *conname, PGresult *res,
-				 const char *dblink_context_msg, bool fail);
+				 bool fail, const char *fmt,...) pg_attribute_printf(5, 6);
 static char *get_connect_string(const char *servername);
 static char *escape_param_str(const char *from);
 static void validate_pkattnums(Relation rel,
@@ -441,7 +441,8 @@ dblink_open(PG_FUNCTION_ARGS)
 	res = PQexec(conn, buf.data);
 	if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
 	{
-		dblink_res_error(conn, conname, res, "could not open cursor", fail);
+		dblink_res_error(conn, conname, res, fail,
+						 "while opening cursor \"%s\"", curname);
 		PG_RETURN_TEXT_P(cstring_to_text("ERROR"));
 	}
 
@@ -509,7 +510,8 @@ dblink_close(PG_FUNCTION_ARGS)
 	res = PQexec(conn, buf.data);
 	if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
 	{
-		dblink_res_error(conn, conname, res, "could not close cursor", fail);
+		dblink_res_error(conn, conname, res, fail,
+						 "while closing cursor \"%s\"", curname);
 		PG_RETURN_TEXT_P(cstring_to_text("ERROR"));
 	}
 
@@ -612,8 +614,8 @@ dblink_fetch(PG_FUNCTION_ARGS)
 		(PQresultStatus(res) != PGRES_COMMAND_OK &&
 		 PQresultStatus(res) != PGRES_TUPLES_OK))
 	{
-		dblink_res_error(conn, conname, res,
-						 "could not fetch from cursor", fail);
+		dblink_res_error(conn, conname, res, fail,
+						 "while fetching from cursor \"%s\"", curname);
 		return (Datum) 0;
 	}
 	else if (PQresultStatus(res) == PGRES_COMMAND_OK)
@@ -763,8 +765,8 @@ dblink_record_internal(FunctionCallInfo fcinfo, bool is_async)
 				if (PQresultStatus(res) != PGRES_COMMAND_OK &&
 					PQresultStatus(res) != PGRES_TUPLES_OK)
 				{
-					dblink_res_error(conn, conname, res,
-									 "could not execute query", fail);
+					dblink_res_error(conn, conname, res, fail,
+									 "while executing query");
 					/* if fail isn't set, we'll return an empty query result */
 				}
 				else
@@ -1009,8 +1011,8 @@ materializeQueryResult(FunctionCallInfo fcinfo,
 			PGresult   *res1 = res;
 
 			res = NULL;
-			dblink_res_error(conn, conname, res1,
-							 "could not execute query", fail);
+			dblink_res_error(conn, conname, res1, fail,
+							 "while executing query");
 			/* if fail isn't set, we'll return an empty query result */
 		}
 		else if (PQresultStatus(res) == PGRES_COMMAND_OK)
@@ -1438,8 +1440,8 @@ dblink_exec(PG_FUNCTION_ARGS)
 			(PQresultStatus(res) != PGRES_COMMAND_OK &&
 			 PQresultStatus(res) != PGRES_TUPLES_OK))
 		{
-			dblink_res_error(conn, conname, res,
-							 "could not execute command", fail);
+			dblink_res_error(conn, conname, res, fail,
+							 "while executing command");
 
 			/*
 			 * and save a copy of the command status string to return as our
@@ -1980,7 +1982,7 @@ dblink_fdw_validator(PG_FUNCTION_ARGS)
 			ereport(ERROR,
 					(errcode(ERRCODE_FDW_OUT_OF_MEMORY),
 					 errmsg("out of memory"),
-					 errdetail("could not get libpq's default connection options")));
+					 errdetail("Could not get libpq's default connection options.")));
 	}
 
 	/* Validate each supplied option. */
@@ -2676,9 +2678,17 @@ dblink_connstr_check(const char *connstr)
 	}
 }
 
+/*
+ * Report an error received from the remote server
+ *
+ * res: the received error result (will be freed)
+ * fail: true for ERROR ereport, false for NOTICE
+ * fmt and following args: sprintf-style format and values for errcontext;
+ * the resulting string should be worded like "while <some action>"
+ */
 static void
 dblink_res_error(PGconn *conn, const char *conname, PGresult *res,
-				 const char *dblink_context_msg, bool fail)
+				 bool fail, const char *fmt,...)
 {
 	int			level;
 	char	   *pg_diag_sqlstate = PQresultErrorField(res, PG_DIAG_SQLSTATE);
@@ -2691,7 +2701,8 @@ dblink_res_error(PGconn *conn, const char *conname, PGresult *res,
 	char	   *message_detail;
 	char	   *message_hint;
 	char	   *message_context;
-	const char *dblink_context_conname = "unnamed";
+	va_list		ap;
+	char		dblink_context_msg[512];
 
 	if (fail)
 		level = ERROR;
@@ -2720,21 +2731,38 @@ dblink_res_error(PGconn *conn, const char *conname, PGresult *res,
 	if (message_primary == NULL)
 		message_primary = pchomp(PQerrorMessage(conn));
 
+	/*
+	 * Now that we've copied all the data we need out of the PGresult, it's
+	 * safe to free it.  We must do this to avoid PGresult leakage.  We're
+	 * leaking all the strings too, but those are in palloc'd memory that will
+	 * get cleaned up eventually.
+	 */
 	if (res)
 		PQclear(res);
 
-	if (conname)
-		dblink_context_conname = conname;
+	/*
+	 * Format the basic errcontext string.  Below, we'll add on something
+	 * about the connection name.  That's a violation of the translatability
+	 * guidelines about constructing error messages out of parts, but since
+	 * there's no translation support for dblink, there's no need to worry
+	 * about that (yet).
+	 */
+	va_start(ap, fmt);
+	vsnprintf(dblink_context_msg, sizeof(dblink_context_msg), fmt, ap);
+	va_end(ap);
 
 	ereport(level,
 			(errcode(sqlstate),
 			 message_primary ? errmsg_internal("%s", message_primary) :
 			 errmsg("could not obtain message string for remote error"),
 			 message_detail ? errdetail_internal("%s", message_detail) : 0,
 			 message_hint ? errhint("%s", message_hint) : 0,
-			 message_context ? errcontext("%s", message_context) : 0,
-			 errcontext("Error occurred on dblink connection named \"%s\": %s.",
-						dblink_context_conname, dblink_context_msg)));
+			 message_context ? (errcontext("%s", message_context)) : 0,
+			 conname ?
+			 (errcontext("%s on dblink connection named \"%s\"",
+						 dblink_context_msg, conname)) :
+			 (errcontext("%s on unnamed dblink connection",
+						 dblink_context_msg))));
 }
 
 /*
@@ -2769,7 +2797,7 @@ get_connect_string(const char *servername)
 			ereport(ERROR,
 					(errcode(ERRCODE_FDW_OUT_OF_MEMORY),
 					 errmsg("out of memory"),
-					 errdetail("could not get libpq's default connection options")));
+					 errdetail("Could not get libpq's default connection options.")));
 	}
 
 	/* first gather the server connstr options */

contrib/dblink/expected/dblink.out

@@ -155,7 +155,7 @@ WHERE t.a > 7;
 -- open a cursor with bad SQL and fail_on_error set to false
 SELECT dblink_open('rmt_foo_cursor','SELECT * FROM foobar',false);
 NOTICE:  relation "foobar" does not exist
-CONTEXT:  Error occurred on dblink connection named "unnamed": could not open cursor.
+CONTEXT:  while opening cursor "rmt_foo_cursor" on unnamed dblink connection
  dblink_open 
 -------------
  ERROR
@@ -223,7 +223,7 @@ FROM dblink_fetch('rmt_foo_cursor',4) AS t(a int, b text, c text[]);
 SELECT *
 FROM dblink_fetch('rmt_foobar_cursor',4,false) AS t(a int, b text, c text[]);
 NOTICE:  cursor "rmt_foobar_cursor" does not exist
-CONTEXT:  Error occurred on dblink connection named "unnamed": could not fetch from cursor.
+CONTEXT:  while fetching from cursor "rmt_foobar_cursor" on unnamed dblink connection
  a | b | c 
 ---+---+---
 (0 rows)
@@ -238,7 +238,7 @@ SELECT dblink_exec('ABORT');
 -- close the wrong cursor
 SELECT dblink_close('rmt_foobar_cursor',false);
 NOTICE:  cursor "rmt_foobar_cursor" does not exist
-CONTEXT:  Error occurred on dblink connection named "unnamed": could not close cursor.
+CONTEXT:  while closing cursor "rmt_foobar_cursor" on unnamed dblink connection
  dblink_close 
 --------------
  ERROR
@@ -248,12 +248,12 @@ CONTEXT:  Error occurred on dblink connection named "unnamed": could not close c
 SELECT *
 FROM dblink_fetch('rmt_foo_cursor',4) AS t(a int, b text, c text[]);
 ERROR:  cursor "rmt_foo_cursor" does not exist
-CONTEXT:  Error occurred on dblink connection named "unnamed": could not fetch from cursor.
+CONTEXT:  while fetching from cursor "rmt_foo_cursor" on unnamed dblink connection
 -- this time, 'cursor "rmt_foo_cursor" not found' as a notice
 SELECT *
 FROM dblink_fetch('rmt_foo_cursor',4,false) AS t(a int, b text, c text[]);
 NOTICE:  cursor "rmt_foo_cursor" does not exist
-CONTEXT:  Error occurred on dblink connection named "unnamed": could not fetch from cursor.
+CONTEXT:  while fetching from cursor "rmt_foo_cursor" on unnamed dblink connection
  a | b | c 
 ---+---+---
 (0 rows)
@@ -316,7 +316,7 @@ FROM dblink('SELECT * FROM foo') AS t(a int, b text, c text[]);
 SELECT *
 FROM dblink('SELECT * FROM foobar',false) AS t(a int, b text, c text[]);
 NOTICE:  relation "foobar" does not exist
-CONTEXT:  Error occurred on dblink connection named "unnamed": could not execute query.
+CONTEXT:  while executing query on unnamed dblink connection
  a | b | c 
 ---+---+---
 (0 rows)
@@ -340,7 +340,7 @@ WHERE a = 11;
 -- botch a change to some other data
 SELECT dblink_exec('UPDATE foobar SET f3[2] = ''b99'' WHERE f1 = 11',false);
 NOTICE:  relation "foobar" does not exist
-CONTEXT:  Error occurred on dblink connection named "unnamed": could not execute command.
+CONTEXT:  while executing command on unnamed dblink connection
  dblink_exec 
 -------------
  ERROR
@@ -400,7 +400,7 @@ SELECT *
 FROM dblink('myconn','SELECT * FROM foobar',false) AS t(a int, b text, c text[])
 WHERE t.a > 7;
 NOTICE:  relation "foobar" does not exist
-CONTEXT:  Error occurred on dblink connection named "myconn": could not execute query.
+CONTEXT:  while executing query on dblink connection named "myconn"
  a | b | c 
 ---+---+---
 (0 rows)
@@ -437,7 +437,7 @@ SELECT dblink_disconnect('myconn2');
 -- open a cursor incorrectly
 SELECT dblink_open('myconn','rmt_foo_cursor','SELECT * FROM foobar',false);
 NOTICE:  relation "foobar" does not exist
-CONTEXT:  Error occurred on dblink connection named "myconn": could not open cursor.
+CONTEXT:  while opening cursor "rmt_foo_cursor" on dblink connection named "myconn"
  dblink_open 
 -------------
  ERROR
@@ -523,7 +523,7 @@ SELECT dblink_close('myconn','rmt_foo_cursor');
 -- this should fail because there is no open transaction
 SELECT dblink_exec('myconn','DECLARE xact_test CURSOR FOR SELECT * FROM foo');
 ERROR:  DECLARE CURSOR can only be used in transaction blocks
-CONTEXT:  Error occurred on dblink connection named "myconn": could not execute command.
+CONTEXT:  while executing command on dblink connection named "myconn"
 -- reset remote transaction state
 SELECT dblink_exec('myconn','ABORT');
  dblink_exec 
@@ -573,7 +573,7 @@ FROM dblink_fetch('myconn','rmt_foo_cursor',4) AS t(a int, b text, c text[]);
 SELECT *
 FROM dblink_fetch('myconn','rmt_foobar_cursor',4,false) AS t(a int, b text, c text[]);
 NOTICE:  cursor "rmt_foobar_cursor" does not exist
-CONTEXT:  Error occurred on dblink connection named "myconn": could not fetch from cursor.
+CONTEXT:  while fetching from cursor "rmt_foobar_cursor" on dblink connection named "myconn"
  a | b | c 
 ---+---+---
 (0 rows)
@@ -589,7 +589,7 @@ SELECT dblink_exec('myconn','ABORT');
 SELECT *
 FROM dblink_fetch('myconn','rmt_foo_cursor',4) AS t(a int, b text, c text[]);
 ERROR:  cursor "rmt_foo_cursor" does not exist
-CONTEXT:  Error occurred on dblink connection named "myconn": could not fetch from cursor.
+CONTEXT:  while fetching from cursor "rmt_foo_cursor" on dblink connection named "myconn"
 -- close the named persistent connection
 SELECT dblink_disconnect('myconn');
  dblink_disconnect 

contrib/file_fdw/file_fdw.c

@@ -277,7 +277,7 @@ file_fdw_validator(PG_FUNCTION_ARGS)
 				ereport(ERROR,
 						(errcode(ERRCODE_SYNTAX_ERROR),
 						 errmsg("conflicting or redundant options"),
-						 errhint("option \"force_not_null\" supplied more than once for a column")));
+						 errhint("Option \"force_not_null\" supplied more than once for a column.")));
 			force_not_null = def;
 			/* Don't care what the value is, as long as it's a legal boolean */
 			(void) defGetBoolean(def);
@@ -289,7 +289,7 @@ file_fdw_validator(PG_FUNCTION_ARGS)
 				ereport(ERROR,
 						(errcode(ERRCODE_SYNTAX_ERROR),
 						 errmsg("conflicting or redundant options"),
-						 errhint("option \"force_null\" supplied more than once for a column")));
+						 errhint("Option \"force_null\" supplied more than once for a column.")));
 			force_null = def;
 			(void) defGetBoolean(def);
 		}

contrib/pgcrypto/px.c

@@ -105,8 +105,8 @@ px_THROW_ERROR(int err)
 		ereport(ERROR,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 errmsg("generating random data is not supported by this build"),
-				 errdetail("This functionality requires a source of strong random numbers"),
-				 errhint("You need to rebuild PostgreSQL using --enable-strong-random")));
+				 errdetail("This functionality requires a source of strong random numbers."),
+				 errhint("You need to rebuild PostgreSQL using --enable-strong-random.")));
 #endif
 	}
 	else

contrib/postgres_fdw/connection.c

@@ -630,7 +630,7 @@ pgfdw_report_error(int elevel, PGresult *res, PGconn *conn,
 				 message_detail ? errdetail_internal("%s", message_detail) : 0,
 				 message_hint ? errhint("%s", message_hint) : 0,
 				 message_context ? errcontext("%s", message_context) : 0,
-				 sql ? errcontext("Remote SQL command: %s", sql) : 0));
+				 sql ? errcontext("remote SQL command: %s", sql) : 0));
 	}
 	PG_CATCH();
 	{

contrib/postgres_fdw/expected/postgres_fdw.out

@@ -4126,7 +4126,7 @@ FETCH c;
 SAVEPOINT s;
 SELECT * FROM ft1 WHERE 1 / (c1 - 1) > 0;  -- ERROR
 ERROR:  division by zero
-CONTEXT:  Remote SQL command: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE (((1 / ("C 1" - 1)) > 0))
+CONTEXT:  remote SQL command: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE (((1 / ("C 1" - 1)) > 0))
 ROLLBACK TO s;
 FETCH c;
  c1 | c2 |  c3   |              c4              |            c5            | c6 |     c7     | c8  
@@ -5737,7 +5737,7 @@ ALTER TABLE "S 1"."T 1" ADD CONSTRAINT c2positive CHECK (c2 >= 0);
 INSERT INTO ft1(c1, c2) VALUES(11, 12);  -- duplicate key
 ERROR:  duplicate key value violates unique constraint "t1_pkey"
 DETAIL:  Key ("C 1")=(11) already exists.
-CONTEXT:  Remote SQL command: INSERT INTO "S 1"."T 1"("C 1", c2, c3, c4, c5, c6, c7, c8) VALUES ($1, $2, $3, $4, $5, $6, $7, $8)
+CONTEXT:  remote SQL command: INSERT INTO "S 1"."T 1"("C 1", c2, c3, c4, c5, c6, c7, c8) VALUES ($1, $2, $3, $4, $5, $6, $7, $8)
 INSERT INTO ft1(c1, c2) VALUES(11, 12) ON CONFLICT DO NOTHING; -- works
 INSERT INTO ft1(c1, c2) VALUES(11, 12) ON CONFLICT (c1, c2) DO NOTHING; -- unsupported
 ERROR:  there is no unique or exclusion constraint matching the ON CONFLICT specification
@@ -5746,11 +5746,11 @@ ERROR:  there is no unique or exclusion constraint matching the ON CONFLICT spec
 INSERT INTO ft1(c1, c2) VALUES(1111, -2);  -- c2positive
 ERROR:  new row for relation "T 1" violates check constraint "c2positive"
 DETAIL:  Failing row contains (1111, -2, null, null, null, null, ft1       , null).
-CONTEXT:  Remote SQL command: INSERT INTO "S 1"."T 1"("C 1", c2, c3, c4, c5, c6, c7, c8) VALUES ($1, $2, $3, $4, $5, $6, $7, $8)
+CONTEXT:  remote SQL command: INSERT INTO "S 1"."T 1"("C 1", c2, c3, c4, c5, c6, c7, c8) VALUES ($1, $2, $3, $4, $5, $6, $7, $8)
 UPDATE ft1 SET c2 = -c2 WHERE c1 = 1;  -- c2positive
 ERROR:  new row for relation "T 1" violates check constraint "c2positive"
 DETAIL:  Failing row contains (1, -1, 00001_trig_update, 1970-01-02 08:00:00+00, 1970-01-02 00:00:00, 1, 1         , foo).
-CONTEXT:  Remote SQL command: UPDATE "S 1"."T 1" SET c2 = (- c2) WHERE (("C 1" = 1))
+CONTEXT:  remote SQL command: UPDATE "S 1"."T 1" SET c2 = (- c2) WHERE (("C 1" = 1))
 -- Test savepoint/rollback behavior
 select c2, count(*) from ft2 where c2 < 500 group by 1 order by 1;
  c2  | count 
@@ -5909,7 +5909,7 @@ savepoint s3;
 update ft2 set c2 = -2 where c2 = 42 and c1 = 10; -- fail on remote side
 ERROR:  new row for relation "T 1" violates check constraint "c2positive"
 DETAIL:  Failing row contains (10, -2, 00010_trig_update_trig_update, 1970-01-11 08:00:00+00, 1970-01-11 00:00:00, 0, 0         , foo).
-CONTEXT:  Remote SQL command: UPDATE "S 1"."T 1" SET c2 = (-2) WHERE ((c2 = 42)) AND (("C 1" = 10))
+CONTEXT:  remote SQL command: UPDATE "S 1"."T 1" SET c2 = (-2) WHERE ((c2 = 42)) AND (("C 1" = 10))
 rollback to savepoint s3;
 select c2, count(*) from ft2 where c2 < 500 group by 1 order by 1;
  c2  | count 
@@ -6126,11 +6126,11 @@ RESET constraint_exclusion;
 INSERT INTO ft1(c1, c2) VALUES(1111, -2);  -- c2positive
 ERROR:  new row for relation "T 1" violates check constraint "c2positive"
 DETAIL:  Failing row contains (1111, -2, null, null, null, null, ft1       , null).
-CONTEXT:  Remote SQL command: INSERT INTO "S 1"."T 1"("C 1", c2, c3, c4, c5, c6, c7, c8) VALUES ($1, $2, $3, $4, $5, $6, $7, $8)
+CONTEXT:  remote SQL command: INSERT INTO "S 1"."T 1"("C 1", c2, c3, c4, c5, c6, c7, c8) VALUES ($1, $2, $3, $4, $5, $6, $7, $8)
 UPDATE ft1 SET c2 = -c2 WHERE c1 = 1;  -- c2positive
 ERROR:  new row for relation "T 1" violates check constraint "c2positive"
 DETAIL:  Failing row contains (1, -1, 00001_trig_update, 1970-01-02 08:00:00+00, 1970-01-02 00:00:00, 1, 1         , foo).
-CONTEXT:  Remote SQL command: UPDATE "S 1"."T 1" SET c2 = (- c2) WHERE (("C 1" = 1))
+CONTEXT:  remote SQL command: UPDATE "S 1"."T 1" SET c2 = (- c2) WHERE (("C 1" = 1))
 ALTER FOREIGN TABLE ft1 DROP CONSTRAINT ft1_c2positive;
 -- But inconsistent check constraints provide inconsistent results
 ALTER FOREIGN TABLE ft1 ADD CONSTRAINT ft1_c2negative CHECK (c2 < 0);

contrib/postgres_fdw/option.c

@@ -196,7 +196,7 @@ InitPgFdwOptions(void)
 		ereport(ERROR,
 				(errcode(ERRCODE_FDW_OUT_OF_MEMORY),
 				 errmsg("out of memory"),
-				 errdetail("could not get libpq's default connection options")));
+				 errdetail("Could not get libpq's default connection options.")));
 
 	/* Count how many libpq options are available. */
 	num_libpq_opts = 0;

src/backend/access/transam/xlog.c

@@ -9328,7 +9328,7 @@ CreateRestartPoint(int flags)
 	ereport((log_checkpoints ? LOG : DEBUG2),
 			(errmsg("recovery restart point at %X/%X",
 					(uint32) (lastCheckPoint.redo >> 32), (uint32) lastCheckPoint.redo),
-			 xtime ? errdetail("last completed transaction was at log time %s",
+			 xtime ? errdetail("Last completed transaction was at log time %s.",
 							   timestamptz_to_str(xtime)) : 0));
 
 	LWLockRelease(CheckpointLock);

src/backend/replication/logical/logical.c

@@ -417,7 +417,7 @@ CreateDecodingContext(XLogRecPtr start_lsn,
 	ereport(LOG,
 			(errmsg("starting logical decoding for slot \"%s\"",
 					NameStr(slot->data.name)),
-			 errdetail("streaming transactions committing after %X/%X, reading WAL from %X/%X",
+			 errdetail("Streaming transactions committing after %X/%X, reading WAL from %X/%X.",
 					   (uint32) (slot->data.confirmed_flush >> 32),
 					   (uint32) slot->data.confirmed_flush,
 					   (uint32) (slot->data.restart_lsn >> 32),