Feedback-ID: ie3de48e3:Fastmail
Date: Fri, 4 Jul 2025 18:20:37 +0200
From: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Michael Paquier <michael.paquier@gmail.com>,
	Tom Lane <tgl@sss.pgh.pa.us>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>,
	Ali Akbar <the.apaan@gmail.com>
Subject: Re: [HACKERS] pg_upgrade failed with error - ERROR: column "a" in
 child table must be marked NOT NULL
Message-ID: <202507041620.muotfjk37hqe@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20171214054902.GC23527@telsasoft.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202507041620.muotfjk37hqe%40alvherre.pgsql>
Precedence: bulk

Two more minor things on this:

I found it a bit silly to mention the ALTER command in the error message
and then output only the three-part-qualified name in the output file
(and no specific command to run) -- so for an instant I was tempted to
write the full ALTER TABLE command in the output file instead.  However
I then noticed that check.c does not use fmtId() or fmtQualifiedId()
anywhere, so that code would look out of place in check.c and perhaps
even be a problem for backpatching.  Do people thing it'd be an
improvement to change it that way?

On 2017-Dec-13, Justin Pryzby wrote:

> Here are some spelling and grammar fixes to that patch:
> 
> but nullabe in its children: nullable
> child column is not market: marked
> with adding: by adding
> and restart: and restarting
> the problem columns: the problematic columns
> 9.5, 9.6, 10: 9.5, 9.6, and 10
> restore, that will cause error.: restore phase of pg_upgrade, that would cause an error.

So the phrase "the problem columns" appears in multiple places in
pg_upgrade/check.c.  Should we fix them all?  It turns out that I
rewrote the message as submitted into completely different wording
(without realizing that this had been reported), so these words don't
appear anymore in the new commit.  But we could fix these old ones while
we're here -- yes?  (See below)

My rewrite is this:
       pg_fatal("Your installation contains inconsistent NOT NULL constraints.\n"
                "If the parent column(s) are NOT NULL, then the child column must\n"
                "also be marked NOT NULL, or the upgrade will fail.\n"
                "You can fix this by running\n"
                "  ALTER TABLE tablename ALTER column SET NOT NULL;\n"
                "on each column listed in the file:\n"
                "    %s", report.path);


Here's what a patch looks like to change "problem columns" to
"problematic columns".

diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 2106869b999..ad80f00c055 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -119,7 +119,7 @@ static DataTypesUsageChecks data_types_usage_checks[] =
 		gettext_noop("Your installation contains system-defined composite types in user tables.\n"
 					 "These type OIDs are not stable across PostgreSQL versions,\n"
 					 "so this cluster cannot currently be upgraded.  You can drop the\n"
-					 "problem columns and restart the upgrade.\n"),
+					 "problematic columns and restart the upgrade.\n"),
 		.threshold_version = ALL_VERSIONS
 	},
 
@@ -139,7 +139,7 @@ static DataTypesUsageChecks data_types_usage_checks[] =
 					 "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"),
+					 "drop the problematic columns and restart the upgrade.\n"),
 		.threshold_version = 903
 	},
 
@@ -183,7 +183,7 @@ static DataTypesUsageChecks data_types_usage_checks[] =
 		gettext_noop("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"),
+					 "drop the problematic columns and restart the upgrade.\n"),
 		.threshold_version = ALL_VERSIONS
 	},
 
@@ -200,7 +200,7 @@ static DataTypesUsageChecks data_types_usage_checks[] =
 		gettext_noop("Your installation contains the \"aclitem\" data type in user tables.\n"
 					 "The internal format of \"aclitem\" changed in PostgreSQL version 16\n"
 					 "so this cluster cannot currently be upgraded.  You can drop the\n"
-					 "problem columns and restart the upgrade.\n"),
+					 "problematic columns and restart the upgrade.\n"),
 		.threshold_version = 1500
 	},
 
@@ -223,7 +223,7 @@ static DataTypesUsageChecks data_types_usage_checks[] =
 		.report_text =
 		gettext_noop("Your installation contains the \"unknown\" data type in user tables.\n"
 					 "This data type is no longer allowed in tables, so this cluster\n"
-					 "cannot currently be upgraded.  You can drop the problem columns\n"
+					 "cannot currently be upgraded.  You can drop the problematic columns\n"
 					 "and restart the upgrade.\n"),
 		.threshold_version = 906
 	},
@@ -279,7 +279,7 @@ static DataTypesUsageChecks data_types_usage_checks[] =
 		gettext_noop("Your installation contains the \"abstime\" data type in user tables.\n"
 					 "The \"abstime\" type has been removed in PostgreSQL version 12,\n"
 					 "so this cluster cannot currently be upgraded.  You can drop the\n"
-					 "problem columns, or change them to another data type, and restart\n"
+					 "problematic columns, or change them to another data type, and restart\n"
 					 "the upgrade.\n"),
 		.threshold_version = 1100
 	},
@@ -292,7 +292,7 @@ static DataTypesUsageChecks data_types_usage_checks[] =
 		gettext_noop("Your installation contains the \"reltime\" data type in user tables.\n"
 					 "The \"reltime\" type has been removed in PostgreSQL version 12,\n"
 					 "so this cluster cannot currently be upgraded.  You can drop the\n"
-					 "problem columns, or change them to another data type, and restart\n"
+					 "problematic columns, or change them to another data type, and restart\n"
 					 "the upgrade.\n"),
 		.threshold_version = 1100
 	},
@@ -305,7 +305,7 @@ static DataTypesUsageChecks data_types_usage_checks[] =
 		gettext_noop("Your installation contains the \"tinterval\" data type in user tables.\n"
 					 "The \"tinterval\" type has been removed in PostgreSQL version 12,\n"
 					 "so this cluster cannot currently be upgraded.  You can drop the\n"
-					 "problem columns, or change them to another data type, and restart\n"
+					 "problematic columns, or change them to another data type, and restart\n"
 					 "the upgrade.\n"),
 		.threshold_version = 1100
 	},
@@ -423,7 +423,7 @@ process_data_type_check(DbInfo *dbinfo, PGresult *res, void *arg)
 		pg_log(PG_REPORT, "failed check: %s", _(state->check->status));
 		appendPQExpBuffer(*state->report, "\n%s\n%s    %s\n",
 						  _(state->check->report_text),
-						  _("A list of the problem columns is in the file:"),
+						  _("A list of the problematic columns is in the file:"),
 						  output_path);
 	}
 	state->result = true;

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Ed is the standard text editor."
      http://groups.google.com/group/alt.religion.emacs/msg/8d94ddab6a9b0ad3