Re: [HACKERS] pg_upgrade failed with error - ERROR: column "a" in child table must be marked NOT NULL

[Justin Pryzby]

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.
>From f804bec39acac0b53e29563be9ef7a10237ba717 Mon Sep 17 00:00:00 2001
From: Ali Akbar 
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(_cluster);
 
+	check_for_not_null_inheritance(_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 = >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 = 

Re: [HACKERS] pg_upgrade failed with error - ERROR: column "a" in child table must be marked NOT NULL

[Justin Pryzby]

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.

Re: [HACKERS] pg_upgrade failed with error - ERROR: column "a" in child table must be marked NOT NULL

[Michael Paquier]

On Thu, Sep 28, 2023 at 01:29:21PM -0500, Justin Pryzby wrote:
> On Thu, Dec 14, 2017 at 07:18:59PM +0700, Ali Akbar wrote:
> > By the way, should i add this patch to the current commitfest?
> 
> The patch for pg_upgrade --check got forgotten 6 years ago, but it's a
> continuing problem (we hit it again which cost an hour during
> pg_upgrade) and ought to be (have been) backpatched.

You mean when upgrading from an instance of 9.6 or older as c30f177 is
not there, right?  With the new project policy to support pg_upgrade
for 10 years, there's still a very good argument for backpatching a
pre-check down to v11.

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.

> I didn't dig into it, but maybe it'd even be possible to fix the issue
> with ALTER..DROP NOT NULL ...

Interesting point.  I haven't checked either.

[1]: 
https://postgr.es/m/cacqjqlqoxtzcv4+-s1xot62ty8ggkzkh4ky03gm2aqyomxe...@mail.gmail.com
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] pg_upgrade failed with error - ERROR: column "a" in child table must be marked NOT NULL

[Justin Pryzby]

On Thu, Dec 14, 2017 at 07:18:59PM +0700, Ali Akbar wrote:
> By the way, should i add this patch to the current commitfest?

The patch for pg_upgrade --check got forgotten 6 years ago, but it's a
continuing problem (we hit it again which cost an hour during
pg_upgrade) and ought to be (have been) backpatched.

I didn't dig into it, but maybe it'd even be possible to fix the issue
with ALTER..DROP NOT NULL ...

-- 
Justin

Re: [HACKERS] pg_upgrade failed with error - ERROR: column "a" in child table must be marked NOT NULL

[Ali Akbar]

2017-12-14 15:08 GMT+07:00 Michael Paquier :
>
> On Thu, Dec 14, 2017 at 2:49 PM, Justin Pryzby  wrote:
> > On Thu, Dec 14, 2017 at 08:51:06AM +0700, Ali Akbar wrote:
> >> 2017-12-13 15:37 GMT+07:00 Amit Langote :
> >> Patch for adding check in pg_upgrade. Going through git history, the check
> >> for inconsistency in NOT NULL constraint has ben there since a long time
> >> ago. In this patch the check will be applied for all old cluster version.
> >> I'm not sure in which version was the release of table inheritance.
> >
> > 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.


Thanks. Typo fixed.

>
> I agree that we should do better in pg_upgrade for HEAD and
> REL_10_STABLE as it is not cool to fail late in the upgrade process
> especially if you are using the --link mode.
>
> + *  Currently pg_upgrade will fail if there are any inconsistencies in NOT 
> NULL
> + *  constraint inheritance: In Postgres version 9.5, 9.6, 10 we can
> have a column
> + *  that is NOT NULL in parent, but nullabe in its children. But during 
> schema
> + *  restore, that will cause error.
> On top of the multiple typos here, there is no need to mention
> anything other than "PostgreSQL 9.6 and older versions did not add NOT
> NULL constraints when a primary key was added to to a parent, so check
> for inconsistent NOT NULL constraints in inheritance trees".


In my case, my database becomes like that because accidental ALTER
COLUMN x DROP NOT NULL, and no, not the Primary Key.

Something like this:

CREATE TABLE parent (id SERIAL PRIMARY KEY, name VARCHAR(52) NOT NULL);
CREATE TABLE child () INHERITS (parent);
ALTER TABLE child ALTER COLUMN name DROP NOT NULL;

And yes, in current version 10 (and HEAD), we can still do that.

Because both version currently accepts that inconsistency, ideally
pg_upgrade must be able to upgrade the cluster without problem. Hence
the comment: "Currently pg_upgrade will fail if there are any
inconsistencies in NOT NULL constraint inheritance".

> You seem to take a path similar to what is done with the jsonb check.
> Why not. I would find cleaner to tell also the user about using ALTER
> TABLE to add the proper constraints.
>
> Note that I have not checked or tested the query in details, but
> reading through the thing looks basically correct as it handles
> inheritance chains with WITH RECURSIVE.

Any suggestion to make it more readable? Maybe by separating column
query with schema & table name by using another CTE?

> @@ -99,6 +100,7 @@ check_and_dump_old_cluster(bool live_check)
>  check_for_prepared_transactions(_cluster);
>  check_for_reg_data_type_usage(_cluster);
>  check_for_isn_and_int8_passing_mismatch(_cluster);
> +check_for_not_null_inheritance(_cluster);
> The check needs indeed to happen in check_and_dump_old_cluster(), but
> there is no point to check for it in servers with version 10 and
> upwards, hence you should make it conditional with GET_MAJOR_VERSION()
> <= 906.

No, currently it can happen again in version 10 (and above) because of
the problem above.

By the way, should i add this patch to the current commitfest?

Thanks,
Ali Akbar
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 1b9083597c..0bd2cd0ed1 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -26,6 +26,7 @@ static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
 static void check_for_reg_data_type_usage(ClusterInfo *cluster);
 static void check_for_jsonb_9_4_usage(ClusterInfo *cluster);
 static void check_for_pg_role_prefix(ClusterInfo *cluster);
+static void check_for_not_null_inheritance(ClusterInfo *cluster);
 static char *get_canonical_locale_name(int category, const char *locale);
 
 
@@ -99,6 +100,7 @@ check_and_dump_old_cluster(bool live_check)
 	check_for_prepared_transactions(_cluster);
 	check_for_reg_data_type_usage(_cluster);
 	check_for_isn_and_int8_passing_mismatch(_cluster);
+	check_for_not_null_inheritance(_cluster);
 
 	/*
 	 * Pre-PG 10 allowed tables with 'unknown' type columns and non WAL logged
@@ -1096,6 +1098,105 @@ check_for_pg_role_prefix(ClusterInfo *cluster)
 	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	   

Re: [HACKERS] pg_upgrade failed with error - ERROR: column "a" in child table must be marked NOT NULL

[Michael Paquier]

On Thu, Dec 14, 2017 at 2:49 PM, Justin Pryzby  wrote:
> On Thu, Dec 14, 2017 at 08:51:06AM +0700, Ali Akbar wrote:
>> 2017-12-13 15:37 GMT+07:00 Amit Langote :
>> Patch for adding check in pg_upgrade. Going through git history, the check
>> for inconsistency in NOT NULL constraint has ben there since a long time
>> ago. In this patch the check will be applied for all old cluster version.
>> I'm not sure in which version was the release of table inheritance.
>
> 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.

I agree that we should do better in pg_upgrade for HEAD and
REL_10_STABLE as it is not cool to fail late in the upgrade process
especially if you are using the --link mode.

+ *  Currently pg_upgrade will fail if there are any inconsistencies in NOT NULL
+ *  constraint inheritance: In Postgres version 9.5, 9.6, 10 we can
have a column
+ *  that is NOT NULL in parent, but nullabe in its children. But during schema
+ *  restore, that will cause error.
On top of the multiple typos here, there is no need to mention
anything other than "PostgreSQL 9.6 and older versions did not add NOT
NULL constraints when a primary key was added to to a parent, so check
for inconsistent NOT NULL constraints in inheritance trees".

You seem to take a path similar to what is done with the jsonb check.
Why not. I would find cleaner to tell also the user about using ALTER
TABLE to add the proper constraints.

Note that I have not checked or tested the query in details, but
reading through the thing looks basically correct as it handles
inheritance chains with WITH RECURSIVE.

@@ -99,6 +100,7 @@ check_and_dump_old_cluster(bool live_check)
 check_for_prepared_transactions(_cluster);
 check_for_reg_data_type_usage(_cluster);
 check_for_isn_and_int8_passing_mismatch(_cluster);
+check_for_not_null_inheritance(_cluster);
The check needs indeed to happen in check_and_dump_old_cluster(), but
there is no point to check for it in servers with version 10 and
upwards, hence you should make it conditional with GET_MAJOR_VERSION()
<= 906.
-- 
Michael

Re: [HACKERS] pg_upgrade failed with error - ERROR: column "a" in child table must be marked NOT NULL

[Justin Pryzby]

On Thu, Dec 14, 2017 at 08:51:06AM +0700, Ali Akbar wrote:
> 2017-12-13 15:37 GMT+07:00 Amit Langote :
> 
> > On 2017/12/13 15:59, Ali Akbar wrote:
> > >
> > > Thanks for the link to those thread.
> > >
> > > Judging from the discussion there, it will be a long way to prevent DROP
> > > NOT NULL.
> >
> > Yeah, I remembered that discussion when writing my email, but was for some
> > reason convinced that everything's fine even without the elaborate
> > book-keeping of inheritance information for NOT NULL constraints.  Thanks
> > Michael for reminding.
> >
> 
> Patch for adding check in pg_upgrade. Going through git history, the check
> for inconsistency in NOT NULL constraint has ben there since a long time
> ago. In this patch the check will be applied for all old cluster version.
> I'm not sure in which version was the release of table inheritance.

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.

Justin

Re: [HACKERS] pg_upgrade failed with error - ERROR: column "a" in child table must be marked NOT NULL

[Ali Akbar]

2017-12-13 15:37 GMT+07:00 Amit Langote :

> On 2017/12/13 15:59, Ali Akbar wrote:
> >
> > Thanks for the link to those thread.
> >
> > Judging from the discussion there, it will be a long way to prevent DROP
> > NOT NULL.
>
> Yeah, I remembered that discussion when writing my email, but was for some
> reason convinced that everything's fine even without the elaborate
> book-keeping of inheritance information for NOT NULL constraints.  Thanks
> Michael for reminding.
>

Patch for adding check in pg_upgrade. Going through git history, the check
for inconsistency in NOT NULL constraint has ben there since a long time
ago. In this patch the check will be applied for all old cluster version.
I'm not sure in which version was the release of table inheritance.

Thanks,
Ali Akbar
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 1b9083597c..29bafdff74 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -26,6 +26,7 @@ static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
 static void check_for_reg_data_type_usage(ClusterInfo *cluster);
 static void check_for_jsonb_9_4_usage(ClusterInfo *cluster);
 static void check_for_pg_role_prefix(ClusterInfo *cluster);
+static void check_for_not_null_inheritance(ClusterInfo *cluster);
 static char *get_canonical_locale_name(int category, const char *locale);
 
 
@@ -99,6 +100,7 @@ check_and_dump_old_cluster(bool live_check)
 	check_for_prepared_transactions(_cluster);
 	check_for_reg_data_type_usage(_cluster);
 	check_for_isn_and_int8_passing_mismatch(_cluster);
+	check_for_not_null_inheritance(_cluster);
 
 	/*
 	 * Pre-PG 10 allowed tables with 'unknown' type columns and non WAL logged
@@ -1096,6 +1098,105 @@ check_for_pg_role_prefix(ClusterInfo *cluster)
 	check_ok();
 }
 
+/*
+ * check_for_not_null_inheritance()
+ *
+ *  Currently pg_upgrade will fail if there are any inconsistencies in NOT NULL
+ *  constraint inheritance: In Postgres version 9.5, 9.6, 10 we can have a column
+ *  that is NOT NULL in parent, but nullabe in its children. But during schema
+ *  restore, that will cause error.
+ */
+static void
+check_for_not_null_inheritance(ClusterInfo *cluster)
+{
+	int			dbnum;
+	FILE	   *script = NULL;
+	bool		found = false;
+	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 = >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_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 "
+"		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++)
+		{
+			found = true;
+			if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
+pg_fatal("could not open file \"%s\": %s\n", 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);
+
+	if (found)
+	{
+		pg_log(PG_REPORT, "fatal\n");
+		pg_fatal("Your installation contains has inconsistencies in NOT NULL\n"
+ "constraint inheritance: child column is not market NOT NULL\n"
+ "while parent column has NOT NULL constraint. You can fix the\n"
+ "inconsistency with adding NOT NULL constraint and restart the\n"
+ "upgrade.\n"
+ "A list of the problem columns is in the file:\n"
+ "%s\n\n", output_path);
+	}
+	else
+		check_ok();
+}
 
 /*
  * get_canonical_locale_name

Re: [HACKERS] pg_upgrade failed with error - ERROR: column "a" in child table must be marked NOT NULL

[Michael Paquier]

On Wed, Dec 13, 2017 at 10:41 AM, Amit Langote
 wrote:
> On 2017/12/13 9:22, Ali Akbar wrote:
>> For me, it's better to prevent that from happening. So, attempts to
>> DROP NOT NULL on the child must be rejected. The attached patch does
>> that.
>>
>> Unfortunately, pg_class has no "has_parent" attribute, so in this
>> patch, it hits pg_inherits everytime DROP NOT NULL is attempted.
>>
>> Notes:
>> - It looks like we could remove the parent partition checking above?
>> Because the new check already covers what it does
>
> I noticed that too.
>
> So, if we're going to prevent DROP NOT NULL on *all* child table (not just
> partitions), we don't need the code that now sits there to prevent that
> only for partitions.

It is not the first time that this topic shows up. See for example
this thread from myself's version of last year:
https://www.postgresql.org/message-id/cab7npqtpxgx9hiyhhtagpw7jba1iskmcsoqxpeeb_kyxyy1...@mail.gmail.com
Or this one:
http://www.postgresql.org/message-id/21633.1448383...@sss.pgh.pa.us

And the conclusion is that we don't want to do what you are doing
here, and that it would be better to store NOT NULL constraints in a
way similar to CHECK constraints.

> How about: ".. if some parent has the same"
>
> +   heap_close(parent, AccessShareLock);
>
> Maybe, we shouldn't be dropping the lock so soon.

Yes, such things usually need to be kept until the end of the
transaction, and usually you need to be careful about potential lock
upgrades that could cause deadlocks. This patch looks wrong for both
of those things.
-- 
Michael

Re: [HACKERS] pg_upgrade failed with error - ERROR: column "a" in child table must be marked NOT NULL

[Amit Langote]

Hi.

On 2017/12/13 9:22, Ali Akbar wrote:
> For me, it's better to prevent that from happening. So, attempts to
> DROP NOT NULL on the child must be rejected. The attached patch does
> that.
> 
> Unfortunately, pg_class has no "has_parent" attribute, so in this
> patch, it hits pg_inherits everytime DROP NOT NULL is attempted.
> 
> Notes:
> - It looks like we could remove the parent partition checking above?
> Because the new check already covers what it does

I noticed that too.

So, if we're going to prevent DROP NOT NULL on *all* child table (not just
partitions), we don't need the code that now sits there to prevent that
only for partitions.

Minor comments on the patch:

+   /* If rel has parents, shoudn't drop NOT NULL if parent has the same */

How about: ".. if some parent has the same"

+   heap_close(parent, AccessShareLock);

Maybe, we shouldn't be dropping the lock so soon.

Thanks,
Amit

Re: [HACKERS] pg_upgrade failed with error - ERROR: column "a" in child table must be marked NOT NULL

[Ali Akbar]

> For me, it's better to prevent that from happening. So, attempts to
> DROP NOT NULL on the child must be rejected. The attached patch does
> that.

I'm sorry. Accidentaly i "--color"-ed the patch format. Attached the
correct patch.

Best Regards,
Ali Akbar
diff --git a/src/backend/catalog/pg_inherits.c b/src/backend/catalog/pg_inherits.c
index 1bd8a58b7f..74903a8f24 100644
--- a/src/backend/catalog/pg_inherits.c
+++ b/src/backend/catalog/pg_inherits.c
@@ -242,6 +242,40 @@ find_all_inheritors(Oid parentrelId, LOCKMODE lockmode, List **numparents)
 }
 
 
+/*
+ * get_superclasses -
+ *		Returns a list of relation OIDs of direct parents
+ */
+List *
+get_superclasses(Oid relationId)
+{
+	List	   *list = NIL;
+	Relation	catalog;
+	SysScanDesc scan;
+	ScanKeyData skey;
+	HeapTuple	inheritsTuple;
+	Oid			inhparent;
+
+	catalog = heap_open(InheritsRelationId, AccessShareLock);
+	ScanKeyInit(, Anum_pg_inherits_inhrelid, BTEqualStrategyNumber,
+F_OIDEQ, ObjectIdGetDatum(relationId));
+	scan = systable_beginscan(catalog, InheritsRelidSeqnoIndexId, true,
+			  NULL, 1, );
+
+	while ((inheritsTuple = systable_getnext(scan)) != NULL)
+	{
+		inhparent = ((Form_pg_inherits) GETSTRUCT(inheritsTuple))->inhparent;
+		list = lappend_oid(list, inhparent);
+	}
+
+	systable_endscan(scan);
+
+	heap_close(catalog, AccessShareLock);
+
+	return list;
+}
+
+
 /*
  * has_subclass - does this relation have any children?
  *
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d979ce266d..c76fc3715d 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5683,6 +5683,8 @@ ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode)
 	Relation	attr_rel;
 	List	   *indexoidlist;
 	ListCell   *indexoidscan;
+	List	   *parentlist;
+	ListCell   *parentscan;
 	ObjectAddress address;
 
 	/*
@@ -5773,6 +5775,24 @@ ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode)
 		heap_close(parent, AccessShareLock);
 	}
 
+	/* If rel has parents, shoudn't drop NOT NULL if parent has the same */
+	parentlist = get_superclasses(RelationGetRelid(rel));
+	foreach(parentscan, parentlist) {
+		Oid			parentId = lfirst_oid(parentscan);
+		Relation	parent = heap_open(parentId, AccessShareLock);
+		TupleDesc	tupDesc = RelationGetDescr(parent);
+		AttrNumber	parent_attnum;
+
+		parent_attnum = get_attnum(parentId, colName);
+		if (parent_attnum != InvalidAttrNumber &&
+			TupleDescAttr(tupDesc, parent_attnum - 1)->attnotnull)
+			ereport(ERROR,
+	(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+	 errmsg("column \"%s\" is marked NOT NULL in parent table",
+			colName)));
+		heap_close(parent, AccessShareLock);
+	}
+
 	/*
 	 * Okay, actually perform the catalog change ... if needed
 	 */
diff --git a/src/include/catalog/pg_inherits_fn.h b/src/include/catalog/pg_inherits_fn.h
index 7743388899..291861b846 100644
--- a/src/include/catalog/pg_inherits_fn.h
+++ b/src/include/catalog/pg_inherits_fn.h
@@ -20,6 +20,7 @@
 extern List *find_inheritance_children(Oid parentrelId, LOCKMODE lockmode);
 extern List *find_all_inheritors(Oid parentrelId, LOCKMODE lockmode,
 	List **parents);
+extern List *get_superclasses(Oid relationId);
 extern bool has_subclass(Oid relationId);
 extern bool has_superclass(Oid relationId);
 extern bool typeInheritsFrom(Oid subclassTypeId, Oid superclassTypeId);