Re: [HACKERS] ALTER TABLE...ALTER COLUMN vs inheritance
--On 16. Juni 2010 14:31:00 +0200 Bernd Helmle maili...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TABLE...ALTER COLUMN vs inheritance
On Wed, Jun 16, 2010 at 5:31 AM, Bernd Helmle maili...@oopsware.de wrote: --On 15. Juni 2010 20:51:21 -0700 Selena Deckelmann selenama...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TABLE...ALTER COLUMN vs inheritance
--On 15. Juni 2010 20:51:21 -0700 Selena Deckelmann selenama...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TABLE...ALTER COLUMN vs inheritance
On Wed, May 26, 2010 at 2:37 PM, Bernd Helmle maili...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TABLE...ALTER COLUMN vs inheritance
--On 4. November 2009 09:57:27 -0500 Tom Lane t...@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 notnull_constraint.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TABLE...ALTER COLUMN vs inheritance
On Thu, Nov 12, 2009 at 11:56, Bernd Helmle maili...@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 ] -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TABLE...ALTER COLUMN vs inheritance
Alex Hunsaker bada...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TABLE...ALTER COLUMN vs inheritance
On Mon, Nov 16, 2009 at 11:45, Tom Lane t...@sss.pgh.pa.us wrote: Alex Hunsaker bada...@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... -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TABLE...ALTER COLUMN vs inheritance
--On 16. November 2009 11:00:33 -0700 Alex Hunsaker bada...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TABLE...ALTER COLUMN vs inheritance
--On 4. November 2009 09:57:27 -0500 Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TABLE...ALTER COLUMN vs inheritance
Bernd Helmle maili...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TABLE...ALTER COLUMN vs inheritance
On Thu, Nov 12, 2009 at 13:55, Tom Lane t...@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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TABLE...ALTER COLUMN vs inheritance
Alex Hunsaker bada...@gmail.com writes: On Thu, Nov 12, 2009 at 13:55, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TABLE...ALTER COLUMN vs inheritance
Bernd Helmle maili...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TABLE...ALTER COLUMN vs inheritance
--On 4. November 2009 09:57:27 -0500 Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
