Re: ALTER TABLE...ALTER COLUMN vs inheritance

I just run across an issue with ALTER TABLE and inheritance (i don't know
wether this is of the same kind of issue KaiGai reported today, so i keep
it on a separate thread).

Consider the following workflow:

CREATE TABLE foo(id integer NOT NULL, val text NOT NULL);
CREATE TABLE foo2(another_id integer NOT NULL) INHERITS(foo);

Now someone decides he doesn't want the NOT NULL constraint on the
inherited column "val" anymore:

ALTER TABLE foo2 ALTER COLUMN val DROP NOT NULL;

This breaks at least pg_dump, which will produce unrestorable dumps at
least when using NULL-values as intended on foo2.

I havent thought about that very deep, but we already force ALTER TABLE
... INHERIT that columns have to be NOT NULL when the new parent already
has a constraint on it, so it seems to me that the best way to repair this
deficiency is to introduce the same rules in ALTER TABLE...ALTER COLUMN,
too.

The described workflow is currently used in OpenERP, which employs such an
inheritance structure on some of its tables (however, making ALTER TABLE
here more strict will surely break OpenERP, but that is another story).

Opinions?

--
Thanks

Bernd

Bernd Helmle <mailings(at)oopsware(dot)de> writes:
> Consider the following workflow:

> CREATE TABLE foo(id integer NOT NULL, val text NOT NULL);
> CREATE TABLE foo2(another_id integer NOT NULL) INHERITS(foo);

> Now someone decides he doesn't want the NOT NULL constraint on the
> inherited column "val" anymore:

> ALTER TABLE foo2 ALTER COLUMN val DROP NOT NULL;

Yeah, this is a known issue. The ALTER should be rejected, but it is
not, because we don't have enough infrastructure to notice that the
constraint is inherited and logically can't be dropped. I think the
consensus was that the way to fix this (along with some other problems)
is to start representing NOT NULL constraints in pg_constraint, turning
attnotnull into just a bit of denormalization for performance.

regards, tom lane

--On 4. November 2009 09:57:27 -0500 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> I think the
> consensus was that the way to fix this (along with some other problems)
> is to start representing NOT NULL constraints in pg_constraint, turning
> attnotnull into just a bit of denormalization for performance.

Ah okay, should have checked the archive more carefully. I think i give it
a try...

--
Thanks

Bernd

--On 4. November 2009 09:57:27 -0500 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> I think the
> consensus was that the way to fix this (along with some other problems)
> is to start representing NOT NULL constraints in pg_constraint, turning
> attnotnull into just a bit of denormalization for performance.

I've just started looking into this and wonder how this should look like.
My first idea is to just introduce a special contype in pg_constraint
representing a NOT NULL constraint on a column, which holds all required
information to do the mentioned maintenance stuff on them and to keep most
of the current infrastructure. Utility commands need to track all changes
in pg_constraint and keep pg_attribute.attnotnull up to date.

Another possibility is to change the representation of NOT NULL to be a
CHECK constraint (e.g. CHECK(col IS NOT NULL)) internally and leave all the
responsibility up to the current existing check constraint infrastructure
(which already does the right thing for inheritance, e.g. it's not possible
to drop such a constraint if it was inherited).
ALTER TABLE ... SET NOT NULL and DROP NOT NULL will be just syntactic sugar
then.
I don't know the original design decisions for the current representation,
but it seems it wasn't essential?

Of course, there's still the requirement to special case those check
constraints in various places, since pg_dump and psql have to do the right
thing.

--
Thanks

Bernd

Bernd Helmle <mailings(at)oopsware(dot)de> writes:
> My first idea is to just introduce a special contype in pg_constraint
> representing a NOT NULL constraint on a column, which holds all required
> information to do the mentioned maintenance stuff on them and to keep most
> of the current infrastructure. Utility commands need to track all changes
> in pg_constraint and keep pg_attribute.attnotnull up to date.

> Another possibility is to change the representation of NOT NULL to be a
> CHECK constraint (e.g. CHECK(col IS NOT NULL)) internally and leave all the
> responsibility up to the current existing check constraint infrastructure
> (which already does the right thing for inheritance, e.g. it's not possible
> to drop such a constraint if it was inherited).

I'd go for the first of those, for sure. Testing attnotnull is
significantly cheaper than enforcing a generic constraint expression,
and NOT NULL is a sufficiently common case to be worth worrying about
optimizing it. Furthermore, removing attnotnull would break an unknown
but probably not-negligible amount of client-side code that cares
whether columns are known not null (I think both JDBC and ODBC track
that). And it would significantly complicate code in the backend that
wants to determine whether a column is known not null --- there are
several proposals for planner optimizations that would depend on knowing
that, for example.

You will find yourself copying-and-pasting a fair amount of the
check-constraint code if you implement this as a completely separate
code path, and so it might be worthwhile to be creative about exactly
what the pg_constraint representations look like --- maybe you can
design it to share some of that code. But I recommend strongly that
attnotnull stay there.

regards, tom lane

On Thu, Nov 12, 2009 at 13:55, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> I'd go for the first of those, for sure.  Testing attnotnull is
> significantly cheaper than enforcing a generic constraint expression,
> and NOT NULL is a sufficiently common case to be worth worrying about
> optimizing it.

When I looked at doing this, I thought about just using check
constraints just for the book keeping and leaving attnotnull as it is.
If would be easier, but it seemed quite ugly.

Alex Hunsaker <badalex(at)gmail(dot)com> writes:
> On Thu, Nov 12, 2009 at 13:55, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I'd go for the first of those, for sure. Testing attnotnull is
>> significantly cheaper than enforcing a generic constraint expression,
>> and NOT NULL is a sufficiently common case to be worth worrying about
>> optimizing it.

> When I looked at doing this, I thought about just using check
> constraints just for the book keeping and leaving attnotnull as it is.

Yeah, you could definitely attack it like that. The code that fixes up
attnotnull would have to look for check constraints that look like "foo
NOT NULL" rather than something more instantly recognizable, but
presumably ALTER TABLE is not a performance-critical path.

regards, tom lane

On Thu, Nov 12, 2009 at 11:56, Bernd Helmle <mailings(at)oopsware(dot)de> wrote:
> I've just started looking into this and wonder how this should look like.

IIRC another motivation for moving them into pg_constraint was we
could then give them names as required by the spec (unless I got mixed
up with defaults). Looking at the 2003 spec I don't see any grammar
for that, so either I cant find it (likely) or its not there. Either
way I see something like the below options:

ALTER TABLE ALTER COLUMN ADD CONSTRAINT my_not_null NOT NULL;
[ we dont currently support add constraint on ALTER COLUMN AFAICT...
but it might be nice? ]
-or-
ALTER TABLE ADD CONSTRAINT my_not_null NOT NULL (column);
-or-
ALTER TABLE ALTER COLUMN column SET NOT NULL 'name';

Comments?

Anyway Bernd if you are working on this great! If not lemme know, Ill
plan on having something for the next commit feast. Though I still
may never get around to it :(.

FYI defaults have the same problem. Would it be awkward would it be
to use pg_constraint for the book keeping as well? [ and by that I
really mean ALTER TABLE ADD CONSTRAINT my_default DEFAULT .... so you
can give them a name ]

Alex Hunsaker <badalex(at)gmail(dot)com> writes:
> FYI defaults have the same problem. Would it be awkward would it be
> to use pg_constraint for the book keeping as well? [ and by that I
> really mean ALTER TABLE ADD CONSTRAINT my_default DEFAULT .... so you
> can give them a name ]

That sounds moderately insane to me. Why would you need a name?
What would it mean to have more than one default attached to a column?

regards, tom lane

On Mon, Nov 16, 2009 at 11:45, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Alex Hunsaker <badalex(at)gmail(dot)com> writes:
>> FYI defaults have the same problem.   Would it be awkward would it be
>> to use pg_constraint for the book keeping as well? [ and by that I
>> really mean ALTER TABLE ADD CONSTRAINT my_default DEFAULT .... so you
>> can give them a name ]
>
> That sounds moderately insane to me.  Why would you need a name?

I don't care strongly enough to argue for them. I just thought if it
was something the spec said or someone wanted it would be easy to add
while in the area :) Sorry for the insane hand waving.

We already have pg_attrdef, all we really need is the inhcount and
islocal columns on that. No reason to bring pg_constraint into it all
at.

> What would it mean to have more than one default attached to a column?

"It would be like so far out dude"

Ok so my hippie impression needs work...

--On 16. November 2009 11:00:33 -0700 Alex Hunsaker <badalex(at)gmail(dot)com>
wrote:

> Anyway Bernd if you are working on this great! If not lemme know, Ill
> plan on having something for the next commit feast. Though I still
> may never get around to it :(.

I'm just working on it.

The current patch assigns <tablename>_<col>_not_null (by using
ChooseConstraintName()) as the constraint name to NOT NULL, i record the
attnum this NOT NULL belongs to in conkey. So far so good, creating the
constraints already works, i'm going to adjust the utility commands now.
One thing i just stumpled across: I guess we want the same behavior for
dropping NOT NULL constraints recursively like we already do for CHECK
constraints.

I thought i can reuse some of the infrastructure of ATExecDropConstraint(),
but this seems somekind awful, since it requires a constraint name and we
already did the scanning of pg_constraint up to this point. Since i don't
like duplicating too much code i'm thinking about splitting
ATExecDropConstraint() in an additional function
ATExecDropConstraintInternal(), which does the real work for a given
constraint OID.

--
Thanks

Bernd

--On 4. November 2009 09:57:27 -0500 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Yeah, this is a known issue. The ALTER should be rejected, but it is
> not, because we don't have enough infrastructure to notice that the
> constraint is inherited and logically can't be dropped. I think the
> consensus was that the way to fix this (along with some other problems)
> is to start representing NOT NULL constraints in pg_constraint, turning
> attnotnull into just a bit of denormalization for performance.

Lost it a little from my radar, but here's is a first shot on this issue
now. The patch creates a new CONSTRAINT_NOTNULL contype and assigns all
required information for the NOT NULL constraint to it. Currently the
constraint records the attribute number it belongs to and manages the
inheritance properties. Passes regression tests with some adjustments to
pg_constraint output.

The patch as it stands employs a dedicated code path for
ATExecDropNotNull(), thus duplicates the behavior of
ATExecDropConstraint(). I'm not really satisfied with this, but i did it
this way to prevent some heavy conditional rearrangement in
ATExecDropConstraint(). Maybe its worth to move the code to adjust
constraint inheritance properties into a separate function.

There's also a remaining issue which needs to be addressed: currently
pg_get_constraintdef_worker() is special case'd to dump the correct syntax
for the NOT NULL constraint, which is totally redundant. I'm not sure how
to do it correctly, since for example ATExecAlterColumnType() actually uses
it to restore dependent constraints on a column. We might want to just move
the special case there.

I understand that people are busy with the remaining open items list for
9.0, so it's okay to discuss this during the upcoming reviewfest.

Bernd

On Wed, May 26, 2010 at 2:37 PM, Bernd Helmle <mailings(at)oopsware(dot)de> wrote:

> Lost it a little from my radar, but here's is a first shot on this issue
> now. The patch creates a new CONSTRAINT_NOTNULL contype and assigns all
> required information for the NOT NULL constraint to it. Currently the
> constraint records the attribute number it belongs to and manages the
> inheritance properties. Passes regression tests with some adjustments to
> pg_constraint output.

Confirmed that this tests fine against commit id
0dca7d2f70872a242d4430c4c3aa01ba8dbd4a8c

I also wrote a little test script and verified that it throws an error
when I try to remove a constraint from the parent table.

Should an explicit test be added for this?

There are some spelling and grammar errors in comments that I can help
fix if you want the help.

-selena

--
http://chesnok.com/daily - me

--On 15. Juni 2010 20:51:21 -0700 Selena Deckelmann <selenamarie(at)gmail(dot)com>
wrote:

> Confirmed that this tests fine against commit id
> 0dca7d2f70872a242d4430c4c3aa01ba8dbd4a8c
>
> I also wrote a little test script and verified that it throws an error
> when I try to remove a constraint from the parent table.
>

Thanks for looking at this.

Please note that the main purpose of this patch is to protect *child*
tables against dropping NOT NULL constraints inherited from a parent table.
This could lead to unrestorable dumps formerly.

> Should an explicit test be added for this?
>

I think so, yes.

> There are some spelling and grammar errors in comments that I can help
> fix if you want the help.

You're welcome. I have pushed my branch to my git repos as well, if you
like to start from there:

<http://git.postgresql.org/gitweb?p=bernd_pg.git;a=shortlog;h=refs/heads/notnull_constraint>

--
Thanks

Bernd

On Wed, Jun 16, 2010 at 5:31 AM, Bernd Helmle <mailings(at)oopsware(dot)de> wrote:
>
>
> --On 15. Juni 2010 20:51:21 -0700 Selena Deckelmann <selenamarie(at)gmail(dot)com>
> wrote:
>
>> Confirmed that this tests fine against commit id
>> 0dca7d2f70872a242d4430c4c3aa01ba8dbd4a8c
>>
>> I also wrote a little test script and verified that it throws an error
>> when I try to remove a constraint from the parent table.
>>
>
> Thanks for looking at this.
>
> Please note that the main purpose of this patch is to protect *child* tables
> against dropping NOT NULL constraints inherited from a parent table. This
> could lead to unrestorable dumps formerly.

Yes! I didn't say that right earlier -- sorry I should have attached
the test. I'll just try to add it and send it to you in patch form.

>> Should an explicit test be added for this?
>>
>
> I think so, yes.
>
>> There are some spelling and grammar errors in comments that I can help
>> fix if you want the help.
>
> You're welcome. I have pushed my branch to my git repos as well, if you like
> to start from there:
>
> <http://git.postgresql.org/gitweb?p=bernd_pg.git;a=shortlog;h=refs/heads/notnull_constraint>

Awesome! I'll have a look this afternoon.

And, I didn't really know what to say about the rest of the issues you
brought up around structuring the code, and the couple TODOs still
left in the patch.

-selena

--
http://chesnok.com/daily - me

--On 16. Juni 2010 14:31:00 +0200 Bernd Helmle <mailings(at)oopsware(dot)de> wrote:

>> There are some spelling and grammar errors in comments that I can help
>> fix if you want the help.
>
> You're welcome. I have pushed my branch to my git repos as well, if you
> like to start from there:
>
> <http://git.postgresql.org/gitweb?p=bernd_pg.git;a=shortlog;h=refs/heads/
> notnull_constraint>

I'm going to delay this patch until the next commitfest. I'm able to work
on it only sporadically during the next two weeks, and some remaining
issues need my attention on this patch:

- ALTER TABLE SET NOT NULL works not properly
- ALTER TABLE child NO INHERIT parent leaves inconistent state in
pg_constraint
- Special case in pg_get_constraintdef_worker() needs more thinking

--
Thanks

Bernd

--On 23. Juli 2010 09:23:56 +0200 Bernd Helmle <mailings(at)oopsware(dot)de> wrote:

> I'm going to delay this patch until the next commitfest. I'm able to work
> on it only sporadically during the next two weeks, and some remaining
> issues need my attention on this patch:
>
> - ALTER TABLE SET NOT NULL works not properly
> - ALTER TABLE child NO INHERIT parent leaves inconistent state in
> pg_constraint
> - Special case in pg_get_constraintdef_worker() needs more thinking

Attached is my current progress on this work. It handles ALTER TABLE ...
[NO] INHERIT and NOT NULL constraints the same way we do with CHECK
constraints now. ALTER TABLE SET NOT NULL still lags support for this (ran
out of time to this commitfest deadline), but if nobody complains i would
like to add this to the current commitfest as WIP to hear other opinions
about the current approach again.

--
Thanks

Bernd