Re: [badalex@gmail.com: Re: [BUGS] Problem identifying constraints which should not be inherited]

On Fri, May 9, 2008 at 5:37 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Alex Hunsaker" <badalex(at)gmail(dot)com> 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
>