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

2017-12-14 15:08 GMT+07:00 Michael Paquier <michael.paquier@gmail.com>:
>
> 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.


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(&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.

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