Thread: 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@oopsware.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@sss.pgh.pa.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@sss.pgh.pa.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@oopsware.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@sss.pgh.pa.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@gmail.com> writes:
> On Thu, Nov 12, 2009 at 13:55, Tom Lane <tgl@sss.pgh.pa.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@oopsware.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@gmail.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@sss.pgh.pa.us> wrote: > Alex Hunsaker <badalex@gmail.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@gmail.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@sss.pgh.pa.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
Attachment
On Wed, May 26, 2010 at 2:37 PM, Bernd Helmle <mailings@oopsware.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 Wed, Jun 16, 2010 at 5:31 AM, Bernd Helmle <mailings@oopsware.de> wrote: > > > --On 15. Juni 2010 20:51:21 -0700 Selena Deckelmann <selenamarie@gmail.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 15. Juni 2010 20:51:21 -0700 Selena Deckelmann <selenamarie@gmail.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 16. Juni 2010 14:31:00 +0200 Bernd Helmle <mailings@oopsware.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@oopsware.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