Feedback-ID: ia2694551:Fastmail
Date: Thu, 3 Jul 2025 20:53:01 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Michael Paquier <michael@paquier.xyz>, Ali Akbar <the.apaan@gmail.com>,
	Michael Paquier <michael.paquier@gmail.com>,
 Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>,
	tushar <tushar.ahuja@enterprisedb.com>, Tom Lane <tgl@sss.pgh.pa.us>,
 pgsql-hackers@postgresql.org,
	rmt@lists.postgresql.org
Subject: Re: [HACKERS] pg_upgrade failed with error - ERROR: column "a" in
 child table must be marked NOT NULL
Message-ID: <202507031853.jfbuuompu5to@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="oqway6q7hqrlu7xw"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ZiF8MCeZOAbC6710@pryzbyj2023>
Archived-At: 
 <https://www.postgresql.org/message-id/202507031853.jfbuuompu5to%40alvherre.pgsql>
Precedence: bulk


--oqway6q7hqrlu7xw
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

Hello,

We seem to have forgotten about this issue.  I think this is even more
pressing with the changes to 18 for not-null constraints, though
strictly speaking it's always been a problem.  Here's an updated patch.
I simplified the query (no need for a recursive CTE as far as I can
tell) and updated to use the pg_upgrade "task" infrastructure.

I think from 18 on, the problem can no longer be recreated, since
removing the attnotnull bit from a child table is not possible.

I should add some testing too before pushing, of course.  I only tested
manually.

I'd like to hear from RMT about getting this in 18.  Actually I think we
should consider backporting to all live versions ... thoughts?

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Investigación es lo que hago cuando no sé lo que estoy haciendo"
(Wernher von Braun)

--oqway6q7hqrlu7xw
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="0001-pg_upgrade-check-for-inconsistent-inherited-not-null.patch"

From 91142cc750b041bb8a0ed28004df3b609d70bd0d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Thu, 3 Jul 2025 20:14:27 +0200
Subject: [PATCH] pg_upgrade: check for inconsistent inherited not null columns

Otherwise, the upgrade can fail halfway through with errors with tables
like these:
  CREATE TABLE ip (id int PRIMARY KEY);
  CREATE TABLE ic(id int) INHERITS (ip);
  ALTER TABLE ic ALTER id DROP NOT NULL;

ERROR: column "a" in child table must be marked NOT NULL

Author: Ali Akbar <the.apaan@gmail.com>
Reviewed-by: Justin Pryzby <pryzby@telsasoft.com>
Discussion: https://postgr.es/m/CACQjQLoMsE+1pyLe98pi0KvPG2jQQ94LWJ+PTiLgVRK4B=i_jg@mail.gmail.com
---
 src/bin/pg_upgrade/check.c | 91 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)

diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index fb063a2de42..7f23175d662 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -31,6 +31,7 @@ static void check_new_cluster_logical_replication_slots(void);
 static void check_new_cluster_subscription_configuration(void);
 static void check_old_cluster_for_valid_slots(void);
 static void check_old_cluster_subscription_state(void);
+static void check_for_not_null_inheritance(ClusterInfo *cluster);
 
 /*
  * DataTypesUsageChecks - definitions of data type checks for the old cluster
@@ -672,6 +673,8 @@ check_and_dump_old_cluster(void)
 	if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1100)
 		check_for_tables_with_oids(&old_cluster);
 
+	check_for_not_null_inheritance(&old_cluster);
+
 	/*
 	 * Pre-PG 10 allowed tables with 'unknown' type columns and non WAL logged
 	 * hash indexes
@@ -1943,6 +1946,94 @@ check_for_unicode_update(ClusterInfo *cluster)
 		check_ok();
 }
 
+/*
+ * Callback function for processing results of query for
+ * check_for_not_null_inheritance.
+ */
+static void
+process_inconsistent_notnull(DbInfo *dbinfo, PGresult *res, void *arg)
+{
+	UpgradeTaskReport *report = (UpgradeTaskReport *) arg;
+	int			ntups = PQntuples(res);
+	int			i_nspname = PQfnumber(res, "nspname");
+	int			i_relname = PQfnumber(res, "relname");
+	int			i_attname = PQfnumber(res, "attname");
+
+	AssertVariableIsOfType(&process_inconsistent_notnull,
+						   UpgradeTaskProcessCB);
+
+	if (ntups == 0)
+		return;
+
+	if (report->file == NULL &&
+		(report->file = fopen_priv(report->path, "w")) == NULL)
+		pg_fatal("could not open file \"%s\": %m", report->path);
+
+	fprintf(report->file, "In database: %s\n", dbinfo->db_name);
+
+	for (int rowno = 0; rowno < ntups; rowno++)
+	{
+		fprintf(report->file, "  %s.%s.%s\n",
+				PQgetvalue(res, rowno, i_nspname),
+				PQgetvalue(res, rowno, i_relname),
+				PQgetvalue(res, rowno, i_attname));
+	}
+}
+
+/*
+ * check_for_not_null_inheritance()
+ *
+ * An attempt to create child tables lacking not-null constraints that are
+ * present in their parents errors out.  This can no longer occur since 18,
+ * but previously there were various ways for that to happen.  Check that
+ * the cluster to be upgraded doesn't have any of those problems.
+ */
+static void
+check_for_not_null_inheritance(ClusterInfo *cluster)
+{
+	UpgradeTaskReport report;
+	UpgradeTask *task;
+	const char *query;
+
+	prep_status("Checking for not-null constraint inconsistencies");
+
+	report.file = NULL;
+	snprintf(report.path, sizeof(report.path), "%s/%s",
+			 log_opts.basedir,
+			 "not_null_inconsistent_columns.txt");
+
+	query = "SELECT cc.relnamespace::pg_catalog.regnamespace AS nspname, "
+		"       cc.relname, ac.attname "
+		"FROM pg_catalog.pg_inherits i, pg_catalog.pg_attribute ac, "
+		"     pg_catalog.pg_attribute ap, pg_catalog.pg_class cc "
+		"WHERE cc.oid = ac.attrelid AND i.inhrelid = ac.attrelid "
+		"      AND i.inhparent = ap.attrelid AND ac.attname = ap.attname "
+		"      AND ap.attnum > 0 and ap.attnotnull AND NOT ac.attnotnull";
+
+	task = upgrade_task_create();
+	upgrade_task_add_step(task, query,
+						  process_inconsistent_notnull,
+						  true, &report);
+	upgrade_task_run(task, cluster);
+	upgrade_task_free(task);
+
+	if (report.file)
+	{
+		fclose(report.file);
+		pg_log(PG_REPORT, "fatal");
+		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);
+	}
+	else
+		check_ok();
+}
+
+
 /*
  * check_new_cluster_logical_replication_slots()
  *
-- 
2.39.5


--oqway6q7hqrlu7xw--