Giving more detail in pg_upgrade errormessage
Looking at the upgrade question in [0]CACoPQdbQTysF=EKckyFNGTdpOdXXMEsf_2ACno+bcNqQCB5raA@mail.gmail.com made me realize that we discard
potentially useful information for troubleshooting. When we check if the
cluster is properly shut down we might as well include the status from
pg_controldata in the errormessage as per the trivial (but yet untested)
proposed diff.
Is there a reason not to be verbose here as users might copy/paste this output
when asking for help?
--
Daniel Gustafsson
[0]: CACoPQdbQTysF=EKckyFNGTdpOdXXMEsf_2ACno+bcNqQCB5raA@mail.gmail.com
Attachments:
pg_upgrade_controldata.diffapplication/octet-stream; name=pg_upgrade_controldata.diff; x-unix-mode=0644Download
diff --git a/src/bin/pg_upgrade/controldata.c b/src/bin/pg_upgrade/controldata.c
index 9071a6fd45..29f1153340 100644
--- a/src/bin/pg_upgrade/controldata.c
+++ b/src/bin/pg_upgrade/controldata.c
@@ -162,9 +162,9 @@ get_control_data(ClusterInfo *cluster, bool live_check)
else if (strcmp(p, "shut down\n") != 0)
{
if (cluster == &old_cluster)
- pg_fatal("The source cluster was not shut down cleanly.");
+ pg_fatal("The source cluster was not shut down cleanly, state reported as: \"%s\"", p);
else
- pg_fatal("The target cluster was not shut down cleanly.");
+ pg_fatal("The target cluster was not shut down cleanly, state reported as: \"%s\"", p);
}
got_cluster_state = true;
}
Hi,
Is there a reason not to be verbose here as users might copy/paste this output
when asking for help?
Seems better than nothing.
[0] CACoPQdbQTysF=EKckyFNGTdpOdXXMEsf_2ACno+bcNqQCB5raA@mail.gmail.com
Full link for convenience.
[0]: /messages/by-id/CACoPQdbQTysF=EKckyFNGTdpOdXXMEsf_2ACno+bcNqQCB5raA@mail.gmail.com
Zhang Mingli
https://www.hashdata.xyz
Daniel Gustafsson <daniel@yesql.se> writes:
Looking at the upgrade question in [0] made me realize that we discard
potentially useful information for troubleshooting. When we check if the
cluster is properly shut down we might as well include the status from
pg_controldata in the errormessage as per the trivial (but yet untested)
proposed diff.
Is there a reason not to be verbose here as users might copy/paste this output
when asking for help?
Agreed, but I think you need to chomp the string's trailing newline,
or it'll look ugly. You might as well do that further up and remove
the newlines from the comparison strings, too.
regards, tom lane
On 18 Jul 2023, at 18:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Daniel Gustafsson <daniel@yesql.se> writes:
Looking at the upgrade question in [0] made me realize that we discard
potentially useful information for troubleshooting. When we check if the
cluster is properly shut down we might as well include the status from
pg_controldata in the errormessage as per the trivial (but yet untested)
proposed diff.Is there a reason not to be verbose here as users might copy/paste this output
when asking for help?Agreed, but I think you need to chomp the string's trailing newline,
or it'll look ugly. You might as well do that further up and remove
the newlines from the comparison strings, too.
Yeah, the previous diff was mostly a sketch. The attached strips newline and
makes the comparisons a bit neater in the process due to that. Will apply this
trivial but seemingly useful change unless objected to.
--
Daniel Gustafsson
Attachments:
v2-0001-pg_upgrade-include-additional-detail-in-cluster-c.patchapplication/octet-stream; name=v2-0001-pg_upgrade-include-additional-detail-in-cluster-c.patch; x-unix-mode=0644Download
From a7721e56d69d8de631a0d6689059097d17380304 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Wed, 19 Jul 2023 22:14:42 +0200
Subject: [PATCH v2] pg_upgrade: include additional detail in cluster check
If the cluster fails the pg_controldata check for clean shut down
we only reported that it did so, not why. Fix by including the
state reported by pg_controldata in the error message to assist
the user in debugging the upgrade.
Discussion: https://postgr.es/m/E0D5EA16-A085-4753-8DDC-C7055048B439@yesql.se
---
src/bin/pg_upgrade/controldata.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/src/bin/pg_upgrade/controldata.c b/src/bin/pg_upgrade/controldata.c
index 9071a6fd45..4beb65ab22 100644
--- a/src/bin/pg_upgrade/controldata.c
+++ b/src/bin/pg_upgrade/controldata.c
@@ -149,22 +149,23 @@ get_control_data(ClusterInfo *cluster, bool live_check)
* the server was shut down cleanly, from the controldata
* perspective.
*/
- /* remove leading spaces */
+ /* Remove trailing newline and leading spaces */
+ (void) pg_strip_crlf(p);
while (*p == ' ')
p++;
- if (strcmp(p, "shut down in recovery\n") == 0)
+ if (strcmp(p, "shut down in recovery") == 0)
{
if (cluster == &old_cluster)
pg_fatal("The source cluster was shut down while in recovery mode. To upgrade, use \"rsync\" as documented or shut it down as a primary.");
else
pg_fatal("The target cluster was shut down while in recovery mode. To upgrade, use \"rsync\" as documented or shut it down as a primary.");
}
- else if (strcmp(p, "shut down\n") != 0)
+ else if (strcmp(p, "shut down") != 0)
{
if (cluster == &old_cluster)
- pg_fatal("The source cluster was not shut down cleanly.");
+ pg_fatal("The source cluster was not shut down cleanly, state reported as: \"%s\"", p);
else
- pg_fatal("The target cluster was not shut down cleanly.");
+ pg_fatal("The target cluster was not shut down cleanly, state reported as: \"%s\"", p);
}
got_cluster_state = true;
}
--
2.32.1 (Apple Git-133)