Обсуждение: [HACKERS] pg_upgrade failed with error - ERROR: column "a" in child table mustbe marked NOT NULL

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 .

-- 
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

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

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

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

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

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

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

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:
https://www.postgresql.org/message-id/CAB7nPqTPXgX9HiyhhtAgpW7jbA1iskMCSoqXPEEB_KYXYy1E1Q@mail.gmail.com
Or this one:
http://www.postgresql.org/message-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

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:
>> https://www.postgresql.org/message-id/CAB7nPqTPXgX9HiyhhtAgpW7jbA1is
>> kMCSoqXPEEB_KYXYy1E1Q@mail.gmail.com
>> Or this one:
>> http://www.postgresql.org/message-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

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] 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+SA@mail.gmail.com
--
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.