Prevent ALTER TABLE DROP NOT NULL on child tables if parent column has it
Hi all,
A couple of months back the $subject has been mentioned, though nobody
actually wrote a patch to prevent that:
/messages/by-id/21633.1448383428@sss.pgh.pa.us
So here is one..
To put it short, it should not be possible to drop a NOT NULL
constraint on a child relation when its parent table is using it, this
should only be possible from the parent. Attached is a patch handling
this problem by adding a new function in pg_inherits.c to fetch the
list of parent relations for a given relation OID, and did some
refactoring to stick with what is done when scanning child relations.
And here is what this patch can do:
=# create table parent (a int not null);
CREATE TABLE
=# create table child (a int not null);
CREATE TABLE
=# alter table child inherit parent ;
ALTER TABLE
=# alter table child alter COLUMN a drop not null; -- would work on HEAD
ERROR: 42P16: cannot drop NOT NULL constraint for attribute "a"
DETAIL: The same column in parent relation "parent" is marked NOT NULL
LOCATION: ATExecDropNotNull, tablecmds.c:5281
=# alter table parent alter COLUMN a drop not null; -- works on parent
ALTER TABLE
=# \d child
Table "public.child"
Column | Type | Modifiers
--------+---------+-----------
a | integer |
Inherits: parent
I have added a new index to pg_inherits, so that's not something that
could be back-patched, still it would be good to fix this weird
behavior on HEAD. I am adding that to the next CF.
Regards,
--
Michael
Attachments:
Michael Paquier <michael.paquier@gmail.com> writes:
To put it short, it should not be possible to drop a NOT NULL
constraint on a child relation when its parent table is using it, this
should only be possible from the parent. Attached is a patch handling
this problem by adding a new function in pg_inherits.c to fetch the
list of parent relations for a given relation OID, and did some
refactoring to stick with what is done when scanning child relations.
This doesn't sound like the right approach; in particular, it won't really
help for deciding whether to propagate a DROP NOT NULL on a parent rel to
its children. What we've discussed in the past is to store NOT NULL
constraints in pg_constraint, much like CHECK constraints are already, and
use use-count logic identical to the CHECK case to keep track of whether
NOT NULL constraints are inherited or not. My feeling is that we'd keep
the pg_attribute.attnotnull field and continue to drive actual enforcement
off that, but it would just reflect a summary of the pg_constraint state.
IIRC, Alvaro posted a WIP patch for that awhile back. Not sure what the
current state is.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jun 15, 2016 at 10:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Michael Paquier <michael.paquier@gmail.com> writes:
This doesn't sound like the right approach; in particular, it won't really
help for deciding whether to propagate a DROP NOT NULL on a parent rel to
its children. What we've discussed in the past is to store NOT NULL
constraints in pg_constraint, much like CHECK constraints are already, and
use use-count logic identical to the CHECK case to keep track of whether
NOT NULL constraints are inherited or not. My feeling is that we'd keep
the pg_attribute.attnotnull field and continue to drive actual enforcement
off that, but it would just reflect a summary of the pg_constraint state.
OK, I see. Hm, by storing this information I would actually think that
we want to drop this attnotnull so as we don't need to bother about
updating pg_attribute through the whole tree when dropping a NOT NULL
constraint on the parent, and we do not actually need to store this
information in two different places..
I would also rather do nothing for the DDL interface regarding for
example the possibility to change the constraint names for domains and
tables to keep things simple. A patch of this caliber would be
complicated enough if a catalog switch is done.
IIRC, Alvaro posted a WIP patch for that awhile back. Not sure what the
current state is.
Are you talking about that?
/messages/by-id/20110707213401.GA27098@alvh.no-ip.org
This is not a small patch :)
Alvaro, others, any opinions?
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 6/15/16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Michael Paquier <michael.paquier@gmail.com> writes:
To put it short, it should not be possible to drop a NOT NULL
constraint on a child relation when its parent table is using it, this
should only be possible from the parent. Attached is a patch handling
this problem by adding a new function in pg_inherits.c to fetch the
list of parent relations for a given relation OID, and did some
refactoring to stick with what is done when scanning child relations.This doesn't sound like the right approach; in particular, it won't really
help for deciding whether to propagate a DROP NOT NULL on a parent rel to
its children. What we've discussed in the past is to store NOT NULL
constraints in pg_constraint, much like CHECK constraints are already, and
use use-count logic identical to the CHECK case to keep track of whether
NOT NULL constraints are inherited or not. My feeling is that we'd keep
the pg_attribute.attnotnull field and continue to drive actual enforcement
off that, but it would just reflect a summary of the pg_constraint state.IIRC, Alvaro posted a WIP patch for that awhile back. Not sure what the
current state is.regards, tom lane
The last thread about NOT NULLs as constraints is accessible by the link[1]/messages/by-id/CAKOSWNkN6HSyatuys8xZxzRCR-KL1OkHS5-b9qd9bf1Rad3PLA@mail.gmail.com.
I rebased[2]/messages/by-id/attachment/41886/catalog-notnull-2-c477e84_cleaned.patch Alvaro's patch to the actual master at that time, but I
have not repeated it since then.
In the initial letter[1]/messages/by-id/CAKOSWNkN6HSyatuys8xZxzRCR-KL1OkHS5-b9qd9bf1Rad3PLA@mail.gmail.com I posted a digest from the SQL-2011 standard
and a conclusion as a design of a new patch.
Now I have more free time and I'm hacking it that way. The new patch
is at the very early stage, full of WIPs and TODOs. I hope it'll be
ready to be shown in a month (may be two).
But it already forbids dropping NOT NULLs if they were set as result
of inheritance.
[1]: /messages/by-id/CAKOSWNkN6HSyatuys8xZxzRCR-KL1OkHS5-b9qd9bf1Rad3PLA@mail.gmail.com
[2]: /messages/by-id/attachment/41886/catalog-notnull-2-c477e84_cleaned.patch
--
Best regards,
Vitaly Burovoy
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jun 16, 2016 at 12:10 PM, Vitaly Burovoy
<vitaly.burovoy@gmail.com> wrote:
In the initial letter[1] I posted a digest from the SQL-2011 standard
and a conclusion as a design of a new patch.
Now I have more free time and I'm hacking it that way. The new patch
is at the very early stage, full of WIPs and TODOs. I hope it'll be
ready to be shown in a month (may be two).
I have just read both your patch and the one of Alvaro, but yours does
not touch pg_constraint in any way. Isn't that unexpected?
But it already forbids dropping NOT NULLs if they were set as result
of inheritance.
Okay, I'll drop any proposal on my side in this case. Looking forward
to seeing what you got for the first commit fest of 10.0.
--
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 wrote:
On Wed, Jun 15, 2016 at 10:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
IIRC, Alvaro posted a WIP patch for that awhile back. Not sure what the
current state is.Are you talking about that?
/messages/by-id/20110707213401.GA27098@alvh.no-ip.org
This is not a small patch :)Alvaro, others, any opinions?
I haven't looked at this in a while. I suppose I should get it in shape
sometime. Unless you would like to work on it?
Note that Vitaly Burovoy rebased it, but I think the rebase is wrong.
/messages/by-id/CAKOSWNkN6HSyatuys8xZxzRCR-KL1OkHS5-b9qd9bf1Rad3PLA@mail.gmail.com
--
Ãlvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jun 16, 2016 at 1:27 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
Michael Paquier wrote:
On Wed, Jun 15, 2016 at 10:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
IIRC, Alvaro posted a WIP patch for that awhile back. Not sure what the
current state is.Are you talking about that?
/messages/by-id/20110707213401.GA27098@alvh.no-ip.org
This is not a small patch :)Alvaro, others, any opinions?
I haven't looked at this in a while. I suppose I should get it in shape
sometime. Unless you would like to work on it?
Well, I could, but there is another big patch I am getting into shape
for the next CF (Ahem. scram. Ahem), so I am going to stay focused on
this one to have fuel for it during the CF. But I'm fine reviewing the
patch for the DROP NOT NULL.
Note that Vitaly Burovoy rebased it, but I think the rebase is wrong.
/messages/by-id/CAKOSWNkN6HSyatuys8xZxzRCR-KL1OkHS5-b9qd9bf1Rad3PLA@mail.gmail.com
Yes, it is definitely wrong.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 6/15/16, Michael Paquier <michael.paquier@gmail.com> wrote:
On Thu, Jun 16, 2016 at 12:10 PM, Vitaly Burovoy
<vitaly.burovoy@gmail.com> wrote:In the initial letter[1] I posted a digest from the SQL-2011 standard
and a conclusion as a design of a new patch.
Now I have more free time and I'm hacking it that way. The new patch
is at the very early stage, full of WIPs and TODOs. I hope it'll be
ready to be shown in a month (may be two).I have just read both your patch and the one of Alvaro, but yours does
not touch pg_constraint in any way. Isn't that unexpected?
The consensus was reached to use CHECK constraints instead of new type
of constrains.
Alvaro made attempt[1]/messages/by-id/1343682669-sup-2532@alvh.no-ip.org to write PoC in 2012 but it failed to apply on
top of master (and after solving conflicts led to core dumps) in Jan
2016.
I just rebased Alvaro's one to understand how he wanted to solve issue
and to run tests and queries. After all I sent rebased working patch
for anyone who wants to read it and try it without core dumps.
I have not published my patch for NOT NULLs yet.
Alvaro, the correct path[2]/messages/by-id/attachment/41886/catalog-notnull-2-c477e84_cleaned.patch in the second message[3]/messages/by-id/CAKOSWNnXbOY4pEiwN9wePOx8J+B44yTj40BQ8RVo-eWkdiGDFQ@mail.gmail.com -- Best regards, Vitaly Burovoy of the thread.
What's wrong in it (I got the source in the previous[1]/messages/by-id/1343682669-sup-2532@alvh.no-ip.org thread)?
[1]: /messages/by-id/1343682669-sup-2532@alvh.no-ip.org
[2]: /messages/by-id/attachment/41886/catalog-notnull-2-c477e84_cleaned.patch
[3]: /messages/by-id/CAKOSWNnXbOY4pEiwN9wePOx8J+B44yTj40BQ8RVo-eWkdiGDFQ@mail.gmail.com -- Best regards, Vitaly Burovoy
--
Best regards,
Vitaly Burovoy
--
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:
On Wed, Jun 15, 2016 at 10:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
My feeling is that we'd keep
the pg_attribute.attnotnull field and continue to drive actual enforcement
off that, but it would just reflect a summary of the pg_constraint state.
OK, I see. Hm, by storing this information I would actually think that
we want to drop this attnotnull so as we don't need to bother about
updating pg_attribute through the whole tree when dropping a NOT NULL
constraint on the parent, and we do not actually need to store this
information in two different places..
There are a couple of reasons for not removing attnotnull: one is to not
need to touch the executor for this, and another is to not break
client-side code that is accustomed to looking at attnotnull.
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