SEO

"Alex Hunsaker" <badalex@gmail.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.

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.

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.

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.

regards, tom lane

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

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

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

Hi,
On Sat, May 10, 2008 at 6:11 AM, Alex Hunsaker <badalex@gmail.com> wrote:

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


Ouchie indeed!

> 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.
>
Yeah, same IMHO. I do hope we have covered things properly for inherited
check constraints by now. One minor thing that myself and Alex discussed was
the usage of "child tables" in tablecmds.c, especially in error messages.
Again English is not my native language, but shouldn't that be worded as
"children tables"? Admittedly even this does not sound any better than
"child tables" though . It is nit-picking really, but I can submit a
cleanup patch to reword this if the list thinks so..

Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com

Nikhils <nikkhils@gmail.com> writes:
> ... One minor thing that myself and Alex discussed was
> the usage of "child tables" in tablecmds.c, especially in error messages.
> Again English is not my native language, but shouldn't that be worded as
> "children tables"? Admittedly even this does not sound any better than
> "child tables" though .

No, "child tables" sounds better to me. English doesn't usually
pluralize adjectives.

regards, tom lane

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches