Re: [HACKERS] pg_upgrade failed with error - ERROR: column "a" in child table must be marked NOT NULL
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
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
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
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
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
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
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