On Fri, May 9, 2008 at 5:37 PM, Tom Lane <[EMAIL PROTECTED]> wrote: > "Alex Hunsaker" <[EMAIL PROTECTED]> writes: >> [ patch to change inherited-check-constraint behavior ] > > Applied after rather heavy editorializations. You didn't do very well on > getting it to work in multiple-inheritance scenarios, such as > > create table p (f1 int check (f1>0)); > create table c1 (f2 int) inherits (p); > create table c2 (f3 int) inherits (p); > create table cc () inherits (c1,c2); > > Here the same constraint is multiply inherited. The base case as above > worked okay, but adding the constraint to an existing inheritance tree > via ALTER TABLE, not so much.
Ouch. Ok Ill (obviously) review what you committed so I can do a lot better next time. Thanks for muddling through it! > I also didn't like the choice to add is_local/inhcount fields to > ConstrCheck: that struct is fairly heavily used, but you were leaving the > fields undefined/invalid in most code paths, which would surely lead to > bugs down the road when somebody expected them to contain valid data. > I considered extending the patch to always set them up, but rejected that > idea because ConstrCheck is essentially a creature of the executor, which > couldn't care less about constraint inheritance. After some reflection > I chose to put the fields in CookedConstraint instead, which is used only > in the table creation / constraint addition code paths. That required > a bit of refactoring of the API of heap_create_with_catalog, but I think > it ended up noticeably cleaner: constraints and defaults are fed to > heap.c in only one format now. That sounds *way* cleaner and hopefully got rid of some of those gross hacks I had to do. Interestingly enough thats similar to how I initially started doing it. But it felt way to intrusive, so i dropped it. Course I then failed the non-intrusive with the ConstrCheck changes... > I found one case that has not really worked as intended for a long time: > ALTER TABLE ADD CHECK ... (that is, ADD CONSTRAINT without specifying > a constraint name) failed to ensure that the same constraint name was used > at child tables as at the parent, and thus the constraints ended up not > being seen as related at all. Fixing this was a bit ugly since it meant > that ADD CONSTRAINT has to recurse to child tables during Phase 2 after > all, and has to be able to add work queue entries for Phase 3 at that > time, which is not something needed by any other ALTER TABLE operation. Ouch... > I'm not sure if we ought to try to back-patch that --- it'd be a > behavioral change with non-obvious implications. In the back branches, > ADD CHECK followed by DROP CONSTRAINT will end up not deleting the > child-table constraints, which is probably a bug but I wouldn't be > surprised if applications were depending on the behavior. Given the lack complaints it does not seem worth a back patch IMHO. > 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
