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

[Ali Akbar]

Just stumbled across the same issues while upgrading one of my cluster
to Pg 10 with pg_upgrade. Finished the upgrade by fixing the old
database(s) and re-running pg_upgrade.

2017-08-04 23:06 GMT+07:00 Michael Paquier <michael.paqu...@gmail.com>:
>
> On Fri, Aug 4, 2017 at 5:50 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> > Michael Paquier <michael.paqu...@gmail.com> writes:
> >> So I think that the attached patch is able to do the legwork.
> >
> > I've pushed this into HEAD.  It seems like enough of a behavioral
> > change that we wouldn't want to back-patch, but IMO it's not too late
> > to be making this type of change in v10.  If we wait for the next CF
> > then it will take another year for the fix to reach the field.
>
> Thanks for applying the fix. My intention when adding that in a CF is
> not to see things lost.

Thans for the fix. Just found some issues:

1. My old database schema becomes like that by accidental modification
on the child table, and on HEAD, it still works:

# create table parent (id serial PRIMARY KEY, name VARCHAR(52) NOT NULL);
CREATE TABLE
# create table child () inherits (parent);
CREATE TABLE
# alter table child alter column name drop not null;
ALTER TABLE
# \d parent
                                   Table "public.parent"
 Column |         Type          | Collation | Nullable |
Default
--------+-----------------------+-----------+----------+------------------------------------
 id     | integer               |           | not null |
nextval('parent_id_seq'::regclass)
 name   | character varying(52) |           | not null |
Indexes:
    "parent_pkey" PRIMARY KEY, btree (id)
Number of child tables: 1 (Use \d+ to list them.)

# \d child
                                    Table "public.child"
 Column |         Type          | Collation | Nullable |
Default
--------+-----------------------+-----------+----------+------------------------------------
 id     | integer               |           | not null |
nextval('parent_id_seq'::regclass)
 name   | character varying(52) |           |          |
Inherits: parent


2. If we execute pg_dump manually, it silently corrects the schema:

..... (cut)
--
-- Name: parent; Type: TABLE; Schema: public; Owner: -
--

CREATE TABLE parent (
    id integer NOT NULL,
    name character varying(52) NOT NULL
);


--
-- Name: child; Type: TABLE; Schema: public; Owner: -
--

CREATE TABLE child (
)
INHERITS (parent);

...... (cut)

There is't any DROP NOT NULL there.



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
- If this patch will be applied, i will work on pg_upgrade to check
for this problem before attempting to dump schema. In my case, because
the cluster has many databases, the error arise much late in the
process, it will be much better if pg_upgrade complains while
performing pre-checks.


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(&skey, Anum_pg_inherits_inhrelid, BTEqualStrategyNumber,
+				F_OIDEQ, ObjectIdGetDatum(relationId));
+	scan = systable_beginscan(catalog, InheritsRelidSeqnoIndexId, true,
+							  NULL, 1, &skey);
+
+	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);