Date: Thu, 18 Apr 2024 15:01:52 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: 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,
	Alvaro Herrera <alvherre@alvh.no-ip.org>
Subject: Re: [HACKERS] pg_upgrade failed with error - ERROR: column "a" in
 child table must be marked NOT NULL
Message-ID: <ZiF8MCeZOAbC6710@pryzbyj2023>
References: 
 <CAB7nPqStji75D1M=DEkpOXsJbF151m=ZLrJ4jXTh-Czk3kKZ1Q@mail.gmail.com>
 <CACQjQLrN88BrpUAFv-soW+bCnDEkkWGc+z6cmSEMfGMpd+Aa=g@mail.gmail.com>
 <107ccc78-1a10-ff59-e837-e5c99dc8c152@lab.ntt.co.jp>
 <CACQjQLoMsE+1pyLe98pi0KvPG2jQQ94LWJ+PTiLgVRK4B=i_jg@mail.gmail.com>
 <20171214054902.GC23527@telsasoft.com>
 <CAB7nPqRgRbMWLaidxKW+n144kT+3xRfTSqNS8OUaV77FuQo48w@mail.gmail.com>
 <CACQjQLqoXTzCV4+-s1XOT62tY8ggkZkH4kY03gm2aQYOMXE+SA@mail.gmail.com>
 <ZRXGAaOvamcRjoL+@telsasoft.com>
 <ZRYXY3WMVep8yEgq@paquier.xyz>
 <ZRb9GugoMk74CAn+@pryzbyj2023>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="f9RqhN/xbOGzZVv+"
Content-Disposition: inline
In-Reply-To: <ZRb9GugoMk74CAn+@pryzbyj2023>
Archived-At: 
 <https://www.postgresql.org/message-id/ZiF8MCeZOAbC6710%40pryzbyj2023>
Precedence: bulk


--f9RqhN/xbOGzZVv+
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Thu, Dec 14, 2017 at 08:51:06AM +0700, Ali Akbar wrote:
> Patch for adding check in pg_upgrade.

[...]

On Fri, Sep 29, 2023 at 11:36:42AM -0500, Justin Pryzby wrote:
> On Fri, Sep 29, 2023 at 09:16:35AM +0900, Michael Paquier wrote:
> > You mean when upgrading from an instance of 9.6 or older as c30f177 is
> > not there, right?
> 
> No - while upgrading from v15 to v16.  I'm not clear on how we upgraded
> *to* v15 without hitting the issue, nor how the "not null" got
> dropped...
> 
> > Anyway, it seems like the patch from [1] has no need to run this check
> > when the old cluster's version is 10 or newer.  And perhaps it should
> > mention that this check could be removed from pg_upgrade once v10
> > support is out of scope, in the shape of a comment.
> 
> You're still thinking of PRIMARY KEY as the only way to hit this, right?
> But Ali Akbar already pointed out how to reproduce the problem with DROP
> NOT NULL - which still applies to both v16 and v17.

Rebased and amended the forgotten patch.
See also ZiE3NoY6DdvlvFl9@pryzbyj2023 et seq.

--f9RqhN/xbOGzZVv+
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="0001-pg_upgrade-check-for-inconsistent-inherited-not-null.patch"

From f804bec39acac0b53e29563be9ef7a10237ba717 Mon Sep 17 00:00:00 2001
From: Ali Akbar <the.apaan@gmail.com>
Date: Thu, 14 Dec 2017 19:18:59 +0700
Subject: [PATCH] pg_upgrade: check for inconsistent inherited not null columns

Otherwise, the upgrade can fail halfway through with error:
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

JTP: updated to handle contype='n' - otherwise, the cnn_grandchild2
added at b0e96f311 triggers the issue this patch tries to avoid ...

Should be backpatched to all versions.
---
 src/bin/pg_upgrade/check.c | 101 +++++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index ad1f8e3bcb1..79671a9cdb4 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(bool live_check);
 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
@@ -646,6 +647,8 @@ check_and_dump_old_cluster(bool live_check)
 	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
@@ -1832,6 +1835,104 @@ check_new_cluster_subscription_configuration(void)
 	check_ok();
 }
 
+/*
+ * check_for_not_null_inheritance()
+ *
+ *  Currently pg_upgrade will fail if there are any inconsistencies in NOT NULL
+ *  constraint inheritance: In PostgreSQL, we can have a column that is NOT NULL
+ *  in parent, but nullable in its children. So check for inconsistent NOT NULL
+ *  constraints in inheritance tree.
+ */
+static void
+check_for_not_null_inheritance(ClusterInfo *cluster)
+{
+	int			dbnum;
+	FILE	   *script = NULL;
+	char		output_path[MAXPGPATH];
+
+	prep_status("Checking for NOT NULL inconsistencies in inheritance");
+
+	snprintf(output_path, sizeof(output_path), "not_null_inconsistent_columns.txt");
+
+	for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
+	{
+		PGresult   *res;
+		bool		db_used = false;
+		int			ntups;
+		int			rowno;
+		int			i_nspname,
+					i_relname,
+					i_attname;
+		DbInfo	   *active_db = &cluster->dbarr.dbs[dbnum];
+		PGconn	   *conn = connectToServer(cluster, active_db->db_name);
+
+		res = executeQueryOrDie(conn,
+								"WITH RECURSIVE parents AS ( "
+								"	SELECT	i.inhrelid, i.inhparent "
+								"	FROM	pg_catalog.pg_inherits i "
+								"	UNION ALL "
+								"	SELECT	p.inhrelid, i.inhparent "
+								"   FROM	parents p "
+								"	JOIN	pg_catalog.pg_inherits i "
+								"		ON	i.inhrelid = p.inhparent "
+								") "
+								"SELECT	n.nspname, c.relname, ac.attname  "
+								"FROM	parents p, "
+								"		pg_catalog.pg_attribute ac, "
+								"		pg_catalog.pg_attribute ap, "
+								"		pg_catalog.pg_constraint cc, "
+								"		pg_catalog.pg_class c, "
+								"		pg_catalog.pg_namespace n "
+								"WHERE	NOT ac.attnotnull AND "
+								"		ac.attrelid = p.inhrelid AND "
+								"		ap.attrelid = p.inhparent AND "
+								"		ac.attname = ap.attname AND "
+								"		ac.attrelid = cc.conrelid AND ac.attnum = ANY(cc.conkey) AND contype='n' AND NOT connoinherit AND"
+								"		ap.attnotnull AND "
+								"		c.oid = ac.attrelid AND "
+								"		c.relnamespace = n.oid");
+
+		ntups = PQntuples(res);
+		i_nspname = PQfnumber(res, "nspname");
+		i_relname = PQfnumber(res, "relname");
+		i_attname = PQfnumber(res, "attname");
+		for (rowno = 0; rowno < ntups; rowno++)
+		{
+			if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
+				pg_fatal("could not open file \"%s\": %s", output_path,
+						 strerror(errno));
+			if (!db_used)
+			{
+				fprintf(script, "Database: %s\n", active_db->db_name);
+				db_used = true;
+			}
+			fprintf(script, "  %s.%s.%s\n",
+					PQgetvalue(res, rowno, i_nspname),
+					PQgetvalue(res, rowno, i_relname),
+					PQgetvalue(res, rowno, i_attname));
+		}
+
+		PQclear(res);
+
+		PQfinish(conn);
+	}
+
+	if (script)
+	{
+		fclose(script);
+		pg_log(PG_REPORT, "fatal");
+		pg_fatal("Your installation contains has inconsistencies in NOT NULL\n"
+				 "constraint inheritance: child column is not marked NOT NULL\n"
+				 "while parent column has NOT NULL constraint. You can fix the\n"
+				 "inconsistency by adding NOT NULL constraints and rerunning the\n"
+				 "upgrade.\n"
+				 "A list of the problematic columns is in the file:\n"
+				 "    %s", output_path);
+	}
+	else
+		check_ok();
+}
+
 /*
  * check_old_cluster_for_valid_slots()
  *
-- 
2.42.0


--f9RqhN/xbOGzZVv+--