Thread: Prevent ALTER TABLE DROP NOT NULL on child tables if parent column has it
Prevent ALTER TABLE DROP NOT NULL on child tables if parent column has it
From
Michael Paquier
Date:
Hi all, A couple of months back the $subject has been mentioned, though nobody actually wrote a patch to prevent that: http://www.postgresql.org/message-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
Attachment
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
Re: Prevent ALTER TABLE DROP NOT NULL on child tables if parent column has it
From
Michael Paquier
Date:
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? https://www.postgresql.org/message-id/20110707213401.GA27098%40alvh.no-ip.org This is not a small patch :) Alvaro, others, any opinions? -- Michael
Re: Prevent ALTER TABLE DROP NOT NULL on child tables if parent column has it
From
Vitaly Burovoy
Date:
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]. I rebased[2] Alvaro's patch to the actual master at that time, but I have not repeated it since then. 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). But it already forbids dropping NOT NULLs if they were set as result of inheritance. [1] https://www.postgresql.org/message-id/flat/CAKOSWNkN6HSyatuys8xZxzRCR-KL1OkHS5-b9qd9bf1Rad3PLA%40mail.gmail.com [2] https://www.postgresql.org/message-id/attachment/41886/catalog-notnull-2-c477e84_cleaned.patch -- Best regards, Vitaly Burovoy
Re: Prevent ALTER TABLE DROP NOT NULL on child tables if parent column has it
From
Michael Paquier
Date:
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
Re: Prevent ALTER TABLE DROP NOT NULL on child tables if parent column has it
From
Alvaro Herrera
Date:
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? > https://www.postgresql.org/message-id/20110707213401.GA27098%40alvh.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. https://www.postgresql.org/message-id/CAKOSWNkN6HSyatuys8xZxzRCR-KL1OkHS5-b9qd9bf1Rad3PLA@mail.gmail.com -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Prevent ALTER TABLE DROP NOT NULL on child tables if parent column has it
From
Michael Paquier
Date:
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? >> https://www.postgresql.org/message-id/20110707213401.GA27098%40alvh.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. > https://www.postgresql.org/message-id/CAKOSWNkN6HSyatuys8xZxzRCR-KL1OkHS5-b9qd9bf1Rad3PLA@mail.gmail.com Yes, it is definitely wrong. -- Michael
Re: Prevent ALTER TABLE DROP NOT NULL on child tables if parent column has it
From
Vitaly Burovoy
Date:
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] 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] in the second message[3] of the thread. What's wrong in it (I got the source in the previous[1] thread)? [1] https://www.postgresql.org/message-id/flat/1343682669-sup-2532@alvh.no-ip.org [2] https://www.postgresql.org/message-id/attachment/41886/catalog-notnull-2-c477e84_cleaned.patch [3] https://www.postgresql.org/message-id/CAKOSWNnXbOY4pEiwN9wePOx8J%2BB44yTj40BQ8RVo-eWkdiGDFQ%40mail.gmail.com -- Best regards, Vitaly Burovoy
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