Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix new pg_upgrade query not to rely on regnamespace

  2. pg_upgrade: check for inconsistencies in not-null constraints w/inheritance

  3. Apply ALTER ... SET NOT NULL recursively in ALTER ... ADD PRIMARY KEY.

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

    tushar <tushar.ahuja@enterprisedb.com> — 2017-07-26T10:09:51Z

    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

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

    Michael Paquier <michael.paquier@gmail.com> — 2017-07-26T12:05:51Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-07-26T14:02:55Z

    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

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

    Michael Paquier <michael.paquier@gmail.com> — 2017-07-26T14:50:27Z

    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

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

    Michael Paquier <michael.paquier@gmail.com> — 2017-07-27T14:58:46Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-08-04T15:50:16Z

    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

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

    Michael Paquier <michael.paquier@gmail.com> — 2017-08-04T16:06:38Z

    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

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

    Ali Akbar <the.apaan@gmail.com> — 2017-12-13T00:22:12Z

    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

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

    Ali Akbar <the.apaan@gmail.com> — 2017-12-13T00:26:52Z

    > 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

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

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-12-13T01:41:24Z

    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

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

    Michael Paquier <michael.paquier@gmail.com> — 2017-12-13T02:10:48Z

    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

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

    Ali Akbar <the.apaan@gmail.com> — 2017-12-13T06:59:56Z

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

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

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-12-13T08:37:25Z

    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

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

    Ali Akbar <the.apaan@gmail.com> — 2017-12-14T01:51:06Z

    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

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

    Justin Pryzby <pryzby@telsasoft.com> — 2017-12-14T05:49:02Z

    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

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

    Michael Paquier <michael.paquier@gmail.com> — 2017-12-14T08:08:44Z

    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

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

    Ali Akbar <the.apaan@gmail.com> — 2017-12-14T12:18:59Z

    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

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

    Justin Pryzby <pryzby@telsasoft.com> — 2023-09-28T18:29:21Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2023-09-29T00:16:35Z

    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

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

    Justin Pryzby <pryzby@telsasoft.com> — 2023-09-29T16:36:42Z

    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.

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

    Justin Pryzby <pryzby@telsasoft.com> — 2024-04-18T20:01:52Z

    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.

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

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-07-03T18:53:01Z

    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)

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

    Justin Pryzby <pryzby@telsasoft.com> — 2025-07-03T18:58:24Z

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

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

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-07-03T19:59:11Z

    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)

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-07-03T20:07:21Z

    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

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

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-07-04T16:20:37Z

    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

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

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-07-04T16:21:31Z

    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)

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

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-07-04T16:45:38Z

    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/

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

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-07-04T17:19:23Z

    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)

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-07-04T17:37:11Z

    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

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

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-07-05T08:07:46Z

    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)