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

On Thu, Dec 14, 2017 at 2:49 PM, Justin Pryzby <pryzby@telsasoft.com> wrote:
> On Thu, Dec 14, 2017 at 08:51:06AM +0700, Ali Akbar wrote:
>> 2017-12-13 15:37 GMT+07:00 Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>:
>> 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(&old_cluster);
     check_for_reg_data_type_usage(&old_cluster);
     check_for_isn_and_int8_passing_mismatch(&old_cluster);
+    check_for_not_null_inheritance(&old_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