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

[Michael Paquier]

On Fri, Aug 4, 2017 at 5:50 PM, Tom Lane  wrote:
> Michael Paquier  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

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

[Tom Lane]

Michael Paquier  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

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

[Michael Paquier]

On Wed, Jul 26, 2017 at 4:50 PM, Michael Paquier
 wrote:
> On Wed, Jul 26, 2017 at 4:02 PM, Tom Lane  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


alttab-setnotnull-v1.patch
Description: Binary data

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

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

[Michael Paquier]

On Wed, Jul 26, 2017 at 4:02 PM, Tom Lane  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

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

[Tom Lane]

Michael Paquier  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

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

[Michael Paquier]

On Wed, Jul 26, 2017 at 12:09 PM, tushar  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

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

[tushar]

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



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