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

On Wed, Jul 26, 2017 at 12:09 PM, tushar <tushar.ahuja@enterprisedb.com> wrote:

v9.5/9.6

create these objects -
CREATE TABLE constraint_rename_test (a int CONSTRAINT con1 CHECK (a > 0), b
int, c int);
CREATE TABLE constraint_rename_test2 (a int CONSTRAINT con1 CHECK (a > 0), d
int) INHERITS (constraint_rename_test);
ALTER TABLE constraint_rename_test ADD CONSTRAINT con3 PRIMARY KEY (a);

v9.6/v10 - run pg_upgrade

pg_restore: creating COMMENT "SCHEMA "public""
pg_restore: creating TABLE "public.constraint_rename_test"
pg_restore: creating TABLE "public.constraint_rename_test2"
pg_restore: [archiver (db)] Error while PROCESSING TOC:
pg_restore: [archiver (db)] Error from TOC entry 351; 1259 16388 TABLE
constraint_rename_test2 edb
pg_restore: [archiver (db)] could not execute query: ERROR: column "a" in
child table must be marked NOT NULL
Command was:
-- For binary upgrade, must preserve pg_type oid
SELECT
pg_catalog.binary_upgrade_set_next_pg_type_oid('16390'::pg_catalog.oid);

manually i am able to create all these objects .

You can go further down to reproduce the failure of your test case. I
can see the same thing with at least 9.3, which is as far as I tested,
for any upgrade combinations up to HEAD. And I would imagine that this
is an old behavior.

The documentation says the following:
https://www.postgresql.org/docs/9.3/static/ddl-inherit.html
All check constraints and not-null constraints on a parent table are
automatically inherited by its children. Other types of constraints
(unique, primary key, and foreign key constraints) are not inherited.

When adding a primary key, the system does first SET NOT NULL on each
column under cover, but this does not get spread to the child tables
as this is a PRIMARY KEY. The question is, do we want to spread the
NOT NULL constraint created by the PRIMARY KEY command to the child
tables, or not? It is easy enough to fix your problem by switching the
second and third commands in your test case, still I think that the
current behavior is non-intuitive, and that we ought to fix this by
applying NOT NULL to the child's columns when a primary key is added
to the parent. Thoughts?
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Michael Paquier <michael.paquier@gmail.com> writes:

The documentation says the following:
https://www.postgresql.org/docs/9.3/static/ddl-inherit.html
All check constraints and not-null constraints on a parent table are
automatically inherited by its children. Other types of constraints
(unique, primary key, and foreign key constraints) are not inherited.

When adding a primary key, the system does first SET NOT NULL on each
column under cover, but this does not get spread to the child tables
as this is a PRIMARY KEY. The question is, do we want to spread the
NOT NULL constraint created by the PRIMARY KEY command to the child
tables, or not?

Yeah, I think we really ought to for consistency. Quite aside from
pg_dump hazards, this allows situations where selecting from an
apparently not-null column can produce nulls (coming from unconstrained
child tables). That's exactly what the automatic inheritance rules
are intended to prevent. If we had a way to mark NOT NULL constraints
as not-inherited, it'd be legitimate for ADD PRIMARY KEY to paste on
one of those instead of a regular one ... but we lack that feature,
so it's moot.

Not sure what's involved there code-wise, though, nor whether we'd want
to back-patch.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Jul 26, 2017 at 4:02 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Not sure what's involved there code-wise, though, nor whether we'd want
to back-patch.

I'll try to look at the code around that to come up with a clear
conclusion in the next couple of days, likely more as that's a
vacation period. Surely anything invasive would not be backpatched.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Jul 26, 2017 at 4:50 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Wed, Jul 26, 2017 at 4:02 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Not sure what's involved there code-wise, though, nor whether we'd want
to back-patch.

I'll try to look at the code around that to come up with a clear
conclusion in the next couple of days, likely more as that's a
vacation period. Surely anything invasive would not be backpatched.

So I think that the attached patch is able to do the legwork. While
looking at the code, I have bumped into index_check_primary_key() that
discarded the case of sending SET NOT NULL to child tables, as
introduced by 88452d5b. But that's clearly an oversight IMO, and the
comment is wrong to begin with because SET NOT NULL is spread to child
tables. Using is_alter_table instead of a plain true in
index_check_primary_key() for the call of AlterTableInternal() is
defensive, but I don't think that we want to impact any modules
relying on this API, so recursing only when ALTER TABLE is used is the
safest course of action to me. With this logic, we rely as well on the
fact that ADD PRIMARY KEY is not recursive in tablecmds.c. Comments
are welcome, I'll park that into the CF app so as it does not get lost
in the void.
--
Michael

Michael Paquier <michael.paquier@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.

While
looking at the code, I have bumped into index_check_primary_key() that
discarded the case of sending SET NOT NULL to child tables, as
introduced by 88452d5b. But that's clearly an oversight IMO, and the
comment is wrong to begin with because SET NOT NULL is spread to child
tables. Using is_alter_table instead of a plain true in
index_check_primary_key() for the call of AlterTableInternal() is
defensive, but I don't think that we want to impact any modules
relying on this API, so recursing only when ALTER TABLE is used is the
safest course of action to me.

I didn't find that persuasive: I think passing "recurse" as just plain
"true" is safer and more readable. We shouldn't be able to get there
in a CREATE case, as per the comments; and if we did, there shouldn't
be any child tables anyway; but if somehow there were, surely the same
consistency argument would imply that we *must* recurse and fix those
child tables. So I just made it "true".

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Aug 4, 2017 at 5:50 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Michael Paquier <michael.paquier@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.

While
looking at the code, I have bumped into index_check_primary_key() that
discarded the case of sending SET NOT NULL to child tables, as
introduced by 88452d5b. But that's clearly an oversight IMO, and the
comment is wrong to begin with because SET NOT NULL is spread to child
tables. Using is_alter_table instead of a plain true in
index_check_primary_key() for the call of AlterTableInternal() is
defensive, but I don't think that we want to impact any modules
relying on this API, so recursing only when ALTER TABLE is used is the
safest course of action to me.

I didn't find that persuasive: I think passing "recurse" as just plain
"true" is safer and more readable. We shouldn't be able to get there
in a CREATE case, as per the comments; and if we did, there shouldn't
be any child tables anyway; but if somehow there were, surely the same
consistency argument would imply that we *must* recurse and fix those
child tables. So I just made it "true".

Yeah, that matches what I read as well. No complains to switch to a
plain "true" even if it is not reached yet.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.paquier@gmail.com>:

On Fri, Aug 4, 2017 at 5:50 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Michael Paquier <michael.paquier@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

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

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

On Wed, Dec 13, 2017 at 10:41 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> 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:
/messages/by-id/CAB7nPqTPXgX9HiyhhtAgpW7jbA1iskMCSoqXPEEB_KYXYy1E1Q@mail.gmail.com
Or this one:
/messages/by-id/21633.1448383428@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

2017-12-13 9:10 GMT+07:00 Michael Paquier <michael.paquier@gmail.com>:

It is not the first time that this topic shows up. See for example
this thread from myself's version of last year:
/messages/by-id/CAB7nPqTPXgX9HiyhhtAgpW7jbA1is
kMCSoqXPEEB_KYXYy1E1Q@mail.gmail.com
Or this one:
/messages/by-id/21633.1448383428@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.

Thanks for the link to those thread.

Judging from the discussion there, it will be a long way to prevent DROP
NOT NULL. So for this problem (pg_upgrade failing because of it), i propose
that we only add a check in pg_upgrade, so anyone using pg_upgrade can know
and fix the issue before the error?

If it's OK, i can write the patch.

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.

Thanks. Judging from above, it's better that we continue the DROP NOT NULL
problem in another patch (and another thread)

Best Regards,
Ali Akbar

On 2017/12/13 15:59, Ali Akbar wrote:

2017-12-13 9:10 GMT+07:00 Michael Paquier <michael.paquier@gmail.com>:

It is not the first time that this topic shows up. See for example
this thread from myself's version of last year:
/messages/by-id/CAB7nPqTPXgX9HiyhhtAgpW7jbA1is
kMCSoqXPEEB_KYXYy1E1Q@mail.gmail.com
Or this one:
/messages/by-id/21633.1448383428@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.

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.

Thanks,
Amit

2017-12-13 15:37 GMT+07:00 Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>:

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

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>:

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

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

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

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

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]/messages/by-id/CACQjQLqoXTzCV4+-s1XOT62tY8ggkZkH4kY03gm2aQYOMXE+SA@mail.gmail.com -- Michael 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]: /messages/by-id/CACQjQLqoXTzCV4+-s1XOT62tY8ggkZkH4kY03gm2aQYOMXE+SA@mail.gmail.com -- Michael
--
Michael

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.

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.

Hello,

We seem to have forgotten about this issue. I think this is even more
pressing with the changes to 18 for not-null constraints, though
strictly speaking it's always been a problem. Here's an updated patch.
I simplified the query (no need for a recursive CTE as far as I can
tell) and updated to use the pg_upgrade "task" infrastructure.

I think from 18 on, the problem can no longer be recreated, since
removing the attnotnull bit from a child table is not possible.

I should add some testing too before pushing, of course. I only tested
manually.

I'd like to hear from RMT about getting this in 18. Actually I think we
should consider backporting to all live versions ... thoughts?

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Investigación es lo que hago cuando no sé lo que estoy haciendo"
(Wernher von Braun)

I think from 18 on, the problem can no longer be recreated,

Actually I think we should consider backporting to all live versions

If you don't backpatch it, there's no point.

On 2025-Jul-03, Justin Pryzby wrote:

I think from 18 on, the problem can no longer be recreated,

Actually I think we should consider backporting to all live versions

If you don't backpatch it, there's no point.

Oh yeah, you're absolutely right.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"[PostgreSQL] is a great group; in my opinion it is THE best open source
development communities in existence anywhere." (Lamar Owen)

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

On 2025-Jul-03, Justin Pryzby wrote:

Actually I think we should consider backporting to all live versions

If you don't backpatch it, there's no point.

Oh yeah, you're absolutely right.

No, it'd still be useful for handling upgrades from $busted_version
to current. But it seems also useful in the back branches, so
+1 for back-patch.

regards, tom lane

Two more minor things on this:

I found it a bit silly to mention the ALTER command in the error message
and then output only the three-part-qualified name in the output file
(and no specific command to run) -- so for an instant I was tempted to
write the full ALTER TABLE command in the output file instead. However
I then noticed that check.c does not use fmtId() or fmtQualifiedId()
anywhere, so that code would look out of place in check.c and perhaps
even be a problem for backpatching. Do people thing it'd be an
improvement to change it that way?

On 2017-Dec-13, Justin Pryzby wrote:

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.

So the phrase "the problem columns" appears in multiple places in
pg_upgrade/check.c. Should we fix them all? It turns out that I
rewrote the message as submitted into completely different wording
(without realizing that this had been reported), so these words don't
appear anymore in the new commit. But we could fix these old ones while
we're here -- yes? (See below)

My rewrite is this:
pg_fatal("Your installation contains inconsistent NOT NULL constraints.\n"
"If the parent column(s) are NOT NULL, then the child column must\n"
"also be marked NOT NULL, or the upgrade will fail.\n"
"You can fix this by running\n"
" ALTER TABLE tablename ALTER column SET NOT NULL;\n"
"on each column listed in the file:\n"
" %s", report.path);

Here's what a patch looks like to change "problem columns" to
"problematic columns".

diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 2106869b999..ad80f00c055 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -119,7 +119,7 @@ static DataTypesUsageChecks data_types_usage_checks[] =
 		gettext_noop("Your installation contains system-defined composite types in user tables.\n"
 					 "These type OIDs are not stable across PostgreSQL versions,\n"
 					 "so this cluster cannot currently be upgraded.  You can drop the\n"
-					 "problem columns and restart the upgrade.\n"),
+					 "problematic columns and restart the upgrade.\n"),
 		.threshold_version = ALL_VERSIONS
 	},

@@ -139,7 +139,7 @@ static DataTypesUsageChecks data_types_usage_checks[] =
 					 "This data type changed its internal and input/output format\n"
 					 "between your old and new versions so this\n"
 					 "cluster cannot currently be upgraded.  You can\n"
-					 "drop the problem columns and restart the upgrade.\n"),
+					 "drop the problematic columns and restart the upgrade.\n"),
 		.threshold_version = 903
 	},

@@ -183,7 +183,7 @@ static DataTypesUsageChecks data_types_usage_checks[] =
 		gettext_noop("Your installation contains one of the reg* data types in user tables.\n"
 					 "These data types reference system OIDs that are not preserved by\n"
 					 "pg_upgrade, so this cluster cannot currently be upgraded.  You can\n"
-					 "drop the problem columns and restart the upgrade.\n"),
+					 "drop the problematic columns and restart the upgrade.\n"),
 		.threshold_version = ALL_VERSIONS
 	},

@@ -200,7 +200,7 @@ static DataTypesUsageChecks data_types_usage_checks[] =
 		gettext_noop("Your installation contains the \"aclitem\" data type in user tables.\n"
 					 "The internal format of \"aclitem\" changed in PostgreSQL version 16\n"
 					 "so this cluster cannot currently be upgraded.  You can drop the\n"
-					 "problem columns and restart the upgrade.\n"),
+					 "problematic columns and restart the upgrade.\n"),
 		.threshold_version = 1500
 	},

@@ -223,7 +223,7 @@ static DataTypesUsageChecks data_types_usage_checks[] =
 		.report_text =
 		gettext_noop("Your installation contains the \"unknown\" data type in user tables.\n"
 					 "This data type is no longer allowed in tables, so this cluster\n"
-					 "cannot currently be upgraded.  You can drop the problem columns\n"
+					 "cannot currently be upgraded.  You can drop the problematic columns\n"
 					 "and restart the upgrade.\n"),
 		.threshold_version = 906
 	},
@@ -279,7 +279,7 @@ static DataTypesUsageChecks data_types_usage_checks[] =
 		gettext_noop("Your installation contains the \"abstime\" data type in user tables.\n"
 					 "The \"abstime\" type has been removed in PostgreSQL version 12,\n"
 					 "so this cluster cannot currently be upgraded.  You can drop the\n"
-					 "problem columns, or change them to another data type, and restart\n"
+					 "problematic columns, or change them to another data type, and restart\n"
 					 "the upgrade.\n"),
 		.threshold_version = 1100
 	},
@@ -292,7 +292,7 @@ static DataTypesUsageChecks data_types_usage_checks[] =
 		gettext_noop("Your installation contains the \"reltime\" data type in user tables.\n"
 					 "The \"reltime\" type has been removed in PostgreSQL version 12,\n"
 					 "so this cluster cannot currently be upgraded.  You can drop the\n"
-					 "problem columns, or change them to another data type, and restart\n"
+					 "problematic columns, or change them to another data type, and restart\n"
 					 "the upgrade.\n"),
 		.threshold_version = 1100
 	},
@@ -305,7 +305,7 @@ static DataTypesUsageChecks data_types_usage_checks[] =
 		gettext_noop("Your installation contains the \"tinterval\" data type in user tables.\n"
 					 "The \"tinterval\" type has been removed in PostgreSQL version 12,\n"
 					 "so this cluster cannot currently be upgraded.  You can drop the\n"
-					 "problem columns, or change them to another data type, and restart\n"
+					 "problematic columns, or change them to another data type, and restart\n"
 					 "the upgrade.\n"),
 		.threshold_version = 1100
 	},
@@ -423,7 +423,7 @@ process_data_type_check(DbInfo *dbinfo, PGresult *res, void *arg)
 		pg_log(PG_REPORT, "failed check: %s", _(state->check->status));
 		appendPQExpBuffer(*state->report, "\n%s\n%s    %s\n",
 						  _(state->check->report_text),
-						  _("A list of the problem columns is in the file:"),
+						  _("A list of the problematic columns is in the file:"),
 						  output_path);
 	}
 	state->result = true;

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Ed is the standard text editor."
http://groups.google.com/group/alt.religion.emacs/msg/8d94ddab6a9b0ad3

On 2025-Jul-03, Tom Lane wrote:

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

On 2025-Jul-03, Justin Pryzby wrote:

Actually I think we should consider backporting to all live versions

If you don't backpatch it, there's no point.

Oh yeah, you're absolutely right.

No, it'd still be useful for handling upgrades from $busted_version
to current. But it seems also useful in the back branches, so
+1 for back-patch.

Pushed to all branches.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"El miedo atento y previsor es la madre de la seguridad" (E. Burke)

Hmm, crake doesn't like the fact that there's no regnamespace in 9.4.
Apparently in version 13 we claim to support back to 9.0. I only tried
with an old server version 12. Should be an easy fix ...

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/

On 2025-Jul-04, Alvaro Herrera wrote:

Hmm, crake doesn't like the fact that there's no regnamespace in 9.4.
Apparently in version 13 we claim to support back to 9.0. I only tried
with an old server version 12. Should be an easy fix ...

It goes like this. It's tested with 9.2 only though, as 9.1 doesn't
compile from Git (lack of Flex) and I didn't want to try a tarball.

(This is for master. 17 and back require a small change for lack of
task.c)

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"I apologize for the confusion in my previous responses.
There appears to be an error." (ChatGPT)

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

On 2025-Jul-04, Alvaro Herrera wrote:

Hmm, crake doesn't like the fact that there's no regnamespace in 9.4.
Apparently in version 13 we claim to support back to 9.0. I only tried
with an old server version 12. Should be an easy fix ...

It goes like this. It's tested with 9.2 only though, as 9.1 doesn't
compile from Git (lack of Flex) and I didn't want to try a tarball.

Query works, or at least throws no error, back to 8.4 (the oldest I
have at hand).

regards, tom lane

On 2025-Jul-04, Tom Lane wrote:

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

On 2025-Jul-04, Alvaro Herrera wrote:

Hmm, crake doesn't like the fact that there's no regnamespace in 9.4.
Apparently in version 13 we claim to support back to 9.0. I only tried
with an old server version 12. Should be an easy fix ...

It goes like this. It's tested with 9.2 only though, as 9.1 doesn't
compile from Git (lack of Flex) and I didn't want to try a tarball.

Query works, or at least throws no error, back to 8.4 (the oldest I
have at hand).

Thanks for checking! Pushed late yesterday, crake is happy now.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Ninguna manada de bestias tiene una voz tan horrible como la humana" (Orual)