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

Forwarding this message so that it gets archived.

--
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Thu, Mar 27, 2008 at 5:14 AM, NikhilS <nikkhils@gmail.com> wrote:
> Hi,
>
> On Thu, Mar 20, 2008 at 7:36 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> >
> > More to the point, it takes a capability away from the user without
> > actually solving the problem we need to solve, namely to guarantee
> > consistency between parent and child constraints.  You can be sure
> > that there is someone out there who will complain that we've broken
> > his application when we disallow this, and we need to be able to
> > point to some positive benefit we got from it.
> >
>
> Agreed. So I think we need to implement the whole enchilada or nothing at
> all. We need to do the following for this:
>
> * Add logic to disallow ADD CONSTRAINT ONLY to parent of an inheritance
> hierarchy
>
> * Add logic to mark inherited constraints in the children:
> This can be achieved by introducing a new bool "coninherited" attribute in
> pg_constraint. This will be set to true on only those check constraints that
> are added to children via the inheritance mechanism
>
> * Add logic to disallow dropping inherited constraints directly on children
> Obviously they will get dropped if a DROP CONSTRAINT is fired on the parent.
> with recurse set to true (this is the default behaviour)
>
>  * Modify the pg_dump logic to use the new pg_constraint based attribute
> logic for version 80300 (or should it be PG_VERSION_NUM 80400?) onwards.
> Infact the current logic to determine if a check constraint is inherited is
> to compare its name with the parent constraint name and the comment already
> mentions that we should make changes in pg_constraint to avoid this
> rudimentary way of determing the inheritance :).
>
> Am important decision here is about adding a new attribute to pg_constraint
> as it is the only sane way of determining inherited constraints, but that
> will require an initdb. Comments?
>

Attached is a WIP patch I have been playing with in my spare time.  It
should take care of the first 2.  It does nothing for pg_dump or set
(not) null/set default.

Note it has some gross points (see comment in
src/backend/catalog/heap.c AddRelationRawConstraints) and the
regression tests I added are not quite up to par (and probably a bit
redundant).  But in the interest of saving work I thought i would post
it.

famous last words "It seems to work"...

diff stat for those that care:

 src/backend/catalog/heap.c                |   55 ++++-
 src/backend/catalog/index.c               |    4 +-
 src/backend/catalog/pg_constraint.c       |   51 ++++-
 src/backend/commands/tablecmds.c          |  366 ++++++++++++++++++----------
 src/backend/commands/typecmds.c           |    4 +-
 src/backend/nodes/copyfuncs.c             |    2 +
 src/backend/nodes/equalfuncs.c            |    2 +
 src/backend/nodes/outfuncs.c              |    3 +
 src/backend/parser/gram.y                 |    4 +
 src/backend/parser/parse_utilcmd.c        |    5 +
 src/backend/utils/cache/catcache.c        |    2 +-
 src/backend/utils/cache/syscache.c        |   12 +
 src/include/access/tupdesc.h              |    6 +-
 src/include/catalog/catversion.h          |    2 +-
 src/include/catalog/heap.h                |    1 +
 src/include/catalog/indexing.h            |    2 +
 src/include/catalog/pg_constraint.h       |   59 +++--
 src/include/nodes/parsenodes.h            |    4 +-
 src/include/utils/syscache.h              |   99 ++++----
 src/test/regress/expected/alter_table.out |    2 +-
 src/test/regress/expected/inherit.out     |   90 +++++++
 src/test/regress/sql/inherit.sql          |   45 ++++
 22 files changed, 605 insertions(+), 215 deletions(-)

(trimmed cc's)

Find attached inherited_constraint_v2.patch

Changes since v1:
-rebased against latest HEAD
-changed enum { Anum_pg_constraint_... } back into #define
Anum_pg_constraint_...
-remove whitespace damage I added
-fixed regression tests I added to be more robust
-fixed
   create table ac (a int constraint check_a check (a <> 0));
   create table bc (a int constraint check_a check (a <> 0)) inherits (ac);
   so it properly works (removed crud I put into
AddRelationRawConstraints and created a proper fix in DefineRelation)

diffstat to head:
 src/backend/catalog/heap.c                |   15 +-
 src/backend/catalog/index.c               |    4 +-
 src/backend/catalog/pg_constraint.c       |    7 +-
 src/backend/commands/tablecmds.c          |  488 ++++++++++++++++++++++-------
 src/backend/commands/typecmds.c           |    4 +-
 src/backend/nodes/copyfuncs.c             |    2 +
 src/backend/nodes/equalfuncs.c            |    2 +
 src/backend/nodes/outfuncs.c              |    3 +
 src/backend/parser/gram.y                 |    4 +
 src/backend/parser/parse_utilcmd.c        |    5 +
 src/backend/utils/cache/catcache.c        |    2 +-
 src/backend/utils/cache/syscache.c        |   12 +
 src/include/access/tupdesc.h              |    6 +-
 src/include/catalog/catversion.h          |    2 +-
 src/include/catalog/indexing.h            |    2 +
 src/include/catalog/pg_constraint.h       |   51 ++--
 src/include/nodes/parsenodes.h            |    4 +-
 src/include/utils/syscache.h              |   99 +++---
 src/test/regress/expected/alter_table.out |    2 +-
 src/test/regress/expected/inherit.out     |   76 +++++
 src/test/regress/sql/inherit.sql          |   38 +++
 21 files changed, 637 insertions(+), 191 deletions(-)

Hi Alex,


I was taking a look at this patch to add the pg_dump related changes. Just wanted to give you a heads up as this patch crashes if we run "make installcheck". Seems there is an issue introduced in the CREATE TABLE REFERENCES code path due to your patch (this is without my pg_dump changes just to be sure).  Looks like some memory overwrite issue. The trace is as follows:

Core was generated by `postgres: nikhils regression [local] CREATE TABLE                     '.
Program terminated with signal 11, Segmentation fault.
#0  0x08378024 in AllocSetCheck (context=0xa060368) at aset.c:1112
1112                            if (dsize > 0 && dsize < chsize && *chdata_end != 0x7E)
(gdb) bt
#0  0x08378024 in AllocSetCheck (context=0xa060368) at aset.c:1112
#1  0x0837704f in AllocSetDelete (context=0xa060368) at aset.c:487
#2  0x083783c2 in MemoryContextDelete (context=0xa060368) at mcxt.c:196
#3  0x083797fb in PortalDrop (portal=0xa0845bc, isTopCommit=0 '\0') at portalmem.c:448
#4  0x08281939 in exec_simple_query (
    query_string=0xa07e564 "CREATE TABLE enumtest_child (parent rainbow REFERENCES enumtest_parent);") at postgres.c:992
#5  0x082857d4 in PostgresMain (argc=4, argv=0x9ffbe28, username=0x9ffbcc4 "nikhils") at postgres.c:3550
#6  0x0824917b in BackendRun (port=0xa003180) at postmaster.c:3204
#7  0x082486a2 in BackendStartup (port=0xa003180) at postmaster.c:2827
#8  0x08245e9c in ServerLoop () at postmaster.c:1271
#9  0x082457fd in PostmasterMain (argc=3, argv=0x9ff9c60) at postmaster.c:1019
#10 0x081e1c03 in main (argc=3, argv=0x9ff9c60) at main.c:188


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

On Mon, Mar 31, 2008 at 2:36 AM, NikhilS <nikkhils@gmail.com> wrote:
> Hi Alex,

> I was taking a look at this patch to add the pg_dump related changes. Just
> wanted to give you a heads up as this patch crashes if we run "make
> installcheck". Seems there is an issue introduced in the CREATE TABLE
> REFERENCES code path due to your patch (this is without my pg_dump changes
> just to be sure).  Looks like some memory overwrite issue. The trace is as
> follows:

Ouch, sorry i did not reply sooner... been out with the flu.  Oddly
enough make check and make installcheck worked great on my 64 bit box.
 But on my laptop(32 bits) make check lights up like a christmas tree.
 Which is why I did not notice the problem. :(

Attached is a patch that fixes the problem... (it was debugging from
an earlier version)

Hi,


Yup, that fixes the problem.

PFA, a revised version of Alex' patch. I have added the relevant pg_dump related changes too. As I mentioned earlier, I still don't know whether Alex' syscache related changes are necessary, but I will leave it to the patch reviewers to decide :)

This combined patch meets the following TODOs:

* Add logic to disallow ADD CONSTRAINT ONLY to parent of an inheritance hierarchy

* Add logic to mark inherited constraints in the children:
This is achieved by introducing bool "conislocal" and int4 "coninhcount" attributes in pg_constraint. The behaviour of these 2 attributes is pretty similar to attislocal and attinhcount attributes in pg_attribute.

* Add logic to disallow dropping inherited constraints directly on children
Obviously they will get dropped if a DROP CONSTRAINT is fired on the parent. with recurse set to true (this is the default behaviour)

* Modify the pg_dump logic to use the new pg_constraint based attributes logic for versions above 80300 (or should it be PG_VERSION_NUM 80400?).

Please direct comments/feedback towards both me and Alex.

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

On Tue, Apr 1, 2008 at 2:40 AM, NikhilS <nikkhils@gmail.com> wrote:
> PFA, a revised version of Alex' patch. I have added the relevant pg_dump
> related changes too. As I mentioned earlier, I still don't know whether
> Alex' syscache related changes are necessary, but I will leave it to the
> patch reviewers to decide :)
>
> This combined patch meets the following TODOs:
>
>  * Add logic to disallow ADD CONSTRAINT ONLY to parent of an inheritance
> hierarchy
>
> * Add logic to mark inherited constraints in the children:
> This is achieved by introducing bool "conislocal" and int4 "coninhcount"
> attributes in pg_constraint. The behaviour of these 2 attributes is pretty
> similar to attislocal and attinhcount attributes in pg_attribute.
>
>  * Add logic to disallow dropping inherited constraints directly on children
>  Obviously they will get dropped if a DROP CONSTRAINT is fired on the
> parent. with recurse set to true (this is the default behaviour)
>
>  * Modify the pg_dump logic to use the new pg_constraint based attributes
> logic for versions above 80300 (or should it be PG_VERSION_NUM 80400?).
>
> Please direct comments/feedback towards both me and Alex.
>
>
> Regards,
> Nikhils
> --
> EnterpriseDB http://www.enterprisedb.com

Find attached v4 of the patch, changes since v3:

-updated/added catalog documentation for new coninhcount and conislocal columns.
-fixed some regression tests order by (causing them to fail randomly)
-some coding style fixes
-removed some unused vars

 Please direct comments/feedback towards both me and  NikhilS.

>On Tue, May 6, 2008 at 9:20 PM, Alex Hunsaker <badalex@gmail.com> wrote:
>
>  >Tom Lane <tgl@sss.pgh.pa.us> wrote:
>  >  Was there any particularly strong reason why you introduced the syscache
>  >  instead of working with the available indexes?
>  >
>  >                         regards, tom lane
>
>  None other than the syscache stuff was way easier to work with than
>  the 25-50 lines of boilerplate code that Ill need everywhere I use
>  CONSTRNAME. (see the hunk to MergeAttributesIntoExistsing for an
>  example of what i mean).   Not a big deal though, NikhilS was not sure
>  about those changes in the first place.
>
>  Ill just rip it out for now. Patch forthcoming.
>

Find attached a diff from v4-v5, and a full patch.

 src/backend/commands/tablecmds.c   |  242 +++++++++++++++++++++++-------------
 src/backend/utils/cache/syscache.c |   12 --
 src/include/catalog/indexing.h     |    2 -
 src/include/utils/syscache.h       |    1 -
 4 files changed, 153 insertions(+), 104 deletions(-)

Currently this loops through all the constraints for a relation (old
behavior of MergeAttributesIntoExisting)... Do you think its worth
adding a non-unique index to speed this up?  If so I can whip up a
patch real quick if you think its worth it... else

On Wed, May 7, 2008 at 12:20 AM, Alex Hunsaker <badalex@gmail.com> wrote:
>  Find attached a diff from v4-v5, and a full patch.
>
>   src/backend/commands/tablecmds.c   |  242 +++++++++++++++++++++++-------------
>
>  src/backend/utils/cache/syscache.c |   12 --
>
>  src/include/catalog/indexing.h     |    2 -
>   src/include/utils/syscache.h       |    1 -
>   4 files changed, 153 insertions(+), 104 deletions(-)
>
>  Currently this loops through all the constraints for a relation (old
>  behavior of MergeAttributesIntoExisting)... Do you think its worth
>  adding a non-unique index to speed this up?  If so I can whip up a
>  patch real quick if you think its worth it... else
>

*sigh* Here is a fiix for a possible bogus "failed to find constraint"
error when we are trying to drop a constraint that is not a check
constraint
(interesting no regression tests failed... caught it while reviewing
the patch I just posted)

*** a/src/backend/commands/tablecmds.c
--- /bsrc/backend/commands/tablecmds.c
*************** ATExecDropConstraint(Relation rel, const
*** 5080,5094 ****

                                con = (Form_pg_constraint) GETSTRUCT(tuple);

-                               if (con->contype != CONSTRAINT_CHECK)
-                                       continue;
-
                                if (strcmp(NameStr(con->conname),
                                                   constrName) != 0)
                                        continue;
                                else
                                        found = true;

                                if (con->coninhcount <= 0)
                                        elog(ERROR, "relation %u has
non-inherited constraint \"%s\"",
                                                childrelid, constrName);
--- 5080,5095 ----

                                con = (Form_pg_constraint) GETSTRUCT(tuple);

                                if (strcmp(NameStr(con->conname),
                                                   constrName) != 0)
                                        continue;
                                else
                                        found = true;

+                               /* Right now only CHECK constraints
can be inherited */
+                               if (con->contype != CONSTRAINT_CHECK)
+                                       continue;
+
                                if (con->coninhcount <= 0)
                                        elog(ERROR, "relation %u has
non-inherited constraint \"%s\"",
                                                childrelid, constrName);

"Alex Hunsaker" <badalex@gmail.com> writes:
> Currently this loops through all the constraints for a relation (old
> behavior of MergeAttributesIntoExisting)... Do you think its worth
> adding a non-unique index to speed this up?

No.  If we were to refactor pg_constraint as I mentioned earlier,
then it could have a natural primary key (reloid, constrname)
(replacing the existing nonunique index on reloid) and then a number
of things could be sped up.  But just piling more indexes on a
fundamentally bad design doesn't appeal to me ...

Will review the revised patch today.

            regards, tom lane

"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

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
>

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