ALTER TABLE...ALTER COLUMN vs inheritance
in [PgSql]
|
Prev: [HACKERS] functional call named notation clashes with SQL feature
Next: [HACKERS] 9.0 Open Items: Code and Documentation sections
From: Bernd Helmle on 26 May 2010 17:37 --On 4. November 2009 09:57:27 -0500 Tom Lane <tgl(a)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
From: Selena Deckelmann on 15 Jun 2010 23:51 On Wed, May 26, 2010 at 2:37 PM, Bernd Helmle <mailings(a)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(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Selena Deckelmann on 16 Jun 2010 14:19 On Wed, Jun 16, 2010 at 5:31 AM, Bernd Helmle <mailings(a)oopsware.de> wrote: > > > --On 15. Juni 2010 20:51:21 -0700 Selena Deckelmann <selenamarie(a)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(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Bernd Helmle on 16 Jun 2010 08:31
--On 15. Juni 2010 20:51:21 -0700 Selena Deckelmann <selenamarie(a)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(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |