Thread: Remove trailing newlines from pg_upgrade's messages

Remove trailing newlines from pg_upgrade's messages

From
Tom Lane
Date:
Per the discussion at [1], pg_upgrade currently doesn't use
common/logging.c's functions.   Making it do so looks like a
bigger lift than is justified, but there is one particular
inconsistency that I think we ought to remove: pg_upgrade
expects (most) message strings to end in newlines, while logging.c
expects them not to.  This is bad for a couple of reasons:

* Translatable strings that otherwise could be shared with other
code are different.

* Developers might mistakenly add or leave off a newline because of
familiarity with how it's done elsewhere.  This is especially bad for
pg_fatal() which is otherwise caller-compatible with the version
provided by logging.c.  We fixed a couple of bugs of exactly that
description recently, and I found a few more as I went through
pg_upgrade for the attached patch.  It doesn't help any that as it
stands, pg_upgrade requires some messages to end in newline and
others not: there are some places that are adding an extra newline,
apparently because whoever coded them was confused about which
convention applied.

Hence, the patch below removes trailing newlines from all of
pg_upgrade's message strings, and teaches its logging infrastructure
to print them where appropriate.  As in logging.c, there's now an
Assert that no format string passed to pg_log() et al ends with
a newline.

This doesn't quite exactly match the code's prior behavior.  Aside
from the buggy-looking newlines mentioned above, there are a few
messages that formerly ended with a double newline, thus intentionally
producing a blank line, and now they don't.  I could have removed just
one of their newlines, but I'd have had to give up the Assert about
it, and I did not think that the extra blank lines were important
enough to justify that.

BTW, as I went through the code I realized just how badly pg_upgrade
needs a visit from the message style police.  Its messages are not
even consistent with each other, let alone with our message style
guidelines.  I have refrained (mostly) from doing any re-wording
here, but it could stand to be done.

I'll stick this in the CF queue, but I wonder if there is any case
for squeezing it into v15 instead of waiting for v16.

            regards, tom lane

[1] https://www.postgresql.org/message-id/4036037.1655174501%40sss.pgh.pa.us

diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index ace7387eda..89a2135c7b 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -69,13 +69,13 @@ output_check_banner(bool live_check)
     {
         pg_log(PG_REPORT,
                "Performing Consistency Checks on Old Live Server\n"
-               "------------------------------------------------\n");
+               "------------------------------------------------");
     }
     else
     {
         pg_log(PG_REPORT,
                "Performing Consistency Checks\n"
-               "-----------------------------\n");
+               "-----------------------------");
     }
 }

@@ -207,7 +207,7 @@ report_clusters_compatible(void)
 {
     if (user_opts.check)
     {
-        pg_log(PG_REPORT, "\n*Clusters are compatible*\n");
+        pg_log(PG_REPORT, "\n*Clusters are compatible*");
         /* stops new cluster */
         stop_postmaster(false);

@@ -217,7 +217,7 @@ report_clusters_compatible(void)

     pg_log(PG_REPORT, "\n"
            "If pg_upgrade fails after this point, you must re-initdb the\n"
-           "new cluster before continuing.\n");
+           "new cluster before continuing.");
 }


@@ -258,19 +258,19 @@ output_completion_banner(char *deletion_script_file_name)
     pg_log(PG_REPORT,
            "Optimizer statistics are not transferred by pg_upgrade.\n"
            "Once you start the new server, consider running:\n"
-           "    %s/vacuumdb %s--all --analyze-in-stages\n\n", new_cluster.bindir, user_specification.data);
+           "    %s/vacuumdb %s--all --analyze-in-stages", new_cluster.bindir, user_specification.data);

     if (deletion_script_file_name)
         pg_log(PG_REPORT,
                "Running this script will delete the old cluster's data files:\n"
-               "    %s\n",
+               "    %s",
                deletion_script_file_name);
     else
         pg_log(PG_REPORT,
                "Could not create a script to delete the old cluster's data files\n"
                "because user-defined tablespaces or the new cluster's data directory\n"
                "exist in the old cluster directory.  The old cluster's contents must\n"
-               "be deleted manually.\n");
+               "be deleted manually.");

     termPQExpBuffer(&user_specification);
 }
@@ -291,12 +291,12 @@ check_cluster_versions(void)
      */

     if (GET_MAJOR_VERSION(old_cluster.major_version) < 902)
-        pg_fatal("This utility can only upgrade from PostgreSQL version %s and later.\n",
+        pg_fatal("This utility can only upgrade from PostgreSQL version %s and later.",
                  "9.2");

     /* Only current PG version is supported as a target */
     if (GET_MAJOR_VERSION(new_cluster.major_version) != GET_MAJOR_VERSION(PG_VERSION_NUM))
-        pg_fatal("This utility can only upgrade to PostgreSQL version %s.\n",
+        pg_fatal("This utility can only upgrade to PostgreSQL version %s.",
                  PG_MAJORVERSION);

     /*
@@ -305,15 +305,15 @@ check_cluster_versions(void)
      * older versions.
      */
     if (old_cluster.major_version > new_cluster.major_version)
-        pg_fatal("This utility cannot be used to downgrade to older major PostgreSQL versions.\n");
+        pg_fatal("This utility cannot be used to downgrade to older major PostgreSQL versions.");

     /* Ensure binaries match the designated data directories */
     if (GET_MAJOR_VERSION(old_cluster.major_version) !=
         GET_MAJOR_VERSION(old_cluster.bin_version))
-        pg_fatal("Old cluster data and binary directories are from different major versions.\n");
+        pg_fatal("Old cluster data and binary directories are from different major versions.");
     if (GET_MAJOR_VERSION(new_cluster.major_version) !=
         GET_MAJOR_VERSION(new_cluster.bin_version))
-        pg_fatal("New cluster data and binary directories are from different major versions.\n");
+        pg_fatal("New cluster data and binary directories are from different major versions.");

     check_ok();
 }
@@ -329,7 +329,7 @@ check_cluster_compatibility(bool live_check)

     if (live_check && old_cluster.port == new_cluster.port)
         pg_fatal("When checking a live server, "
-                 "the old and new port numbers must be different.\n");
+                 "the old and new port numbers must be different.");
 }


@@ -343,25 +343,25 @@ static void
 check_locale_and_encoding(DbInfo *olddb, DbInfo *newdb)
 {
     if (olddb->db_encoding != newdb->db_encoding)
-        pg_fatal("encodings for database \"%s\" do not match:  old \"%s\", new \"%s\"\n",
+        pg_fatal("encodings for database \"%s\" do not match:  old \"%s\", new \"%s\"",
                  olddb->db_name,
                  pg_encoding_to_char(olddb->db_encoding),
                  pg_encoding_to_char(newdb->db_encoding));
     if (!equivalent_locale(LC_COLLATE, olddb->db_collate, newdb->db_collate))
-        pg_fatal("lc_collate values for database \"%s\" do not match:  old \"%s\", new \"%s\"\n",
+        pg_fatal("lc_collate values for database \"%s\" do not match:  old \"%s\", new \"%s\"",
                  olddb->db_name, olddb->db_collate, newdb->db_collate);
     if (!equivalent_locale(LC_CTYPE, olddb->db_ctype, newdb->db_ctype))
-        pg_fatal("lc_ctype values for database \"%s\" do not match:  old \"%s\", new \"%s\"\n",
+        pg_fatal("lc_ctype values for database \"%s\" do not match:  old \"%s\", new \"%s\"",
                  olddb->db_name, olddb->db_ctype, newdb->db_ctype);
     if (olddb->db_collprovider != newdb->db_collprovider)
-        pg_fatal("locale providers for database \"%s\" do not match:  old \"%s\", new \"%s\"\n",
+        pg_fatal("locale providers for database \"%s\" do not match:  old \"%s\", new \"%s\"",
                  olddb->db_name,
                  collprovider_name(olddb->db_collprovider),
                  collprovider_name(newdb->db_collprovider));
     if ((olddb->db_iculocale == NULL && newdb->db_iculocale != NULL) ||
         (olddb->db_iculocale != NULL && newdb->db_iculocale == NULL) ||
         (olddb->db_iculocale != NULL && newdb->db_iculocale != NULL && strcmp(olddb->db_iculocale,
newdb->db_iculocale)!= 0)) 
-        pg_fatal("ICU locale values for database \"%s\" do not match:  old \"%s\", new \"%s\"\n",
+        pg_fatal("ICU locale values for database \"%s\" do not match:  old \"%s\", new \"%s\"",
                  olddb->db_name,
                  olddb->db_iculocale ? olddb->db_iculocale : "(null)",
                  newdb->db_iculocale ? newdb->db_iculocale : "(null)");
@@ -436,7 +436,7 @@ check_new_cluster_is_empty(void)
         {
             /* pg_largeobject and its index should be skipped */
             if (strcmp(rel_arr->rels[relnum].nspname, "pg_catalog") != 0)
-                pg_fatal("New cluster database \"%s\" is not empty: found relation \"%s.%s\"\n",
+                pg_fatal("New cluster database \"%s\" is not empty: found relation \"%s.%s\"",
                          new_cluster.dbarr.dbs[dbnum].db_name,
                          rel_arr->rels[relnum].nspname,
                          rel_arr->rels[relnum].relname);
@@ -499,7 +499,7 @@ check_for_new_tablespace_dir(ClusterInfo *new_cluster)
                  new_cluster->tablespace_suffix);

         if (stat(new_tablespace_dir, &statbuf) == 0 || errno != ENOENT)
-            pg_fatal("new cluster tablespace directory already exists: \"%s\"\n",
+            pg_fatal("new cluster tablespace directory already exists: \"%s\"",
                      new_tablespace_dir);
     }

@@ -532,7 +532,7 @@ create_script_for_old_cluster_deletion(char **deletion_script_file_name)
     if (path_is_prefix_of_path(old_cluster_pgdata, new_cluster_pgdata))
     {
         pg_log(PG_WARNING,
-               "\nWARNING:  new data directory should not be inside the old data directory, e.g. %s\n",
old_cluster_pgdata);
+               "\nWARNING:  new data directory should not be inside the old data directory, e.g. %s",
old_cluster_pgdata);

         /* Unlink file in case it is left over from a previous run. */
         unlink(*deletion_script_file_name);
@@ -556,7 +556,7 @@ create_script_for_old_cluster_deletion(char **deletion_script_file_name)
         {
             /* reproduce warning from CREATE TABLESPACE that is in the log */
             pg_log(PG_WARNING,
-                   "\nWARNING:  user-defined tablespace locations should not be inside the data directory, e.g. %s\n",
old_tablespace_dir);
+                   "\nWARNING:  user-defined tablespace locations should not be inside the data directory, e.g. %s",
old_tablespace_dir);

             /* Unlink file in case it is left over from a previous run. */
             unlink(*deletion_script_file_name);
@@ -569,7 +569,7 @@ create_script_for_old_cluster_deletion(char **deletion_script_file_name)
     prep_status("Creating script to delete old cluster");

     if ((script = fopen_priv(*deletion_script_file_name, "w")) == NULL)
-        pg_fatal("could not open file \"%s\": %s\n",
+        pg_fatal("could not open file \"%s\": %s",
                  *deletion_script_file_name, strerror(errno));

 #ifndef WIN32
@@ -620,7 +620,7 @@ create_script_for_old_cluster_deletion(char **deletion_script_file_name)

 #ifndef WIN32
     if (chmod(*deletion_script_file_name, S_IRWXU) != 0)
-        pg_fatal("could not add execute permission to file \"%s\": %s\n",
+        pg_fatal("could not add execute permission to file \"%s\": %s",
                  *deletion_script_file_name, strerror(errno));
 #endif

@@ -656,7 +656,7 @@ check_is_install_user(ClusterInfo *cluster)
      */
     if (PQntuples(res) != 1 ||
         atooid(PQgetvalue(res, 0, 1)) != BOOTSTRAP_SUPERUSERID)
-        pg_fatal("database user \"%s\" is not the install user\n",
+        pg_fatal("database user \"%s\" is not the install user",
                  os_info.user);

     PQclear(res);
@@ -667,7 +667,7 @@ check_is_install_user(ClusterInfo *cluster)
                             "WHERE rolname !~ '^pg_'");

     if (PQntuples(res) != 1)
-        pg_fatal("could not determine the number of users\n");
+        pg_fatal("could not determine the number of users");

     /*
      * We only allow the install user in the new cluster because other defined
@@ -675,7 +675,7 @@ check_is_install_user(ClusterInfo *cluster)
      * error during pg_dump restore.
      */
     if (cluster == &new_cluster && atooid(PQgetvalue(res, 0, 0)) != 1)
-        pg_fatal("Only the install user can be defined in the new cluster.\n");
+        pg_fatal("Only the install user can be defined in the new cluster.");

     PQclear(res);

@@ -732,7 +732,7 @@ check_proper_datallowconn(ClusterInfo *cluster)
             /* avoid restore failure when pg_dumpall tries to create template0 */
             if (strcmp(datallowconn, "t") == 0)
                 pg_fatal("template0 must not allow connections, "
-                         "i.e. its pg_database.datallowconn must be false\n");
+                         "i.e. its pg_database.datallowconn must be false");
         }
         else
         {
@@ -744,7 +744,7 @@ check_proper_datallowconn(ClusterInfo *cluster)
             {
                 found = true;
                 if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
-                    pg_fatal("could not open file \"%s\": %s\n",
+                    pg_fatal("could not open file \"%s\": %s",
                              output_path, strerror(errno));

                 fprintf(script, "%s\n", datname);
@@ -761,14 +761,14 @@ check_proper_datallowconn(ClusterInfo *cluster)

     if (found)
     {
-        pg_log(PG_REPORT, "fatal\n");
+        pg_log(PG_REPORT, "fatal");
         pg_fatal("All non-template0 databases must allow connections, i.e. their\n"
                  "pg_database.datallowconn must be true.  Your installation contains\n"
                  "non-template0 databases with their pg_database.datallowconn set to\n"
                  "false.  Consider allowing connection for all non-template0 databases\n"
                  "or drop the databases which do not allow connections.  A list of\n"
                  "databases with the problem is in the file:\n"
-                 "    %s\n\n", output_path);
+                 "    %s", output_path);
     }
     else
         check_ok();
@@ -796,9 +796,9 @@ check_for_prepared_transactions(ClusterInfo *cluster)
     if (PQntuples(res) != 0)
     {
         if (cluster == &old_cluster)
-            pg_fatal("The source cluster contains prepared transactions\n");
+            pg_fatal("The source cluster contains prepared transactions");
         else
-            pg_fatal("The target cluster contains prepared transactions\n");
+            pg_fatal("The target cluster contains prepared transactions");
     }

     PQclear(res);
@@ -864,7 +864,7 @@ check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster)
         {
             found = true;
             if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
-                pg_fatal("could not open file \"%s\": %s\n",
+                pg_fatal("could not open file \"%s\": %s",
                          output_path, strerror(errno));
             if (!db_used)
             {
@@ -886,14 +886,14 @@ check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster)

     if (found)
     {
-        pg_log(PG_REPORT, "fatal\n");
+        pg_log(PG_REPORT, "fatal");
         pg_fatal("Your installation contains \"contrib/isn\" functions which rely on the\n"
                  "bigint data type.  Your old and new clusters pass bigint values\n"
                  "differently so this cluster cannot currently be upgraded.  You can\n"
                  "manually dump databases in the old cluster that use \"contrib/isn\"\n"
                  "facilities, drop them, perform the upgrade, and then restore them.  A\n"
                  "list of the problem functions is in the file:\n"
-                 "    %s\n\n", output_path);
+                 "    %s", output_path);
     }
     else
         check_ok();
@@ -963,7 +963,7 @@ check_for_user_defined_postfix_ops(ClusterInfo *cluster)
             found = true;
             if (script == NULL &&
                 (script = fopen_priv(output_path, "w")) == NULL)
-                pg_fatal("could not open file \"%s\": %s\n",
+                pg_fatal("could not open file \"%s\": %s",
                          output_path, strerror(errno));
             if (!db_used)
             {
@@ -988,12 +988,12 @@ check_for_user_defined_postfix_ops(ClusterInfo *cluster)

     if (found)
     {
-        pg_log(PG_REPORT, "fatal\n");
+        pg_log(PG_REPORT, "fatal");
         pg_fatal("Your installation contains user-defined postfix operators, which are not\n"
                  "supported anymore.  Consider dropping the postfix operators and replacing\n"
                  "them with prefix operators or function calls.\n"
                  "A list of user-defined postfix operators is in the file:\n"
-                 "    %s\n\n", output_path);
+                 "    %s", output_path);
     }
     else
         check_ok();
@@ -1043,7 +1043,7 @@ check_for_tables_with_oids(ClusterInfo *cluster)
         {
             found = true;
             if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
-                pg_fatal("could not open file \"%s\": %s\n",
+                pg_fatal("could not open file \"%s\": %s",
                          output_path, strerror(errno));
             if (!db_used)
             {
@@ -1065,12 +1065,12 @@ check_for_tables_with_oids(ClusterInfo *cluster)

     if (found)
     {
-        pg_log(PG_REPORT, "fatal\n");
+        pg_log(PG_REPORT, "fatal");
         pg_fatal("Your installation contains tables declared WITH OIDS, which is not\n"
                  "supported anymore.  Consider removing the oid column using\n"
                  "    ALTER TABLE ... SET WITHOUT OIDS;\n"
                  "A list of tables with the problem is in the file:\n"
-                 "    %s\n\n", output_path);
+                 "    %s", output_path);
     }
     else
         check_ok();
@@ -1122,13 +1122,13 @@ check_for_composite_data_type_usage(ClusterInfo *cluster)

     if (found)
     {
-        pg_log(PG_REPORT, "fatal\n");
+        pg_log(PG_REPORT, "fatal");
         pg_fatal("Your installation contains system-defined composite type(s) in user tables.\n"
                  "These type OIDs are not stable across PostgreSQL versions,\n"
                  "so this cluster cannot currently be upgraded.  You can\n"
                  "drop the problem columns and restart the upgrade.\n"
                  "A list of the problem columns is in the file:\n"
-                 "    %s\n\n", output_path);
+                 "    %s", output_path);
     }
     else
         check_ok();
@@ -1181,13 +1181,13 @@ check_for_reg_data_type_usage(ClusterInfo *cluster)

     if (found)
     {
-        pg_log(PG_REPORT, "fatal\n");
+        pg_log(PG_REPORT, "fatal");
         pg_fatal("Your installation contains one of the reg* data types in user tables.\n"
                  "These data types reference system OIDs that are not preserved by\n"
                  "pg_upgrade, so this cluster cannot currently be upgraded.  You can\n"
                  "drop the problem columns and restart the upgrade.\n"
                  "A list of the problem columns is in the file:\n"
-                 "    %s\n\n", output_path);
+                 "    %s", output_path);
     }
     else
         check_ok();
@@ -1210,13 +1210,13 @@ check_for_jsonb_9_4_usage(ClusterInfo *cluster)

     if (check_for_data_type_usage(cluster, "pg_catalog.jsonb", output_path))
     {
-        pg_log(PG_REPORT, "fatal\n");
+        pg_log(PG_REPORT, "fatal");
         pg_fatal("Your installation contains the \"jsonb\" data type in user tables.\n"
                  "The internal format of \"jsonb\" changed during 9.4 beta so this\n"
                  "cluster cannot currently be upgraded.  You can\n"
                  "drop the problem columns and restart the upgrade.\n"
                  "A list of the problem columns is in the file:\n"
-                 "    %s\n\n", output_path);
+                 "    %s", output_path);
     }
     else
         check_ok();
@@ -1243,9 +1243,9 @@ check_for_pg_role_prefix(ClusterInfo *cluster)
     if (PQntuples(res) != 0)
     {
         if (cluster == &old_cluster)
-            pg_fatal("The source cluster contains roles starting with \"pg_\"\n");
+            pg_fatal("The source cluster contains roles starting with \"pg_\"");
         else
-            pg_fatal("The target cluster contains roles starting with \"pg_\"\n");
+            pg_fatal("The target cluster contains roles starting with \"pg_\"");
     }

     PQclear(res);
@@ -1306,7 +1306,7 @@ check_for_user_defined_encoding_conversions(ClusterInfo *cluster)
             found = true;
             if (script == NULL &&
                 (script = fopen_priv(output_path, "w")) == NULL)
-                pg_fatal("could not open file \"%s\": %s\n",
+                pg_fatal("could not open file \"%s\": %s",
                          output_path, strerror(errno));
             if (!db_used)
             {
@@ -1329,13 +1329,13 @@ check_for_user_defined_encoding_conversions(ClusterInfo *cluster)

     if (found)
     {
-        pg_log(PG_REPORT, "fatal\n");
+        pg_log(PG_REPORT, "fatal");
         pg_fatal("Your installation contains user-defined encoding conversions.\n"
                  "The conversion function parameters changed in PostgreSQL version 14\n"
                  "so this cluster cannot currently be upgraded.  You can remove the\n"
                  "encoding conversions in the old cluster and restart the upgrade.\n"
                  "A list of user-defined encoding conversions is in the file:\n"
-                 "    %s\n\n", output_path);
+                 "    %s", output_path);
     }
     else
         check_ok();
@@ -1357,7 +1357,7 @@ get_canonical_locale_name(int category, const char *locale)
     /* get the current setting, so we can restore it. */
     save = setlocale(category, NULL);
     if (!save)
-        pg_fatal("failed to get the current locale\n");
+        pg_fatal("failed to get the current locale");

     /* 'save' may be pointing at a modifiable scratch variable, so copy it. */
     save = pg_strdup(save);
@@ -1366,13 +1366,13 @@ get_canonical_locale_name(int category, const char *locale)
     res = setlocale(category, locale);

     if (!res)
-        pg_fatal("failed to get system locale name for \"%s\"\n", locale);
+        pg_fatal("failed to get system locale name for \"%s\"", locale);

     res = pg_strdup(res);

     /* restore old value. */
     if (!setlocale(category, save))
-        pg_fatal("failed to restore old locale \"%s\"\n", save);
+        pg_fatal("failed to restore old locale \"%s\"", save);

     pg_free(save);

diff --git a/src/bin/pg_upgrade/controldata.c b/src/bin/pg_upgrade/controldata.c
index 41b8f69b8c..07de918358 100644
--- a/src/bin/pg_upgrade/controldata.c
+++ b/src/bin/pg_upgrade/controldata.c
@@ -12,6 +12,8 @@
 #include <ctype.h>

 #include "pg_upgrade.h"
+#include "common/string.h"
+

 /*
  * get_control_data()
@@ -125,7 +127,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
         fflush(stderr);

         if ((output = popen(cmd, "r")) == NULL)
-            pg_fatal("could not get control data using %s: %s\n",
+            pg_fatal("could not get control data using %s: %s",
                      cmd, strerror(errno));

         /* we have the result of cmd in "output". so parse it line by line now */
@@ -136,7 +138,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
                 p = strchr(p, ':');

                 if (p == NULL || strlen(p) <= 1)
-                    pg_fatal("%d: database cluster state problem\n", __LINE__);
+                    pg_fatal("%d: database cluster state problem", __LINE__);

                 p++;            /* remove ':' char */

@@ -154,16 +156,16 @@ get_control_data(ClusterInfo *cluster, bool live_check)
                 if (strcmp(p, "shut down in recovery\n") == 0)
                 {
                     if (cluster == &old_cluster)
-                        pg_fatal("The source cluster was shut down while in recovery mode.  To upgrade, use \"rsync\"
asdocumented or shut it down as a primary.\n"); 
+                        pg_fatal("The source cluster was shut down while in recovery mode.  To upgrade, use \"rsync\"
asdocumented or shut it down as a primary."); 
                     else
-                        pg_fatal("The target cluster was shut down while in recovery mode.  To upgrade, use \"rsync\"
asdocumented or shut it down as a primary.\n"); 
+                        pg_fatal("The target cluster was shut down while in recovery mode.  To upgrade, use \"rsync\"
asdocumented or shut it down as a primary."); 
                 }
                 else if (strcmp(p, "shut down\n") != 0)
                 {
                     if (cluster == &old_cluster)
-                        pg_fatal("The source cluster was not shut down cleanly.\n");
+                        pg_fatal("The source cluster was not shut down cleanly.");
                     else
-                        pg_fatal("The target cluster was not shut down cleanly.\n");
+                        pg_fatal("The target cluster was not shut down cleanly.");
                 }
                 got_cluster_state = true;
             }
@@ -174,9 +176,9 @@ get_control_data(ClusterInfo *cluster, bool live_check)
         if (!got_cluster_state)
         {
             if (cluster == &old_cluster)
-                pg_fatal("The source cluster lacks cluster state information:\n");
+                pg_fatal("The source cluster lacks cluster state information:");
             else
-                pg_fatal("The target cluster lacks cluster state information:\n");
+                pg_fatal("The target cluster lacks cluster state information:");
         }
     }

@@ -193,7 +195,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
     fflush(stderr);

     if ((output = popen(cmd, "r")) == NULL)
-        pg_fatal("could not get control data using %s: %s\n",
+        pg_fatal("could not get control data using %s: %s",
                  cmd, strerror(errno));

     /* Only in <= 9.2 */
@@ -206,6 +208,8 @@ get_control_data(ClusterInfo *cluster, bool live_check)
     /* we have the result of cmd in "output". so parse it line by line now */
     while (fgets(bufin, sizeof(bufin), output))
     {
+        /* In verbose mode, log each line */
+        pg_strip_crlf(bufin);
         pg_log(PG_VERBOSE, "%s", bufin);

         if ((p = strstr(bufin, "pg_control version number:")) != NULL)
@@ -213,7 +217,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
             p = strchr(p, ':');

             if (p == NULL || strlen(p) <= 1)
-                pg_fatal("%d: pg_resetwal problem\n", __LINE__);
+                pg_fatal("%d: pg_resetwal problem", __LINE__);

             p++;                /* remove ':' char */
             cluster->controldata.ctrl_ver = str2uint(p);
@@ -223,7 +227,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
             p = strchr(p, ':');

             if (p == NULL || strlen(p) <= 1)
-                pg_fatal("%d: controldata retrieval problem\n", __LINE__);
+                pg_fatal("%d: controldata retrieval problem", __LINE__);

             p++;                /* remove ':' char */
             cluster->controldata.cat_ver = str2uint(p);
@@ -233,7 +237,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
             p = strchr(p, ':');

             if (p == NULL || strlen(p) <= 1)
-                pg_fatal("%d: controldata retrieval problem\n", __LINE__);
+                pg_fatal("%d: controldata retrieval problem", __LINE__);

             p++;                /* remove ':' char */
             tli = str2uint(p);
@@ -244,7 +248,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
             p = strchr(p, ':');

             if (p == NULL || strlen(p) <= 1)
-                pg_fatal("%d: controldata retrieval problem\n", __LINE__);
+                pg_fatal("%d: controldata retrieval problem", __LINE__);

             p++;                /* remove ':' char */
             logid = str2uint(p);
@@ -255,7 +259,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
             p = strchr(p, ':');

             if (p == NULL || strlen(p) <= 1)
-                pg_fatal("%d: controldata retrieval problem\n", __LINE__);
+                pg_fatal("%d: controldata retrieval problem", __LINE__);

             p++;                /* remove ':' char */
             segno = str2uint(p);
@@ -266,7 +270,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
             p = strchr(p, ':');

             if (p == NULL || strlen(p) <= 1)
-                pg_fatal("%d: controldata retrieval problem\n", __LINE__);
+                pg_fatal("%d: controldata retrieval problem", __LINE__);

             p++;                /* remove ':' char */
             cluster->controldata.chkpnt_nxtepoch = str2uint(p);
@@ -285,7 +289,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
                 p = NULL;

             if (p == NULL || strlen(p) <= 1)
-                pg_fatal("%d: controldata retrieval problem\n", __LINE__);
+                pg_fatal("%d: controldata retrieval problem", __LINE__);

             p++;                /* remove '/' or ':' char */
             cluster->controldata.chkpnt_nxtxid = str2uint(p);
@@ -296,7 +300,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
             p = strchr(p, ':');

             if (p == NULL || strlen(p) <= 1)
-                pg_fatal("%d: controldata retrieval problem\n", __LINE__);
+                pg_fatal("%d: controldata retrieval problem", __LINE__);

             p++;                /* remove ':' char */
             cluster->controldata.chkpnt_nxtoid = str2uint(p);
@@ -307,7 +311,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
             p = strchr(p, ':');

             if (p == NULL || strlen(p) <= 1)
-                pg_fatal("%d: controldata retrieval problem\n", __LINE__);
+                pg_fatal("%d: controldata retrieval problem", __LINE__);

             p++;                /* remove ':' char */
             cluster->controldata.chkpnt_nxtmulti = str2uint(p);
@@ -318,7 +322,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
             p = strchr(p, ':');

             if (p == NULL || strlen(p) <= 1)
-                pg_fatal("%d: controldata retrieval problem\n", __LINE__);
+                pg_fatal("%d: controldata retrieval problem", __LINE__);

             p++;                /* remove ':' char */
             cluster->controldata.chkpnt_oldstxid = str2uint(p);
@@ -329,7 +333,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
             p = strchr(p, ':');

             if (p == NULL || strlen(p) <= 1)
-                pg_fatal("%d: controldata retrieval problem\n", __LINE__);
+                pg_fatal("%d: controldata retrieval problem", __LINE__);

             p++;                /* remove ':' char */
             cluster->controldata.chkpnt_oldstMulti = str2uint(p);
@@ -340,7 +344,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
             p = strchr(p, ':');

             if (p == NULL || strlen(p) <= 1)
-                pg_fatal("%d: controldata retrieval problem\n", __LINE__);
+                pg_fatal("%d: controldata retrieval problem", __LINE__);

             p++;                /* remove ':' char */
             cluster->controldata.chkpnt_nxtmxoff = str2uint(p);
@@ -351,14 +355,14 @@ get_control_data(ClusterInfo *cluster, bool live_check)
             /* Skip the colon and any whitespace after it */
             p = strchr(p, ':');
             if (p == NULL || strlen(p) <= 1)
-                pg_fatal("%d: controldata retrieval problem\n", __LINE__);
+                pg_fatal("%d: controldata retrieval problem", __LINE__);
             p = strpbrk(p, "01234567890ABCDEF");
             if (p == NULL || strlen(p) <= 1)
-                pg_fatal("%d: controldata retrieval problem\n", __LINE__);
+                pg_fatal("%d: controldata retrieval problem", __LINE__);

             /* Make sure it looks like a valid WAL file name */
             if (strspn(p, "0123456789ABCDEF") != 24)
-                pg_fatal("%d: controldata retrieval problem\n", __LINE__);
+                pg_fatal("%d: controldata retrieval problem", __LINE__);

             strlcpy(cluster->controldata.nextxlogfile, p, 25);
             got_nextxlogfile = true;
@@ -368,7 +372,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
             p = strchr(p, ':');

             if (p == NULL || strlen(p) <= 1)
-                pg_fatal("%d: controldata retrieval problem\n", __LINE__);
+                pg_fatal("%d: controldata retrieval problem", __LINE__);

             p++;                /* remove ':' char */
             /* used later for contrib check */
@@ -380,7 +384,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
             p = strchr(p, ':');

             if (p == NULL || strlen(p) <= 1)
-                pg_fatal("%d: controldata retrieval problem\n", __LINE__);
+                pg_fatal("%d: controldata retrieval problem", __LINE__);

             p++;                /* remove ':' char */
             cluster->controldata.align = str2uint(p);
@@ -391,7 +395,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
             p = strchr(p, ':');

             if (p == NULL || strlen(p) <= 1)
-                pg_fatal("%d: controldata retrieval problem\n", __LINE__);
+                pg_fatal("%d: controldata retrieval problem", __LINE__);

             p++;                /* remove ':' char */
             cluster->controldata.blocksz = str2uint(p);
@@ -402,7 +406,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
             p = strchr(p, ':');

             if (p == NULL || strlen(p) <= 1)
-                pg_fatal("%d: controldata retrieval problem\n", __LINE__);
+                pg_fatal("%d: controldata retrieval problem", __LINE__);

             p++;                /* remove ':' char */
             cluster->controldata.largesz = str2uint(p);
@@ -413,7 +417,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
             p = strchr(p, ':');

             if (p == NULL || strlen(p) <= 1)
-                pg_fatal("%d: controldata retrieval problem\n", __LINE__);
+                pg_fatal("%d: controldata retrieval problem", __LINE__);

             p++;                /* remove ':' char */
             cluster->controldata.walsz = str2uint(p);
@@ -424,7 +428,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
             p = strchr(p, ':');

             if (p == NULL || strlen(p) <= 1)
-                pg_fatal("%d: controldata retrieval problem\n", __LINE__);
+                pg_fatal("%d: controldata retrieval problem", __LINE__);

             p++;                /* remove ':' char */
             cluster->controldata.walseg = str2uint(p);
@@ -435,7 +439,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
             p = strchr(p, ':');

             if (p == NULL || strlen(p) <= 1)
-                pg_fatal("%d: controldata retrieval problem\n", __LINE__);
+                pg_fatal("%d: controldata retrieval problem", __LINE__);

             p++;                /* remove ':' char */
             cluster->controldata.ident = str2uint(p);
@@ -446,7 +450,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
             p = strchr(p, ':');

             if (p == NULL || strlen(p) <= 1)
-                pg_fatal("%d: controldata retrieval problem\n", __LINE__);
+                pg_fatal("%d: controldata retrieval problem", __LINE__);

             p++;                /* remove ':' char */
             cluster->controldata.index = str2uint(p);
@@ -457,7 +461,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
             p = strchr(p, ':');

             if (p == NULL || strlen(p) <= 1)
-                pg_fatal("%d: controldata retrieval problem\n", __LINE__);
+                pg_fatal("%d: controldata retrieval problem", __LINE__);

             p++;                /* remove ':' char */
             cluster->controldata.toast = str2uint(p);
@@ -468,7 +472,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
             p = strchr(p, ':');

             if (p == NULL || strlen(p) <= 1)
-                pg_fatal("%d: controldata retrieval problem\n", __LINE__);
+                pg_fatal("%d: controldata retrieval problem", __LINE__);

             p++;                /* remove ':' char */
             cluster->controldata.large_object = str2uint(p);
@@ -479,7 +483,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
             p = strchr(p, ':');

             if (p == NULL || strlen(p) <= 1)
-                pg_fatal("%d: controldata retrieval problem\n", __LINE__);
+                pg_fatal("%d: controldata retrieval problem", __LINE__);

             p++;                /* remove ':' char */
             cluster->controldata.date_is_int = strstr(p, "64-bit integers") != NULL;
@@ -490,7 +494,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
             p = strchr(p, ':');

             if (p == NULL || strlen(p) <= 1)
-                pg_fatal("%d: controldata retrieval problem\n", __LINE__);
+                pg_fatal("%d: controldata retrieval problem", __LINE__);

             p++;                /* remove ':' char */
             /* used later for contrib check */
@@ -569,72 +573,72 @@ get_control_data(ClusterInfo *cluster, bool live_check)
     {
         if (cluster == &old_cluster)
             pg_log(PG_REPORT,
-                   "The source cluster lacks some required control information:\n");
+                   "The source cluster lacks some required control information:");
         else
             pg_log(PG_REPORT,
-                   "The target cluster lacks some required control information:\n");
+                   "The target cluster lacks some required control information:");

         if (!got_xid)
-            pg_log(PG_REPORT, "  checkpoint next XID\n");
+            pg_log(PG_REPORT, "  checkpoint next XID");

         if (!got_oid)
-            pg_log(PG_REPORT, "  latest checkpoint next OID\n");
+            pg_log(PG_REPORT, "  latest checkpoint next OID");

         if (!got_multi)
-            pg_log(PG_REPORT, "  latest checkpoint next MultiXactId\n");
+            pg_log(PG_REPORT, "  latest checkpoint next MultiXactId");

         if (!got_oldestmulti &&
             cluster->controldata.cat_ver >= MULTIXACT_FORMATCHANGE_CAT_VER)
-            pg_log(PG_REPORT, "  latest checkpoint oldest MultiXactId\n");
+            pg_log(PG_REPORT, "  latest checkpoint oldest MultiXactId");

         if (!got_oldestxid)
-            pg_log(PG_REPORT, "  latest checkpoint oldestXID\n");
+            pg_log(PG_REPORT, "  latest checkpoint oldestXID");

         if (!got_mxoff)
-            pg_log(PG_REPORT, "  latest checkpoint next MultiXactOffset\n");
+            pg_log(PG_REPORT, "  latest checkpoint next MultiXactOffset");

         if (!live_check && !got_nextxlogfile)
-            pg_log(PG_REPORT, "  first WAL segment after reset\n");
+            pg_log(PG_REPORT, "  first WAL segment after reset");

         if (!got_float8_pass_by_value)
-            pg_log(PG_REPORT, "  float8 argument passing method\n");
+            pg_log(PG_REPORT, "  float8 argument passing method");

         if (!got_align)
-            pg_log(PG_REPORT, "  maximum alignment\n");
+            pg_log(PG_REPORT, "  maximum alignment");

         if (!got_blocksz)
-            pg_log(PG_REPORT, "  block size\n");
+            pg_log(PG_REPORT, "  block size");

         if (!got_largesz)
-            pg_log(PG_REPORT, "  large relation segment size\n");
+            pg_log(PG_REPORT, "  large relation segment size");

         if (!got_walsz)
-            pg_log(PG_REPORT, "  WAL block size\n");
+            pg_log(PG_REPORT, "  WAL block size");

         if (!got_walseg)
-            pg_log(PG_REPORT, "  WAL segment size\n");
+            pg_log(PG_REPORT, "  WAL segment size");

         if (!got_ident)
-            pg_log(PG_REPORT, "  maximum identifier length\n");
+            pg_log(PG_REPORT, "  maximum identifier length");

         if (!got_index)
-            pg_log(PG_REPORT, "  maximum number of indexed columns\n");
+            pg_log(PG_REPORT, "  maximum number of indexed columns");

         if (!got_toast)
-            pg_log(PG_REPORT, "  maximum TOAST chunk size\n");
+            pg_log(PG_REPORT, "  maximum TOAST chunk size");

         if (!got_large_object &&
             cluster->controldata.ctrl_ver >= LARGE_OBJECT_SIZE_PG_CONTROL_VER)
-            pg_log(PG_REPORT, "  large-object chunk size\n");
+            pg_log(PG_REPORT, "  large-object chunk size");

         if (!got_date_is_int)
-            pg_log(PG_REPORT, "  dates/times are integers?\n");
+            pg_log(PG_REPORT, "  dates/times are integers?");

         /* value added in Postgres 9.3 */
         if (!got_data_checksum_version)
-            pg_log(PG_REPORT, "  data checksum version\n");
+            pg_log(PG_REPORT, "  data checksum version");

-        pg_fatal("Cannot continue without required control information, terminating\n");
+        pg_fatal("Cannot continue without required control information, terminating");
     }
 }

@@ -649,37 +653,37 @@ check_control_data(ControlData *oldctrl,
                    ControlData *newctrl)
 {
     if (oldctrl->align == 0 || oldctrl->align != newctrl->align)
-        pg_fatal("old and new pg_controldata alignments are invalid or do not match\n"
-                 "Likely one cluster is a 32-bit install, the other 64-bit\n");
+        pg_fatal("old and new pg_controldata alignments are invalid or do not match.\n"
+                 "Likely one cluster is a 32-bit install, the other 64-bit");

     if (oldctrl->blocksz == 0 || oldctrl->blocksz != newctrl->blocksz)
-        pg_fatal("old and new pg_controldata block sizes are invalid or do not match\n");
+        pg_fatal("old and new pg_controldata block sizes are invalid or do not match");

     if (oldctrl->largesz == 0 || oldctrl->largesz != newctrl->largesz)
-        pg_fatal("old and new pg_controldata maximum relation segment sizes are invalid or do not match\n");
+        pg_fatal("old and new pg_controldata maximum relation segment sizes are invalid or do not match");

     if (oldctrl->walsz == 0 || oldctrl->walsz != newctrl->walsz)
-        pg_fatal("old and new pg_controldata WAL block sizes are invalid or do not match\n");
+        pg_fatal("old and new pg_controldata WAL block sizes are invalid or do not match");

     if (oldctrl->walseg == 0 || oldctrl->walseg != newctrl->walseg)
-        pg_fatal("old and new pg_controldata WAL segment sizes are invalid or do not match\n");
+        pg_fatal("old and new pg_controldata WAL segment sizes are invalid or do not match");

     if (oldctrl->ident == 0 || oldctrl->ident != newctrl->ident)
-        pg_fatal("old and new pg_controldata maximum identifier lengths are invalid or do not match\n");
+        pg_fatal("old and new pg_controldata maximum identifier lengths are invalid or do not match");

     if (oldctrl->index == 0 || oldctrl->index != newctrl->index)
-        pg_fatal("old and new pg_controldata maximum indexed columns are invalid or do not match\n");
+        pg_fatal("old and new pg_controldata maximum indexed columns are invalid or do not match");

     if (oldctrl->toast == 0 || oldctrl->toast != newctrl->toast)
-        pg_fatal("old and new pg_controldata maximum TOAST chunk sizes are invalid or do not match\n");
+        pg_fatal("old and new pg_controldata maximum TOAST chunk sizes are invalid or do not match");

     /* large_object added in 9.5, so it might not exist in the old cluster */
     if (oldctrl->large_object != 0 &&
         oldctrl->large_object != newctrl->large_object)
-        pg_fatal("old and new pg_controldata large-object chunk sizes are invalid or do not match\n");
+        pg_fatal("old and new pg_controldata large-object chunk sizes are invalid or do not match");

     if (oldctrl->date_is_int != newctrl->date_is_int)
-        pg_fatal("old and new pg_controldata date/time storage types do not match\n");
+        pg_fatal("old and new pg_controldata date/time storage types do not match");

     /*
      * float8_pass_by_value does not need to match, but is used in
@@ -692,12 +696,12 @@ check_control_data(ControlData *oldctrl,
      */
     if (oldctrl->data_checksum_version == 0 &&
         newctrl->data_checksum_version != 0)
-        pg_fatal("old cluster does not use data checksums but the new one does\n");
+        pg_fatal("old cluster does not use data checksums but the new one does");
     else if (oldctrl->data_checksum_version != 0 &&
              newctrl->data_checksum_version == 0)
-        pg_fatal("old cluster uses data checksums but the new one does not\n");
+        pg_fatal("old cluster uses data checksums but the new one does not");
     else if (oldctrl->data_checksum_version != newctrl->data_checksum_version)
-        pg_fatal("old and new cluster pg_controldata checksum versions do not match\n");
+        pg_fatal("old and new cluster pg_controldata checksum versions do not match");
 }


@@ -713,12 +717,14 @@ disable_old_cluster(void)
     snprintf(old_path, sizeof(old_path), "%s/global/pg_control", old_cluster.pgdata);
     snprintf(new_path, sizeof(new_path), "%s/global/pg_control.old", old_cluster.pgdata);
     if (pg_mv_file(old_path, new_path) != 0)
-        pg_fatal("Unable to rename %s to %s.\n", old_path, new_path);
+        pg_fatal("could not rename file \"%s\" to \"%s\": %m",
+                 old_path, new_path);
     check_ok();

     pg_log(PG_REPORT, "\n"
            "If you want to start the old cluster, you will need to remove\n"
            "the \".old\" suffix from %s/global/pg_control.old.\n"
            "Because \"link\" mode was used, the old cluster cannot be safely\n"
-           "started once the new cluster has been started.\n\n", old_cluster.pgdata);
+           "started once the new cluster has been started.",
+           old_cluster.pgdata);
 }
diff --git a/src/bin/pg_upgrade/exec.c b/src/bin/pg_upgrade/exec.c
index fadeea12ca..131b67c16b 100644
--- a/src/bin/pg_upgrade/exec.c
+++ b/src/bin/pg_upgrade/exec.c
@@ -42,13 +42,13 @@ get_bin_version(ClusterInfo *cluster)

     if ((output = popen(cmd, "r")) == NULL ||
         fgets(cmd_output, sizeof(cmd_output), output) == NULL)
-        pg_fatal("could not get pg_ctl version data using %s: %s\n",
+        pg_fatal("could not get pg_ctl version data using %s: %s",
                  cmd, strerror(errno));

     pclose(output);

     if (sscanf(cmd_output, "%*s %*s %d.%d", &v1, &v2) < 1)
-        pg_fatal("could not get pg_ctl version output from %s\n", cmd);
+        pg_fatal("could not get pg_ctl version output from %s", cmd);

     if (v1 < 10)
     {
@@ -105,13 +105,13 @@ exec_prog(const char *log_filename, const char *opt_log_file,
     written += vsnprintf(cmd + written, MAXCMDLEN - written, fmt, ap);
     va_end(ap);
     if (written >= MAXCMDLEN)
-        pg_fatal("command too long\n");
+        pg_fatal("command too long");
     written += snprintf(cmd + written, MAXCMDLEN - written,
                         " >> \"%s\" 2>&1", log_file);
     if (written >= MAXCMDLEN)
-        pg_fatal("command too long\n");
+        pg_fatal("command too long");

-    pg_log(PG_VERBOSE, "%s\n", cmd);
+    pg_log(PG_VERBOSE, "%s", cmd);

 #ifdef WIN32

@@ -150,7 +150,7 @@ exec_prog(const char *log_filename, const char *opt_log_file,
 #endif

     if (log == NULL)
-        pg_fatal("could not open log file \"%s\": %m\n", log_file);
+        pg_fatal("could not open log file \"%s\": %m", log_file);

 #ifdef WIN32
     /* Are we printing "command:" before its output? */
@@ -182,16 +182,16 @@ exec_prog(const char *log_filename, const char *opt_log_file,
         report_status(PG_REPORT, "\n*failure*");
         fflush(stdout);

-        pg_log(PG_VERBOSE, "There were problems executing \"%s\"\n", cmd);
+        pg_log(PG_VERBOSE, "There were problems executing \"%s\"", cmd);
         if (opt_log_file)
             pg_log(exit_on_error ? PG_FATAL : PG_REPORT,
                    "Consult the last few lines of \"%s\" or \"%s\" for\n"
-                   "the probable cause of the failure.\n",
+                   "the probable cause of the failure.",
                    log_file, opt_log_file);
         else
             pg_log(exit_on_error ? PG_FATAL : PG_REPORT,
                    "Consult the last few lines of \"%s\" for\n"
-                   "the probable cause of the failure.\n",
+                   "the probable cause of the failure.",
                    log_file);
     }

@@ -205,7 +205,7 @@ exec_prog(const char *log_filename, const char *opt_log_file,
      * log these commands to a third file, but that just adds complexity.
      */
     if ((log = fopen(log_file, "a")) == NULL)
-        pg_fatal("could not write to log file \"%s\": %m\n", log_file);
+        pg_fatal("could not write to log file \"%s\": %m", log_file);
     fprintf(log, "\n\n");
     fclose(log);
 #endif
@@ -231,7 +231,7 @@ pid_lock_file_exists(const char *datadir)
     {
         /* ENOTDIR means we will throw a more useful error later */
         if (errno != ENOENT && errno != ENOTDIR)
-            pg_fatal("could not open file \"%s\" for reading: %s\n",
+            pg_fatal("could not open file \"%s\" for reading: %s",
                      path, strerror(errno));

         return false;
@@ -258,7 +258,7 @@ verify_directories(void)
 #else
     if (win32_check_directory_write_permissions() != 0)
 #endif
-        pg_fatal("You must have read and write access in the current directory.\n");
+        pg_fatal("You must have read and write access in the current directory.");

     check_bin_dir(&old_cluster, false);
     check_data_dir(&old_cluster);
@@ -311,10 +311,10 @@ check_single_dir(const char *pg_data, const char *subdir)
              subdir);

     if (stat(subDirName, &statBuf) != 0)
-        report_status(PG_FATAL, "check for \"%s\" failed: %s\n",
+        report_status(PG_FATAL, "check for \"%s\" failed: %s",
                       subDirName, strerror(errno));
     else if (!S_ISDIR(statBuf.st_mode))
-        report_status(PG_FATAL, "\"%s\" is not a directory\n",
+        report_status(PG_FATAL, "\"%s\" is not a directory",
                       subDirName);
 }

@@ -377,10 +377,10 @@ check_bin_dir(ClusterInfo *cluster, bool check_versions)

     /* check bindir */
     if (stat(cluster->bindir, &statBuf) != 0)
-        report_status(PG_FATAL, "check for \"%s\" failed: %s\n",
+        report_status(PG_FATAL, "check for \"%s\" failed: %s",
                       cluster->bindir, strerror(errno));
     else if (!S_ISDIR(statBuf.st_mode))
-        report_status(PG_FATAL, "\"%s\" is not a directory\n",
+        report_status(PG_FATAL, "\"%s\" is not a directory",
                       cluster->bindir);

     check_exec(cluster->bindir, "postgres", check_versions);
@@ -430,16 +430,16 @@ check_exec(const char *dir, const char *program, bool check_version)
     ret = validate_exec(path);

     if (ret == -1)
-        pg_fatal("check for \"%s\" failed: not a regular file\n",
+        pg_fatal("check for \"%s\" failed: not a regular file",
                  path);
     else if (ret == -2)
-        pg_fatal("check for \"%s\" failed: cannot execute (permission denied)\n",
+        pg_fatal("check for \"%s\" failed: cannot execute (permission denied)",
                  path);

     snprintf(cmd, sizeof(cmd), "\"%s\" -V", path);

     if (!pipe_read_line(cmd, line, sizeof(line)))
-        pg_fatal("check for \"%s\" failed: cannot execute\n",
+        pg_fatal("check for \"%s\" failed: cannot execute",
                  path);

     if (check_version)
@@ -449,7 +449,7 @@ check_exec(const char *dir, const char *program, bool check_version)
         snprintf(versionstr, sizeof(versionstr), "%s (PostgreSQL) " PG_VERSION, program);

         if (strcmp(line, versionstr) != 0)
-            pg_fatal("check for \"%s\" failed: incorrect version: found \"%s\", expected \"%s\"\n",
+            pg_fatal("check for \"%s\" failed: incorrect version: found \"%s\", expected \"%s\"",
                      path, line, versionstr);
     }
 }
diff --git a/src/bin/pg_upgrade/file.c b/src/bin/pg_upgrade/file.c
index b84868c751..c2ca99dcd8 100644
--- a/src/bin/pg_upgrade/file.c
+++ b/src/bin/pg_upgrade/file.c
@@ -40,25 +40,25 @@ cloneFile(const char *src, const char *dst,
 {
 #if defined(HAVE_COPYFILE) && defined(COPYFILE_CLONE_FORCE)
     if (copyfile(src, dst, NULL, COPYFILE_CLONE_FORCE) < 0)
-        pg_fatal("error while cloning relation \"%s.%s\" (\"%s\" to \"%s\"): %s\n",
+        pg_fatal("error while cloning relation \"%s.%s\" (\"%s\" to \"%s\"): %s",
                  schemaName, relName, src, dst, strerror(errno));
 #elif defined(__linux__) && defined(FICLONE)
     int            src_fd;
     int            dest_fd;

     if ((src_fd = open(src, O_RDONLY | PG_BINARY, 0)) < 0)
-        pg_fatal("error while cloning relation \"%s.%s\": could not open file \"%s\": %s\n",
+        pg_fatal("error while cloning relation \"%s.%s\": could not open file \"%s\": %s",
                  schemaName, relName, src, strerror(errno));

     if ((dest_fd = open(dst, O_RDWR | O_CREAT | O_EXCL | PG_BINARY,
                         pg_file_create_mode)) < 0)
-        pg_fatal("error while cloning relation \"%s.%s\": could not create file \"%s\": %s\n",
+        pg_fatal("error while cloning relation \"%s.%s\": could not create file \"%s\": %s",
                  schemaName, relName, dst, strerror(errno));

     if (ioctl(dest_fd, FICLONE, src_fd) < 0)
     {
         unlink(dst);
-        pg_fatal("error while cloning relation \"%s.%s\" (\"%s\" to \"%s\"): %s\n",
+        pg_fatal("error while cloning relation \"%s.%s\" (\"%s\" to \"%s\"): %s",
                  schemaName, relName, src, dst, strerror(errno));
     }

@@ -84,12 +84,12 @@ copyFile(const char *src, const char *dst,
     char       *buffer;

     if ((src_fd = open(src, O_RDONLY | PG_BINARY, 0)) < 0)
-        pg_fatal("error while copying relation \"%s.%s\": could not open file \"%s\": %s\n",
+        pg_fatal("error while copying relation \"%s.%s\": could not open file \"%s\": %s",
                  schemaName, relName, src, strerror(errno));

     if ((dest_fd = open(dst, O_RDWR | O_CREAT | O_EXCL | PG_BINARY,
                         pg_file_create_mode)) < 0)
-        pg_fatal("error while copying relation \"%s.%s\": could not create file \"%s\": %s\n",
+        pg_fatal("error while copying relation \"%s.%s\": could not create file \"%s\": %s",
                  schemaName, relName, dst, strerror(errno));

     /* copy in fairly large chunks for best efficiency */
@@ -103,7 +103,7 @@ copyFile(const char *src, const char *dst,
         ssize_t        nbytes = read(src_fd, buffer, COPY_BUF_SIZE);

         if (nbytes < 0)
-            pg_fatal("error while copying relation \"%s.%s\": could not read file \"%s\": %s\n",
+            pg_fatal("error while copying relation \"%s.%s\": could not read file \"%s\": %s",
                      schemaName, relName, src, strerror(errno));

         if (nbytes == 0)
@@ -115,7 +115,7 @@ copyFile(const char *src, const char *dst,
             /* if write didn't set errno, assume problem is no disk space */
             if (errno == 0)
                 errno = ENOSPC;
-            pg_fatal("error while copying relation \"%s.%s\": could not write file \"%s\": %s\n",
+            pg_fatal("error while copying relation \"%s.%s\": could not write file \"%s\": %s",
                      schemaName, relName, dst, strerror(errno));
         }
     }
@@ -129,7 +129,7 @@ copyFile(const char *src, const char *dst,
     if (CopyFile(src, dst, true) == 0)
     {
         _dosmaperr(GetLastError());
-        pg_fatal("error while copying relation \"%s.%s\" (\"%s\" to \"%s\"): %s\n",
+        pg_fatal("error while copying relation \"%s.%s\" (\"%s\" to \"%s\"): %s",
                  schemaName, relName, src, dst, strerror(errno));
     }

@@ -148,7 +148,7 @@ linkFile(const char *src, const char *dst,
          const char *schemaName, const char *relName)
 {
     if (link(src, dst) < 0)
-        pg_fatal("error while creating link for relation \"%s.%s\" (\"%s\" to \"%s\"): %s\n",
+        pg_fatal("error while creating link for relation \"%s.%s\" (\"%s\" to \"%s\"): %s",
                  schemaName, relName, src, dst, strerror(errno));
 }

@@ -187,16 +187,16 @@ rewriteVisibilityMap(const char *fromfile, const char *tofile,
     rewriteVmBytesPerPage = (BLCKSZ - SizeOfPageHeaderData) / 2;

     if ((src_fd = open(fromfile, O_RDONLY | PG_BINARY, 0)) < 0)
-        pg_fatal("error while copying relation \"%s.%s\": could not open file \"%s\": %s\n",
+        pg_fatal("error while copying relation \"%s.%s\": could not open file \"%s\": %s",
                  schemaName, relName, fromfile, strerror(errno));

     if (fstat(src_fd, &statbuf) != 0)
-        pg_fatal("error while copying relation \"%s.%s\": could not stat file \"%s\": %s\n",
+        pg_fatal("error while copying relation \"%s.%s\": could not stat file \"%s\": %s",
                  schemaName, relName, fromfile, strerror(errno));

     if ((dst_fd = open(tofile, O_RDWR | O_CREAT | O_EXCL | PG_BINARY,
                        pg_file_create_mode)) < 0)
-        pg_fatal("error while copying relation \"%s.%s\": could not create file \"%s\": %s\n",
+        pg_fatal("error while copying relation \"%s.%s\": could not create file \"%s\": %s",
                  schemaName, relName, tofile, strerror(errno));

     /* Save old file size */
@@ -220,10 +220,10 @@ rewriteVisibilityMap(const char *fromfile, const char *tofile,
         if ((bytesRead = read(src_fd, buffer.data, BLCKSZ)) != BLCKSZ)
         {
             if (bytesRead < 0)
-                pg_fatal("error while copying relation \"%s.%s\": could not read file \"%s\": %s\n",
+                pg_fatal("error while copying relation \"%s.%s\": could not read file \"%s\": %s",
                          schemaName, relName, fromfile, strerror(errno));
             else
-                pg_fatal("error while copying relation \"%s.%s\": partial page found in file \"%s\"\n",
+                pg_fatal("error while copying relation \"%s.%s\": partial page found in file \"%s\"",
                          schemaName, relName, fromfile);
         }

@@ -298,7 +298,7 @@ rewriteVisibilityMap(const char *fromfile, const char *tofile,
                 /* if write didn't set errno, assume problem is no disk space */
                 if (errno == 0)
                     errno = ENOSPC;
-                pg_fatal("error while copying relation \"%s.%s\": could not write file \"%s\": %s\n",
+                pg_fatal("error while copying relation \"%s.%s\": could not write file \"%s\": %s",
                          schemaName, relName, tofile, strerror(errno));
             }

@@ -325,7 +325,7 @@ check_file_clone(void)

 #if defined(HAVE_COPYFILE) && defined(COPYFILE_CLONE_FORCE)
     if (copyfile(existing_file, new_link_file, NULL, COPYFILE_CLONE_FORCE) < 0)
-        pg_fatal("could not clone file between old and new data directories: %s\n",
+        pg_fatal("could not clone file between old and new data directories: %s",
                  strerror(errno));
 #elif defined(__linux__) && defined(FICLONE)
     {
@@ -333,23 +333,23 @@ check_file_clone(void)
         int            dest_fd;

         if ((src_fd = open(existing_file, O_RDONLY | PG_BINARY, 0)) < 0)
-            pg_fatal("could not open file \"%s\": %s\n",
+            pg_fatal("could not open file \"%s\": %s",
                      existing_file, strerror(errno));

         if ((dest_fd = open(new_link_file, O_RDWR | O_CREAT | O_EXCL | PG_BINARY,
                             pg_file_create_mode)) < 0)
-            pg_fatal("could not create file \"%s\": %s\n",
+            pg_fatal("could not create file \"%s\": %s",
                      new_link_file, strerror(errno));

         if (ioctl(dest_fd, FICLONE, src_fd) < 0)
-            pg_fatal("could not clone file between old and new data directories: %s\n",
+            pg_fatal("could not clone file between old and new data directories: %s",
                      strerror(errno));

         close(src_fd);
         close(dest_fd);
     }
 #else
-    pg_fatal("file cloning not supported on this platform\n");
+    pg_fatal("file cloning not supported on this platform");
 #endif

     unlink(new_link_file);
@@ -367,7 +367,7 @@ check_hard_link(void)

     if (link(existing_file, new_link_file) < 0)
         pg_fatal("could not create hard link between old and new data directories: %s\n"
-                 "In link mode the old and new data directories must be on the same file system.\n",
+                 "In link mode the old and new data directories must be on the same file system.",
                  strerror(errno));

     unlink(new_link_file);
diff --git a/src/bin/pg_upgrade/function.c b/src/bin/pg_upgrade/function.c
index ea785df771..70b492cc3a 100644
--- a/src/bin/pg_upgrade/function.c
+++ b/src/bin/pg_upgrade/function.c
@@ -162,7 +162,7 @@ check_loadable_libraries(void)
                 was_load_failure = true;

                 if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
-                    pg_fatal("could not open file \"%s\": %s\n",
+                    pg_fatal("could not open file \"%s\": %s",
                              output_path, strerror(errno));
                 fprintf(script, _("could not load library \"%s\": %s"),
                         lib,
@@ -184,12 +184,12 @@ check_loadable_libraries(void)
     if (found)
     {
         fclose(script);
-        pg_log(PG_REPORT, "fatal\n");
+        pg_log(PG_REPORT, "fatal");
         pg_fatal("Your installation references loadable libraries that are missing from the\n"
                  "new installation.  You can add these libraries to the new installation,\n"
                  "or remove the functions using them from the old installation.  A list of\n"
                  "problem libraries is in the file:\n"
-                 "    %s\n\n", output_path);
+                 "    %s", output_path);
     }
     else
         check_ok();
diff --git a/src/bin/pg_upgrade/info.c b/src/bin/pg_upgrade/info.c
index 5c3968e0ea..b6a35651f5 100644
--- a/src/bin/pg_upgrade/info.c
+++ b/src/bin/pg_upgrade/info.c
@@ -123,7 +123,7 @@ gen_db_file_maps(DbInfo *old_db, DbInfo *new_db,
             strcmp(old_rel->relname, new_rel->relname) != 0)
         {
             pg_log(PG_WARNING, "Relation names for OID %u in database \"%s\" do not match: "
-                   "old name \"%s.%s\", new name \"%s.%s\"\n",
+                   "old name \"%s.%s\", new name \"%s.%s\"",
                    old_rel->reloid, old_db->db_name,
                    old_rel->nspname, old_rel->relname,
                    new_rel->nspname, new_rel->relname);
@@ -142,7 +142,7 @@ gen_db_file_maps(DbInfo *old_db, DbInfo *new_db,
     }

     if (!all_matched)
-        pg_fatal("Failed to match up old and new tables in database \"%s\"\n",
+        pg_fatal("Failed to match up old and new tables in database \"%s\"",
                  old_db->db_name);

     *nmaps = num_maps;
@@ -257,10 +257,10 @@ report_unmatched_relation(const RelInfo *rel, const DbInfo *db, bool is_new_db)
     }

     if (is_new_db)
-        pg_log(PG_WARNING, "No match found in old cluster for new relation with OID %u in database \"%s\": %s\n",
+        pg_log(PG_WARNING, "No match found in old cluster for new relation with OID %u in database \"%s\": %s",
                reloid, db->db_name, reldesc);
     else
-        pg_log(PG_WARNING, "No match found in new cluster for old relation with OID %u in database \"%s\": %s\n",
+        pg_log(PG_WARNING, "No match found in new cluster for old relation with OID %u in database \"%s\": %s",
                reloid, db->db_name, reldesc);
 }

@@ -284,9 +284,9 @@ get_db_and_rel_infos(ClusterInfo *cluster)
         get_rel_infos(cluster, &cluster->dbarr.dbs[dbnum]);

     if (cluster == &old_cluster)
-        pg_log(PG_VERBOSE, "\nsource databases:\n");
+        pg_log(PG_VERBOSE, "\nsource databases:");
     else
-        pg_log(PG_VERBOSE, "\ntarget databases:\n");
+        pg_log(PG_VERBOSE, "\ntarget databases:");

     if (log_opts.verbose)
         print_db_infos(&cluster->dbarr);
@@ -602,9 +602,8 @@ print_db_infos(DbInfoArr *db_arr)

     for (dbnum = 0; dbnum < db_arr->ndbs; dbnum++)
     {
-        pg_log(PG_VERBOSE, "Database: %s\n", db_arr->dbs[dbnum].db_name);
+        pg_log(PG_VERBOSE, "Database: %s", db_arr->dbs[dbnum].db_name);
         print_rel_infos(&db_arr->dbs[dbnum].rel_arr);
-        pg_log(PG_VERBOSE, "\n\n");
     }
 }

@@ -615,7 +614,7 @@ print_rel_infos(RelInfoArr *rel_arr)
     int            relnum;

     for (relnum = 0; relnum < rel_arr->nrels; relnum++)
-        pg_log(PG_VERBOSE, "relname: %s.%s: reloid: %u reltblspace: %s\n",
+        pg_log(PG_VERBOSE, "relname: %s.%s: reloid: %u reltblspace: %s",
                rel_arr->rels[relnum].nspname,
                rel_arr->rels[relnum].relname,
                rel_arr->rels[relnum].reloid,
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index e75be2c423..0954b75ec0 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -97,7 +97,7 @@ parseCommandLine(int argc, char *argv[])

     /* Allow help and version to be run as root, so do the test here. */
     if (os_user_effective_id == 0)
-        pg_fatal("%s: cannot be run as root\n", os_info.progname);
+        pg_fatal("%s: cannot be run as root", os_info.progname);

     while ((option = getopt_long(argc, argv, "d:D:b:B:cj:kNo:O:p:P:rs:U:v",
                                  long_options, &optindex)) != -1)
@@ -164,12 +164,12 @@ parseCommandLine(int argc, char *argv[])

             case 'p':
                 if ((old_cluster.port = atoi(optarg)) <= 0)
-                    pg_fatal("invalid old port number\n");
+                    pg_fatal("invalid old port number");
                 break;

             case 'P':
                 if ((new_cluster.port = atoi(optarg)) <= 0)
-                    pg_fatal("invalid new port number\n");
+                    pg_fatal("invalid new port number");
                 break;

             case 'r':
@@ -202,10 +202,10 @@ parseCommandLine(int argc, char *argv[])
     }

     if (optind < argc)
-        pg_fatal("too many command-line arguments (first is \"%s\")\n", argv[optind]);
+        pg_fatal("too many command-line arguments (first is \"%s\")", argv[optind]);

     if (log_opts.verbose)
-        pg_log(PG_REPORT, "Running in verbose mode\n");
+        pg_log(PG_REPORT, "Running in verbose mode");

     log_opts.isatty = isatty(fileno(stdout));

@@ -248,10 +248,10 @@ parseCommandLine(int argc, char *argv[])
         canonicalize_path(new_cluster_pgdata);

         if (!getcwd(cwd, MAXPGPATH))
-            pg_fatal("could not determine current directory\n");
+            pg_fatal("could not determine current directory");
         canonicalize_path(cwd);
         if (path_is_prefix_of_path(new_cluster_pgdata, cwd))
-            pg_fatal("cannot run pg_upgrade from inside the new cluster data directory on Windows\n");
+            pg_fatal("cannot run pg_upgrade from inside the new cluster data directory on Windows");
     }
 #endif
 }
@@ -347,14 +347,14 @@ check_required_directory(char **dirpath, const char *envVarName, bool useCwd,
             char        cwd[MAXPGPATH];

             if (!getcwd(cwd, MAXPGPATH))
-                pg_fatal("could not determine current directory\n");
+                pg_fatal("could not determine current directory");
             *dirpath = pg_strdup(cwd);
         }
         else if (missingOk)
             return;
         else
             pg_fatal("You must identify the directory where the %s.\n"
-                     "Please use the %s command-line option or the %s environment variable.\n",
+                     "Please use the %s command-line option or the %s environment variable.",
                      description, cmdLineOption, envVarName);
     }

@@ -419,7 +419,7 @@ adjust_data_dir(ClusterInfo *cluster)

     if ((output = popen(cmd, "r")) == NULL ||
         fgets(cmd_output, sizeof(cmd_output), output) == NULL)
-        pg_fatal("could not get data directory using %s: %s\n",
+        pg_fatal("could not get data directory using %s: %s",
                  cmd, strerror(errno));

     pclose(output);
@@ -462,7 +462,7 @@ get_sock_dir(ClusterInfo *cluster, bool live_check)
         snprintf(filename, sizeof(filename), "%s/postmaster.pid",
                  cluster->pgdata);
         if ((fp = fopen(filename, "r")) == NULL)
-            pg_fatal("could not open file \"%s\": %s\n",
+            pg_fatal("could not open file \"%s\": %s",
                      filename, strerror(errno));

         for (lineno = 1;
@@ -470,7 +470,7 @@ get_sock_dir(ClusterInfo *cluster, bool live_check)
              lineno++)
         {
             if (fgets(line, sizeof(line), fp) == NULL)
-                pg_fatal("could not read line %d from file \"%s\": %s\n",
+                pg_fatal("could not read line %d from file \"%s\": %s",
                          lineno, filename, strerror(errno));

             /* potentially overwrite user-supplied value */
@@ -487,7 +487,7 @@ get_sock_dir(ClusterInfo *cluster, bool live_check)

         /* warn of port number correction */
         if (orig_port != DEF_PGUPORT && old_cluster.port != orig_port)
-            pg_log(PG_WARNING, "user-supplied old port number %hu corrected to %hu\n",
+            pg_log(PG_WARNING, "user-supplied old port number %hu corrected to %hu",
                    orig_port, cluster->port);
     }
 #else                            /* !HAVE_UNIX_SOCKETS || WIN32 */
diff --git a/src/bin/pg_upgrade/parallel.c b/src/bin/pg_upgrade/parallel.c
index ca40df7b4c..5cb04758aa 100644
--- a/src/bin/pg_upgrade/parallel.c
+++ b/src/bin/pg_upgrade/parallel.c
@@ -124,7 +124,7 @@ parallel_exec_prog(const char *log_file, const char *opt_log_file,
             _exit(!exec_prog(log_file, opt_log_file, true, true, "%s", cmd));
         else if (child < 0)
             /* fork failed */
-            pg_fatal("could not create worker process: %s\n", strerror(errno));
+            pg_fatal("could not create worker process: %s", strerror(errno));
 #else
         /* empty array element are always at the end */
         new_arg = exec_thread_args[parallel_jobs - 1];
@@ -143,7 +143,7 @@ parallel_exec_prog(const char *log_file, const char *opt_log_file,
         child = (HANDLE) _beginthreadex(NULL, 0, (void *) win32_exec_prog,
                                         new_arg, 0, NULL);
         if (child == 0)
-            pg_fatal("could not create worker thread: %s\n", strerror(errno));
+            pg_fatal("could not create worker thread: %s", strerror(errno));

         thread_handles[parallel_jobs - 1] = child;
 #endif
@@ -235,7 +235,7 @@ parallel_transfer_all_new_dbs(DbInfoArr *old_db_arr, DbInfoArr *new_db_arr,
         }
         else if (child < 0)
             /* fork failed */
-            pg_fatal("could not create worker process: %s\n", strerror(errno));
+            pg_fatal("could not create worker process: %s", strerror(errno));
 #else
         /* empty array element are always at the end */
         new_arg = transfer_thread_args[parallel_jobs - 1];
@@ -256,7 +256,7 @@ parallel_transfer_all_new_dbs(DbInfoArr *old_db_arr, DbInfoArr *new_db_arr,
         child = (HANDLE) _beginthreadex(NULL, 0, (void *) win32_transfer_all_new_dbs,
                                         new_arg, 0, NULL);
         if (child == 0)
-            pg_fatal("could not create worker thread: %s\n", strerror(errno));
+            pg_fatal("could not create worker thread: %s", strerror(errno));

         thread_handles[parallel_jobs - 1] = child;
 #endif
@@ -297,11 +297,11 @@ reap_child(bool wait_for_child)
 #ifndef WIN32
     child = waitpid(-1, &work_status, wait_for_child ? 0 : WNOHANG);
     if (child == (pid_t) -1)
-        pg_fatal("%s() failed: %s\n", "waitpid", strerror(errno));
+        pg_fatal("%s() failed: %s", "waitpid", strerror(errno));
     if (child == 0)
         return false;            /* no children, or no dead children */
     if (work_status != 0)
-        pg_fatal("child process exited abnormally: status %d\n", work_status);
+        pg_fatal("child process exited abnormally: status %d", work_status);
 #else
     /* wait for one to finish */
     thread_num = WaitForMultipleObjects(parallel_jobs, thread_handles,
@@ -316,7 +316,7 @@ reap_child(bool wait_for_child)
     /* get the result */
     GetExitCodeThread(thread_handles[thread_num], &res);
     if (res != 0)
-        pg_fatal("child worker exited abnormally: %s\n", strerror(errno));
+        pg_fatal("child worker exited abnormally: %s", strerror(errno));

     /* dispose of handle to stop leaks */
     CloseHandle(thread_handles[thread_num]);
diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c
index 265d829490..115faa222e 100644
--- a/src/bin/pg_upgrade/pg_upgrade.c
+++ b/src/bin/pg_upgrade/pg_upgrade.c
@@ -81,6 +81,10 @@ main(int argc, char **argv)
     char       *deletion_script_file_name = NULL;
     bool        live_check = false;

+    /*
+     * pg_upgrade doesn't currently use common/logging.c, but initialize it
+     * anyway because we might call common code that does.
+     */
     pg_logging_init(argv[0]);
     set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_upgrade"));

@@ -99,7 +103,7 @@ main(int argc, char **argv)
      * output directories with correct permissions.
      */
     if (!GetDataDirectoryCreatePerm(new_cluster.pgdata))
-        pg_fatal("could not read permissions of directory \"%s\": %s\n",
+        pg_fatal("could not read permissions of directory \"%s\": %s",
                  new_cluster.pgdata, strerror(errno));

     umask(pg_mode_mask);
@@ -133,7 +137,7 @@ main(int argc, char **argv)
     pg_log(PG_REPORT,
            "\n"
            "Performing Upgrade\n"
-           "------------------\n");
+           "------------------");

     prepare_new_cluster();

@@ -197,7 +201,7 @@ main(int argc, char **argv)
     pg_log(PG_REPORT,
            "\n"
            "Upgrade Complete\n"
-           "----------------\n");
+           "----------------");

     output_completion_banner(deletion_script_file_name);

@@ -228,7 +232,7 @@ make_outputdirs(char *pgdata)
     log_opts.rootdir = (char *) pg_malloc0(MAXPGPATH);
     len = snprintf(log_opts.rootdir, MAXPGPATH, "%s/%s", pgdata, BASE_OUTPUTDIR);
     if (len >= MAXPGPATH)
-        pg_fatal("directory path for new cluster is too long\n");
+        pg_fatal("directory path for new cluster is too long");

     /* BASE_OUTPUTDIR/$timestamp/ */
     gettimeofday(&time, NULL);
@@ -241,42 +245,42 @@ make_outputdirs(char *pgdata)
     len = snprintf(log_opts.basedir, MAXPGPATH, "%s/%s", log_opts.rootdir,
                    timebuf);
     if (len >= MAXPGPATH)
-        pg_fatal("directory path for new cluster is too long\n");
+        pg_fatal("directory path for new cluster is too long");

     /* BASE_OUTPUTDIR/$timestamp/dump/ */
     log_opts.dumpdir = (char *) pg_malloc0(MAXPGPATH);
     len = snprintf(log_opts.dumpdir, MAXPGPATH, "%s/%s/%s", log_opts.rootdir,
                    timebuf, DUMP_OUTPUTDIR);
     if (len >= MAXPGPATH)
-        pg_fatal("directory path for new cluster is too long\n");
+        pg_fatal("directory path for new cluster is too long");

     /* BASE_OUTPUTDIR/$timestamp/log/ */
     log_opts.logdir = (char *) pg_malloc0(MAXPGPATH);
     len = snprintf(log_opts.logdir, MAXPGPATH, "%s/%s/%s", log_opts.rootdir,
                    timebuf, LOG_OUTPUTDIR);
     if (len >= MAXPGPATH)
-        pg_fatal("directory path for new cluster is too long\n");
+        pg_fatal("directory path for new cluster is too long");

     /*
      * Ignore the error case where the root path exists, as it is kept the
      * same across runs.
      */
     if (mkdir(log_opts.rootdir, pg_dir_create_mode) < 0 && errno != EEXIST)
-        pg_fatal("could not create directory \"%s\": %m\n", log_opts.rootdir);
+        pg_fatal("could not create directory \"%s\": %m", log_opts.rootdir);
     if (mkdir(log_opts.basedir, pg_dir_create_mode) < 0)
-        pg_fatal("could not create directory \"%s\": %m\n", log_opts.basedir);
+        pg_fatal("could not create directory \"%s\": %m", log_opts.basedir);
     if (mkdir(log_opts.dumpdir, pg_dir_create_mode) < 0)
-        pg_fatal("could not create directory \"%s\": %m\n", log_opts.dumpdir);
+        pg_fatal("could not create directory \"%s\": %m", log_opts.dumpdir);
     if (mkdir(log_opts.logdir, pg_dir_create_mode) < 0)
-        pg_fatal("could not create directory \"%s\": %m\n", log_opts.logdir);
+        pg_fatal("could not create directory \"%s\": %m", log_opts.logdir);

     len = snprintf(filename_path, sizeof(filename_path), "%s/%s",
                    log_opts.logdir, INTERNAL_LOG_FILE);
     if (len >= sizeof(filename_path))
-        pg_fatal("directory path for new cluster is too long\n");
+        pg_fatal("directory path for new cluster is too long");

     if ((log_opts.internal = fopen_priv(filename_path, "a")) == NULL)
-        pg_fatal("could not open log file \"%s\": %m\n", filename_path);
+        pg_fatal("could not open log file \"%s\": %m", filename_path);

     /* label start of upgrade in logfiles */
     for (filename = output_files; *filename != NULL; filename++)
@@ -284,9 +288,9 @@ make_outputdirs(char *pgdata)
         len = snprintf(filename_path, sizeof(filename_path), "%s/%s",
                        log_opts.logdir, *filename);
         if (len >= sizeof(filename_path))
-            pg_fatal("directory path for new cluster is too long\n");
+            pg_fatal("directory path for new cluster is too long");
         if ((fp = fopen_priv(filename_path, "a")) == NULL)
-            pg_fatal("could not write to log file \"%s\": %m\n", filename_path);
+            pg_fatal("could not write to log file \"%s\": %m", filename_path);

         fprintf(fp,
                 "-----------------------------------------------------------------\n"
@@ -317,7 +321,7 @@ setup(char *argv0, bool *live_check)
         char        exec_path[MAXPGPATH];

         if (find_my_exec(argv0, exec_path) < 0)
-            pg_fatal("%s: could not find own program executable\n", argv0);
+            pg_fatal("%s: could not find own program executable", argv0);
         /* Trim off program name and keep just path */
         *last_dir_separator(exec_path) = '\0';
         canonicalize_path(exec_path);
@@ -344,7 +348,7 @@ setup(char *argv0, bool *live_check)
         {
             if (!user_opts.check)
                 pg_fatal("There seems to be a postmaster servicing the old cluster.\n"
-                         "Please shutdown that postmaster and try again.\n");
+                         "Please shutdown that postmaster and try again.");
             else
                 *live_check = true;
         }
@@ -357,7 +361,7 @@ setup(char *argv0, bool *live_check)
             stop_postmaster(false);
         else
             pg_fatal("There seems to be a postmaster servicing the new cluster.\n"
-                     "Please shutdown that postmaster and try again.\n");
+                     "Please shutdown that postmaster and try again.");
     }
 }

@@ -529,7 +533,7 @@ remove_new_subdir(const char *subdir, bool rmtopdir)

     snprintf(new_path, sizeof(new_path), "%s/%s", new_cluster.pgdata, subdir);
     if (!rmtree(new_path, rmtopdir))
-        pg_fatal("could not delete directory \"%s\"\n", new_path);
+        pg_fatal("could not delete directory \"%s\"", new_path);

     check_ok();
 }
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index 55de244ac0..ebad6d7dad 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -235,7 +235,8 @@ typedef enum
 typedef enum
 {
     PG_VERBOSE,
-    PG_STATUS,
+    PG_STATUS,                    /* these messages do not get a newline added */
+    PG_REPORT_NONL,                /* these too */
     PG_REPORT,
     PG_WARNING,
     PG_FATAL
diff --git a/src/bin/pg_upgrade/relfilenode.c b/src/bin/pg_upgrade/relfilenode.c
index d23ac884bd..ba9f5a1b60 100644
--- a/src/bin/pg_upgrade/relfilenode.c
+++ b/src/bin/pg_upgrade/relfilenode.c
@@ -112,7 +112,7 @@ transfer_all_new_dbs(DbInfoArr *old_db_arr, DbInfoArr *new_db_arr,
         }

         if (new_dbnum >= new_db_arr->ndbs)
-            pg_fatal("old database \"%s\" not found in the new cluster\n",
+            pg_fatal("old database \"%s\" not found in the new cluster",
                      old_db->db_name);

         mappings = gen_db_file_maps(old_db, new_db, &n_maps, old_pgdata,
@@ -215,7 +215,7 @@ transfer_relfile(FileNameMap *map, const char *type_suffix, bool vm_must_add_fro
                 if (errno == ENOENT)
                     return;
                 else
-                    pg_fatal("error while checking for file existence \"%s.%s\" (\"%s\" to \"%s\"): %s\n",
+                    pg_fatal("error while checking for file existence \"%s.%s\" (\"%s\" to \"%s\"): %s",
                              map->nspname, map->relname, old_file, new_file,
                              strerror(errno));
             }
@@ -233,7 +233,7 @@ transfer_relfile(FileNameMap *map, const char *type_suffix, bool vm_must_add_fro
         if (vm_must_add_frozenbit && strcmp(type_suffix, "_vm") == 0)
         {
             /* Need to rewrite visibility map format */
-            pg_log(PG_VERBOSE, "rewriting \"%s\" to \"%s\"\n",
+            pg_log(PG_VERBOSE, "rewriting \"%s\" to \"%s\"",
                    old_file, new_file);
             rewriteVisibilityMap(old_file, new_file, map->nspname, map->relname);
         }
@@ -241,17 +241,17 @@ transfer_relfile(FileNameMap *map, const char *type_suffix, bool vm_must_add_fro
             switch (user_opts.transfer_mode)
             {
                 case TRANSFER_MODE_CLONE:
-                    pg_log(PG_VERBOSE, "cloning \"%s\" to \"%s\"\n",
+                    pg_log(PG_VERBOSE, "cloning \"%s\" to \"%s\"",
                            old_file, new_file);
                     cloneFile(old_file, new_file, map->nspname, map->relname);
                     break;
                 case TRANSFER_MODE_COPY:
-                    pg_log(PG_VERBOSE, "copying \"%s\" to \"%s\"\n",
+                    pg_log(PG_VERBOSE, "copying \"%s\" to \"%s\"",
                            old_file, new_file);
                     copyFile(old_file, new_file, map->nspname, map->relname);
                     break;
                 case TRANSFER_MODE_LINK:
-                    pg_log(PG_VERBOSE, "linking \"%s\" to \"%s\"\n",
+                    pg_log(PG_VERBOSE, "linking \"%s\" to \"%s\"",
                            old_file, new_file);
                     linkFile(old_file, new_file, map->nspname, map->relname);
             }
diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 265137e86b..79479edc0e 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -130,7 +130,7 @@ executeQueryOrDie(PGconn *conn, const char *fmt,...)
     vsnprintf(query, sizeof(query), fmt, args);
     va_end(args);

-    pg_log(PG_VERBOSE, "executing: %s\n", query);
+    pg_log(PG_VERBOSE, "executing: %s", query);
     result = PQexec(conn, query);
     status = PQresultStatus(result);

@@ -166,11 +166,11 @@ get_major_server_version(ClusterInfo *cluster)
     snprintf(ver_filename, sizeof(ver_filename), "%s/PG_VERSION",
              cluster->pgdata);
     if ((version_fd = fopen(ver_filename, "r")) == NULL)
-        pg_fatal("could not open version file \"%s\": %m\n", ver_filename);
+        pg_fatal("could not open version file \"%s\": %m", ver_filename);

     if (fscanf(version_fd, "%63s", cluster->major_version_str) == 0 ||
         sscanf(cluster->major_version_str, "%d.%d", &v1, &v2) < 1)
-        pg_fatal("could not parse version file \"%s\"\n", ver_filename);
+        pg_fatal("could not parse version file \"%s\"", ver_filename);

     fclose(version_fd);

@@ -293,11 +293,11 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
             PQfinish(conn);
         if (cluster == &old_cluster)
             pg_fatal("could not connect to source postmaster started with the command:\n"
-                     "%s\n",
+                     "%s",
                      cmd);
         else
             pg_fatal("could not connect to target postmaster started with the command:\n"
-                     "%s\n",
+                     "%s",
                      cmd);
     }
     PQfinish(conn);
@@ -310,9 +310,9 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
     if (!pg_ctl_return)
     {
         if (cluster == &old_cluster)
-            pg_fatal("pg_ctl failed to start the source server, or connection failed\n");
+            pg_fatal("pg_ctl failed to start the source server, or connection failed");
         else
-            pg_fatal("pg_ctl failed to start the target server, or connection failed\n");
+            pg_fatal("pg_ctl failed to start the target server, or connection failed");
     }

     return true;
@@ -357,7 +357,7 @@ check_pghost_envvar(void)
     start = PQconndefaults();

     if (!start)
-        pg_fatal("out of memory\n");
+        pg_fatal("out of memory");

     for (option = start; option->keyword != NULL; option++)
     {
@@ -370,7 +370,7 @@ check_pghost_envvar(void)
             /* check for 'local' host values */
                 (strcmp(value, "localhost") != 0 && strcmp(value, "127.0.0.1") != 0 &&
                  strcmp(value, "::1") != 0 && !is_unixsock_path(value)))
-                pg_fatal("libpq environment variable %s has a non-local server value: %s\n",
+                pg_fatal("libpq environment variable %s has a non-local server value: %s",
                          option->envvar, value);
         }
     }
diff --git a/src/bin/pg_upgrade/tablespace.c b/src/bin/pg_upgrade/tablespace.c
index 90a6b52a1f..fa2fbbce32 100644
--- a/src/bin/pg_upgrade/tablespace.c
+++ b/src/bin/pg_upgrade/tablespace.c
@@ -26,7 +26,7 @@ init_tablespaces(void)
     if (os_info.num_old_tablespaces > 0 &&
         strcmp(old_cluster.tablespace_suffix, new_cluster.tablespace_suffix) == 0)
         pg_fatal("Cannot upgrade to/from the same system catalog version when\n"
-                 "using tablespaces.\n");
+                 "using tablespaces.");
 }


@@ -80,16 +80,16 @@ get_tablespace_paths(void)
         {
             if (errno == ENOENT)
                 report_status(PG_FATAL,
-                              "tablespace directory \"%s\" does not exist\n",
+                              "tablespace directory \"%s\" does not exist",
                               os_info.old_tablespaces[tblnum]);
             else
                 report_status(PG_FATAL,
-                              "could not stat tablespace directory \"%s\": %s\n",
+                              "could not stat tablespace directory \"%s\": %s",
                               os_info.old_tablespaces[tblnum], strerror(errno));
         }
         if (!S_ISDIR(statBuf.st_mode))
             report_status(PG_FATAL,
-                          "tablespace path \"%s\" is not a directory\n",
+                          "tablespace path \"%s\" is not a directory",
                           os_info.old_tablespaces[tblnum]);
     }

diff --git a/src/bin/pg_upgrade/util.c b/src/bin/pg_upgrade/util.c
index f25e14c321..593a843917 100644
--- a/src/bin/pg_upgrade/util.c
+++ b/src/bin/pg_upgrade/util.c
@@ -23,18 +23,19 @@ static void pg_log_v(eLogType type, const char *fmt, va_list ap) pg_attribute_pr
  * report_status()
  *
  *    Displays the result of an operation (ok, failed, error message,...)
+ *
+ *    This is no longer functionally different from pg_log(), but we keep
+ *    it around to maintain a notational distinction between operation
+ *    results and other messages.
  */
 void
 report_status(eLogType type, const char *fmt,...)
 {
     va_list        args;
-    char        message[MAX_STRING];

     va_start(args, fmt);
-    vsnprintf(message, sizeof(message), fmt, args);
+    pg_log_v(type, fmt, args);
     va_end(args);
-
-    pg_log(type, "%s\n", message);
 }


@@ -49,10 +50,10 @@ end_progress_output(void)
     if (log_opts.isatty)
     {
         printf("\r");
-        pg_log(PG_REPORT, "%-*s", MESSAGE_WIDTH, "");
+        pg_log(PG_REPORT_NONL, "%-*s", MESSAGE_WIDTH, "");
     }
     else if (log_opts.verbose)
-        pg_log(PG_REPORT, "%-*s", MESSAGE_WIDTH, "");
+        pg_log(PG_REPORT_NONL, "%-*s", MESSAGE_WIDTH, "");
 }

 /*
@@ -102,16 +103,16 @@ cleanup_output_dirs(void)
  * prep_status
  *
  *    Displays a message that describes an operation we are about to begin.
- *    We pad the message out to MESSAGE_WIDTH characters so that all of the "ok" and
- *    "failed" indicators line up nicely.
+ *    We pad the message out to MESSAGE_WIDTH characters so that all of the
+ *    "ok" and "failed" indicators line up nicely.  (Overlength messages
+ *    will be truncated, so don't get too verbose.)
  *
  *    A typical sequence would look like this:
- *        prep_status("about to flarb the next %d files", fileCount );
- *
- *        if(( message = flarbFiles(fileCount)) == NULL)
- *          report_status(PG_REPORT, "ok" );
+ *        prep_status("about to flarb the next %d files", fileCount);
+ *        if ((message = flarbFiles(fileCount)) == NULL)
+ *          report_status(PG_REPORT, "ok");
  *        else
- *          pg_log(PG_FATAL, "failed - %s\n", message );
+ *          pg_log(PG_FATAL, "failed: %s", message);
  */
 void
 prep_status(const char *fmt,...)
@@ -124,7 +125,7 @@ prep_status(const char *fmt,...)
     va_end(args);

     /* trim strings */
-    pg_log(PG_REPORT, "%-*s", MESSAGE_WIDTH, message);
+    pg_log(PG_REPORT_NONL, "%-*s", MESSAGE_WIDTH, message);
 }

 /*
@@ -155,9 +156,9 @@ prep_status_progress(const char *fmt,...)
      * put the individual progress items onto the next line.
      */
     if (log_opts.isatty || log_opts.verbose)
-        pg_log(PG_REPORT, "%-*s\n", MESSAGE_WIDTH, message);
-    else
         pg_log(PG_REPORT, "%-*s", MESSAGE_WIDTH, message);
+    else
+        pg_log(PG_REPORT_NONL, "%-*s", MESSAGE_WIDTH, message);
 }

 static void
@@ -165,6 +166,10 @@ pg_log_v(eLogType type, const char *fmt, va_list ap)
 {
     char        message[QUERY_ALLOC];

+    /* No incoming message should end in newline; we add that here. */
+    Assert(fmt);
+    Assert(fmt[0] == '\0' || fmt[strlen(fmt) - 1] != '\n');
+
     vsnprintf(message, sizeof(message), _(fmt), ap);

     /* PG_VERBOSE and PG_STATUS are only output in verbose mode */
@@ -173,10 +178,12 @@ pg_log_v(eLogType type, const char *fmt, va_list ap)
         log_opts.internal != NULL)
     {
         if (type == PG_STATUS)
-            /* status messages need two leading spaces and a newline */
+            /* status messages get two leading spaces, see below */
             fprintf(log_opts.internal, "  %s\n", message);
-        else
+        else if (type == PG_REPORT_NONL)
             fprintf(log_opts.internal, "%s", message);
+        else
+            fprintf(log_opts.internal, "%s\n", message);
         fflush(log_opts.internal);
     }

@@ -184,45 +191,54 @@ pg_log_v(eLogType type, const char *fmt, va_list ap)
     {
         case PG_VERBOSE:
             if (log_opts.verbose)
-                printf("%s", message);
+                printf("%s\n", message);
             break;

         case PG_STATUS:

             /*
-             * For output to a display, do leading truncation. Append \r so
-             * that the next message is output at the start of the line.
+             * For output to a terminal, we add two leading spaces and no
+             * newline; instead append \r so that the next message is output
+             * on the same line.  Truncate on the left to fit into
+             * MESSAGE_WIDTH (counting the spaces as part of that).
              *
              * If going to non-interactive output, only display progress if
              * verbose is enabled. Otherwise the output gets unreasonably
              * large by default.
              */
             if (log_opts.isatty)
-                /* -2 because we use a 2-space indent */
-                printf("  %s%-*.*s\r",
+            {
+                bool        itfits = (strlen(message) <= MESSAGE_WIDTH - 2);
+
                 /* prefix with "..." if we do leading truncation */
-                       strlen(message) <= MESSAGE_WIDTH - 2 ? "" : "...",
+                printf("  %s%-*.*s\r",
+                       itfits ? "" : "...",
                        MESSAGE_WIDTH - 2, MESSAGE_WIDTH - 2,
-                /* optional leading truncation */
-                       strlen(message) <= MESSAGE_WIDTH - 2 ? message :
+                       itfits ? message :
                        message + strlen(message) - MESSAGE_WIDTH + 3 + 2);
+            }
             else if (log_opts.verbose)
                 printf("  %s\n", message);
             break;

+        case PG_REPORT_NONL:
+            /* This option is for use by prep_status and friends */
+            printf("%s", message);
+            break;
+
         case PG_REPORT:
         case PG_WARNING:
-            printf("%s", message);
+            printf("%s\n", message);
             break;

         case PG_FATAL:
-            printf("\n%s", message);
+            /* Extra newline in case we're interrupting status output */
+            printf("\n%s\n", message);
             printf(_("Failure, exiting\n"));
             exit(1);
             break;

-        default:
-            break;
+            /* No default:, we want a warning for omitted cases */
     }
     fflush(stdout);
 }
@@ -247,6 +263,7 @@ pg_fatal(const char *fmt,...)
     va_start(args, fmt);
     pg_log_v(PG_FATAL, fmt, args);
     va_end(args);
+    /* NOTREACHED */
     printf(_("Failure, exiting\n"));
     exit(1);
 }
@@ -257,7 +274,6 @@ check_ok(void)
 {
     /* all seems well */
     report_status(PG_REPORT, "ok");
-    fflush(stdout);
 }


@@ -307,7 +323,7 @@ get_user_info(char **user_name_p)

     user_name = get_user_name(&errstr);
     if (!user_name)
-        pg_fatal("%s\n", errstr);
+        pg_fatal("%s", errstr);

     /* make a copy */
     *user_name_p = pg_strdup(user_name);
diff --git a/src/bin/pg_upgrade/version.c b/src/bin/pg_upgrade/version.c
index c694558c3d..d2636ba857 100644
--- a/src/bin/pg_upgrade/version.c
+++ b/src/bin/pg_upgrade/version.c
@@ -113,7 +113,7 @@ check_for_data_types_usage(ClusterInfo *cluster,
         {
             found = true;
             if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
-                pg_fatal("could not open file \"%s\": %s\n", output_path,
+                pg_fatal("could not open file \"%s\": %s", output_path,
                          strerror(errno));
             if (!db_used)
             {
@@ -187,14 +187,14 @@ old_9_3_check_for_line_data_type_usage(ClusterInfo *cluster)

     if (check_for_data_type_usage(cluster, "pg_catalog.line", output_path))
     {
-        pg_log(PG_REPORT, "fatal\n");
+        pg_log(PG_REPORT, "fatal");
         pg_fatal("Your installation contains the \"line\" data type in user tables.\n"
                  "This data type changed its internal and input/output format\n"
                  "between your old and new versions so this\n"
                  "cluster cannot currently be upgraded.  You can\n"
                  "drop the problem columns and restart the upgrade.\n"
                  "A list of the problem columns is in the file:\n"
-                 "    %s\n\n", output_path);
+                 "    %s", output_path);
     }
     else
         check_ok();
@@ -225,13 +225,13 @@ old_9_6_check_for_unknown_data_type_usage(ClusterInfo *cluster)

     if (check_for_data_type_usage(cluster, "pg_catalog.unknown", output_path))
     {
-        pg_log(PG_REPORT, "fatal\n");
+        pg_log(PG_REPORT, "fatal");
         pg_fatal("Your installation contains the \"unknown\" data type in user tables.\n"
                  "This data type is no longer allowed in tables, so this\n"
                  "cluster cannot currently be upgraded.  You can\n"
                  "drop the problem columns and restart the upgrade.\n"
                  "A list of the problem columns is in the file:\n"
-                 "    %s\n\n", output_path);
+                 "    %s", output_path);
     }
     else
         check_ok();
@@ -285,7 +285,7 @@ old_9_6_invalidate_hash_indexes(ClusterInfo *cluster, bool check_mode)
             if (!check_mode)
             {
                 if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
-                    pg_fatal("could not open file \"%s\": %s\n", output_path,
+                    pg_fatal("could not open file \"%s\": %s", output_path,
                              strerror(errno));
                 if (!db_used)
                 {
@@ -334,7 +334,7 @@ old_9_6_invalidate_hash_indexes(ClusterInfo *cluster, bool check_mode)
                    "Your installation contains hash indexes.  These indexes have different\n"
                    "internal formats between your old and new clusters, so they must be\n"
                    "reindexed with the REINDEX command.  After upgrading, you will be given\n"
-                   "REINDEX instructions.\n\n");
+                   "REINDEX instructions.");
         else
             pg_log(PG_WARNING, "\n"
                    "Your installation contains hash indexes.  These indexes have different\n"
@@ -342,7 +342,7 @@ old_9_6_invalidate_hash_indexes(ClusterInfo *cluster, bool check_mode)
                    "reindexed with the REINDEX command.  The file\n"
                    "    %s\n"
                    "when executed by psql by the database superuser will recreate all invalid\n"
-                   "indexes; until then, none of these indexes will be used.\n\n",
+                   "indexes; until then, none of these indexes will be used.",
                    output_path);
     }
     else
@@ -369,13 +369,13 @@ old_11_check_for_sql_identifier_data_type_usage(ClusterInfo *cluster)
     if (check_for_data_type_usage(cluster, "information_schema.sql_identifier",
                                   output_path))
     {
-        pg_log(PG_REPORT, "fatal\n");
+        pg_log(PG_REPORT, "fatal");
         pg_fatal("Your installation contains the \"sql_identifier\" data type in user tables.\n"
                  "The on-disk format for this data type has changed, so this\n"
                  "cluster cannot currently be upgraded.  You can\n"
                  "drop the problem columns and restart the upgrade.\n"
                  "A list of the problem columns is in the file:\n"
-                 "    %s\n\n", output_path);
+                 "    %s", output_path);
     }
     else
         check_ok();
@@ -420,7 +420,7 @@ report_extension_updates(ClusterInfo *cluster)
             found = true;

             if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
-                pg_fatal("could not open file \"%s\": %s\n", output_path,
+                pg_fatal("could not open file \"%s\": %s", output_path,
                          strerror(errno));
             if (!db_used)
             {
@@ -452,7 +452,7 @@ report_extension_updates(ClusterInfo *cluster)
                "with the ALTER EXTENSION command.  The file\n"
                "    %s\n"
                "when executed by psql by the database superuser will update\n"
-               "these extensions.\n\n",
+               "these extensions.",
                output_path);
     }
     else

Re: Remove trailing newlines from pg_upgrade's messages

From
Kyotaro Horiguchi
Date:
At Tue, 14 Jun 2022 14:57:40 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> Per the discussion at [1], pg_upgrade currently doesn't use
> common/logging.c's functions.   Making it do so looks like a
> bigger lift than is justified, but there is one particular
> inconsistency that I think we ought to remove: pg_upgrade
> expects (most) message strings to end in newlines, while logging.c
> expects them not to.  This is bad for a couple of reasons:
> 
> * Translatable strings that otherwise could be shared with other
> code are different.

Yes. Also it is annoying that we need to care about ending new lines..

> * Developers might mistakenly add or leave off a newline because of
> familiarity with how it's done elsewhere.  This is especially bad for
> pg_fatal() which is otherwise caller-compatible with the version
> provided by logging.c.  We fixed a couple of bugs of exactly that
> description recently, and I found a few more as I went through
> pg_upgrade for the attached patch.  It doesn't help any that as it
> stands, pg_upgrade requires some messages to end in newline and
> others not: there are some places that are adding an extra newline,
> apparently because whoever coded them was confused about which
> convention applied.
> 
> Hence, the patch below removes trailing newlines from all of
> pg_upgrade's message strings, and teaches its logging infrastructure
> to print them where appropriate.  As in logging.c, there's now an
> Assert that no format string passed to pg_log() et al ends with
> a newline.

I think it's the least-bad way to control ending newline.

-    PG_STATUS,
+    PG_STATUS,                    /* these messages do not get a newline added */

Really?

+    PG_REPORT_NONL,                /* these too */
     PG_REPORT,

> This doesn't quite exactly match the code's prior behavior.  Aside
> from the buggy-looking newlines mentioned above, there are a few
> messages that formerly ended with a double newline, thus intentionally
> producing a blank line, and now they don't.  I could have removed just
> one of their newlines, but I'd have had to give up the Assert about
> it, and I did not think that the extra blank lines were important
> enough to justify that.

I don't think traling double-newlines for pg_fatal is useful so I
agree to you in this point.

Also leading newlines and just "\n" bug me when I edit message
catalogues with poedit. I might want a line-spacing function like
pg_log_newline(PG_REPORT) if we need line-breaks in the ends of a
message.

> BTW, as I went through the code I realized just how badly pg_upgrade
> needs a visit from the message style police.  Its messages are not
> even consistent with each other, let alone with our message style
> guidelines.  I have refrained (mostly) from doing any re-wording
> here, but it could stand to be done.

A bit apart from this, I experince a bit hard time to find an
appropriate translation for "Your installation", which I finally
translate them into (a literal translation of ) "This cluster" or
such..

> I'll stick this in the CF queue, but I wonder if there is any case
> for squeezing it into v15 instead of waiting for v16.

I think we can as it doen't seem to make functional change. But I
haven't checked if the patch doesn't break anything..

>             regards, tom lane
> 
> [1] https://www.postgresql.org/message-id/4036037.1655174501%40sss.pgh.pa.us
> 

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



Re: Remove trailing newlines from pg_upgrade's messages

From
Kyotaro Horiguchi
Date:
By the way, I noticed that pg_upgrade complains wrong way when the
specified binary path doesn't contain "postgres" file.

$ pg_upgrade -b /tmp -B /tmp -d /tmp -D /tmp

check for "/tmp/postgres" failed: not a regular file
Failure, exiting

I think it should be a quite common mistake to specify the parent
directory of the binary directory..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



Re: Remove trailing newlines from pg_upgrade's messages

From
Kyotaro Horiguchi
Date:
At Wed, 15 Jun 2022 13:05:52 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> By the way, I noticed that pg_upgrade complains wrong way when the
> specified binary path doesn't contain "postgres" file.
> 
> $ pg_upgrade -b /tmp -B /tmp -d /tmp -D /tmp
> 
> check for "/tmp/postgres" failed: not a regular file
> Failure, exiting
> 
> I think it should be a quite common mistake to specify the parent
> directory of the binary directory..

FWIW, the following change makes sense to me according to the spec of
validate_exec()...

diff --git a/src/bin/pg_upgrade/exec.c b/src/bin/pg_upgrade/exec.c
index fadeea12ca..3cff186213 100644
--- a/src/bin/pg_upgrade/exec.c
+++ b/src/bin/pg_upgrade/exec.c
@@ -430,10 +430,10 @@ check_exec(const char *dir, const char *program, bool check_version)
     ret = validate_exec(path);
 
     if (ret == -1)
-        pg_fatal("check for \"%s\" failed: not a regular file\n",
+        pg_fatal("check for \"%s\" failed: does not exist or inexecutable\n",
                  path);
     else if (ret == -2)
-        pg_fatal("check for \"%s\" failed: cannot execute (permission denied)\n",
+        pg_fatal("check for \"%s\" failed: cannot read (permission denied)\n",
                  path);
 
     snprintf(cmd, sizeof(cmd), "\"%s\" -V", path);

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



Re: Remove trailing newlines from pg_upgrade's messages

From
Peter Eisentraut
Date:
On 14.06.22 20:57, Tom Lane wrote:
> I'll stick this in the CF queue, but I wonder if there is any case
> for squeezing it into v15 instead of waiting for v16.

Let's stick this into 16 and use it as a starting point of tidying all 
this up in pg_upgrade.



Re: Remove trailing newlines from pg_upgrade's messages

From
Tom Lane
Date:
Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> Also leading newlines and just "\n" bug me when I edit message
> catalogues with poedit. I might want a line-spacing function like
> pg_log_newline(PG_REPORT) if we need line-breaks in the ends of a
> message.

Yeah, that is sort of the inverse problem.  I think those are there
to ensure that the text appears on a fresh line even if the current
line has transient status on it.  We could get rid of those perhaps
if we teach pg_log_v to remember whether it ended the last output
with a newline or not, and then put out a leading newline only if
necessary, rather than hard-wiring one into the message texts.

This might take a little bit of fiddling to make it work, because
we'd not want the extra newline when completing an incomplete line
by adding status.  That would mean that report_status would have
to do something special, plus we'd have to be sure that all such
cases do go through report_status rather than calling pg_log
directly.  (I'm fairly sure that the code is sloppy about that
today :-(.)  It seems probably do-able, though.

            regards, tom lane



Re: Remove trailing newlines from pg_upgrade's messages

From
Greg Stark
Date:
On Wed, 15 Jun 2022 at 11:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Yeah, that is sort of the inverse problem.  I think those are there
> to ensure that the text appears on a fresh line even if the current
> line has transient status on it.  We could get rid of those perhaps
> if we teach pg_log_v to remember whether it ended the last output
> with a newline or not, and then put out a leading newline only if
> necessary, rather than hard-wiring one into the message texts.

Is the problem that pg_upgrade doesn't know what the utilities it's
calling are outputting to the same terminal?

Another thing I wonder is if during development and testing there
might have been more output from utilities or even the backend going
on that are
not happening now.

-- 
greg



Re: Remove trailing newlines from pg_upgrade's messages

From
Tom Lane
Date:
Greg Stark <stark@mit.edu> writes:
> On Wed, 15 Jun 2022 at 11:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Yeah, that is sort of the inverse problem.  I think those are there
>> to ensure that the text appears on a fresh line even if the current
>> line has transient status on it.  We could get rid of those perhaps
>> if we teach pg_log_v to remember whether it ended the last output
>> with a newline or not, and then put out a leading newline only if
>> necessary, rather than hard-wiring one into the message texts.

> Is the problem that pg_upgrade doesn't know what the utilities it's
> calling are outputting to the same terminal?

Hmmm ... that's a point I'd not considered, but I think it's not an
issue here.  The subprograms generally have their output redirected
to their own log files.

            regards, tom lane



Re: Remove trailing newlines from pg_upgrade's messages

From
Peter Eisentraut
Date:
On 14.06.22 20:57, Tom Lane wrote:
> Hence, the patch below removes trailing newlines from all of
> pg_upgrade's message strings, and teaches its logging infrastructure
> to print them where appropriate.  As in logging.c, there's now an
> Assert that no format string passed to pg_log() et al ends with
> a newline.

This patch looks okay to me.  I compared the output before and after in 
a few scenarios and didn't see any problematic differences.

> This doesn't quite exactly match the code's prior behavior.  Aside
> from the buggy-looking newlines mentioned above, there are a few
> messages that formerly ended with a double newline, thus intentionally
> producing a blank line, and now they don't.  I could have removed just
> one of their newlines, but I'd have had to give up the Assert about
> it, and I did not think that the extra blank lines were important
> enough to justify that.

In this particular patch, the few empty lines that disappeared don't 
bother me.  In general, however, I think we can just fprintf(stderr, 
"\n") directly as necessary.



Re: Remove trailing newlines from pg_upgrade's messages

From
Tom Lane
Date:
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> On 14.06.22 20:57, Tom Lane wrote:
>> Hence, the patch below removes trailing newlines from all of
>> pg_upgrade's message strings, and teaches its logging infrastructure
>> to print them where appropriate.  As in logging.c, there's now an
>> Assert that no format string passed to pg_log() et al ends with
>> a newline.

> This patch looks okay to me.  I compared the output before and after in 
> a few scenarios and didn't see any problematic differences.

Thanks, pushed after rebasing and adjusting some recently-added messages.

> In this particular patch, the few empty lines that disappeared don't 
> bother me.  In general, however, I think we can just fprintf(stderr, 
> "\n") directly as necessary.

Hmm, if anyone wants to do that I think it'd be advisable to invent
"pg_log_blank_line()" or something like that, so as to preserve the
logging abstraction layer.  But it's moot unless anyone's interested
enough to send a patch for that.  I'm not.

(I think it *would* be a good idea to try to get rid of the leading
newlines that appear in some of the messages, as discussed upthread.
But I'm not going to trouble over that right now either.)

            regards, tom lane



Re: Remove trailing newlines from pg_upgrade's messages

From
Tom Lane
Date:
Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> FWIW, the following change makes sense to me according to the spec of
> validate_exec()...

> diff --git a/src/bin/pg_upgrade/exec.c b/src/bin/pg_upgrade/exec.c
> index fadeea12ca..3cff186213 100644
> --- a/src/bin/pg_upgrade/exec.c
> +++ b/src/bin/pg_upgrade/exec.c
> @@ -430,10 +430,10 @@ check_exec(const char *dir, const char *program, bool check_version)
>      ret = validate_exec(path);

>      if (ret == -1)
> -        pg_fatal("check for \"%s\" failed: not a regular file\n",
> +        pg_fatal("check for \"%s\" failed: does not exist or inexecutable\n",
>                   path);
>      else if (ret == -2)
> -        pg_fatal("check for \"%s\" failed: cannot execute (permission denied)\n",
> +        pg_fatal("check for \"%s\" failed: cannot read (permission denied)\n",
>                   path);

>      snprintf(cmd, sizeof(cmd), "\"%s\" -V", path);

I initially did this, but then I wondered why validate_exec() was
making it so hard: why can't we just report the failure with %m?
It turns out to take only a couple extra lines of code to ensure
that something more-or-less appropriate is returned, so we don't
need to guess about it here.  Pushed that way.

            regards, tom lane