Re: [HACKERS] [PATCHES] [EMAIL PROTECTED]: Re: [BUGS] Problem identifying constraints which should not be inherited]
Hi, On Sat, May 10, 2008 at 6:11 AM, Alex Hunsaker [EMAIL PROTECTED] wrote: 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 (f10)); 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
Re: [HACKERS] [PATCHES] [EMAIL PROTECTED]: Re: [BUGS] Problem identifying constraints which should not be inherited]
Nikhils [EMAIL PROTECTED] 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-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCHES] [EMAIL PROTECTED]: Re: [BUGS] Problem identifying constraints which should not be inherited]
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 (f10)); 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-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCHES] [EMAIL PROTECTED]: Re: [BUGS] Problem identifying constraints which should not be inherited]
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 (f10)); 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
Re: [HACKERS] [PATCHES] [EMAIL PROTECTED]: Re: [BUGS] Problem identifying constraints which should not be inherited]
On Wed, May 7, 2008 at 12:20 AM, Alex Hunsaker [EMAIL PROTECTED] 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); -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCHES] [EMAIL PROTECTED]: Re: [BUGS] Problem identifying constraints which should not be inherited]
Alex Hunsaker [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCHES] [EMAIL PROTECTED]: Re: [BUGS] Problem identifying constraints which should not be inherited]
Alex Hunsaker [EMAIL PROTECTED] writes: [ patch to fix behavior of inherited constraints ] Looking over this patch, I see that it introduces a syscache on pg_constraint (conrelid, conname), which requires a unique index underlying it. This is not workable because domain constraint entries in pg_constraint will have conrelid = 0. The index would therefore have the effect of forbidding the same constraint name to be used for two different domains' constraints. The fact that pg_constraint stores both relation and domain constraints is a fairly ugly crock, not least because it means there is no natural primary key for the table. I've thought for some time that we should split it into two catalogs. (We could provide a union view to avoid breaking clients that look at it.) However it seems a bit ill-advised to tackle that change as an essential part of this patch. Was there any particularly strong reason why you introduced the syscache instead of working with the available indexes? 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
Re: [HACKERS] [PATCHES] [EMAIL PROTECTED]: Re: [BUGS] Problem identifying constraints which should not be inherited]
On Tue, May 6, 2008 at 7:57 PM, Tom Lane [EMAIL PROTECTED] wrote: Alex Hunsaker [EMAIL PROTECTED] writes: [ patch to fix behavior of inherited constraints ] Looking over this patch, I see that it introduces a syscache on pg_constraint (conrelid, conname), which requires a unique index underlying it. This is not workable because domain constraint entries in pg_constraint will have conrelid = 0. The index would therefore have the effect of forbidding the same constraint name to be used for two different domains' constraints. The fact that pg_constraint stores both relation and domain constraints is a fairly ugly crock, not least because it means there is no natural primary key for the table. I've thought for some time that we should split it into two catalogs. (We could provide a union view to avoid breaking clients that look at it.) However it seems a bit ill-advised to tackle that change as an essential part of this patch. 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCHES] [EMAIL PROTECTED]: Re: [BUGS] Problem identifying constraints which should not be inherited]
Hi Alex,
On Sun, Mar 30, 2008 at 7:10 AM, Alex Hunsaker [EMAIL PROTECTED] wrote:
(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)
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
1112if (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
Re: [HACKERS] [PATCHES] [EMAIL PROTECTED]: Re: [BUGS] Problem identifying constraints which should not be inherited]
On Mon, Mar 31, 2008 at 2:36 AM, NikhilS [EMAIL PROTECTED] 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)
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index f105d39..7d12156 100644
*** a/src/backend/parser/parse_utilcmd.c
--- /bsrc/backend/parser/parse_utilcmd.c
*** transformColumnDefinition(ParseState *ps
*** 409,417
{
constraint = lfirst(clist);
- constraint-is_local = true;
- constraint-inhcount = 0;
-
/*
* If this column constraint is a FOREIGN KEY constraint, then we fill
* in the current attribute's name and throw it into the list of FK
--- 409,414
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
