Обсуждение: 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:
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

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

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

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

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

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

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

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