Re: [HACKERS] alter table only ... drop constraint broken in HEAD
On Fri, Oct 7, 2011 at 5:45 PM, Alex Hunsaker bada...@gmail.com wrote: If I find the time maybe Ill submit something along these lines for the next commit fest. So i just picked up the non-inherited constraints patch and quickly ran into the same problem and found this thread. I think it makes sense to hold off on this patch until these issues are resolved. Because we really do need to test the cases when adding or removing child tables that have constraints with the same name as non-inherited parent tables. And I'm not sure what will happen in these cases once these issues are resolved. -- greg -- 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] alter table only ... drop constraint broken in HEAD
On Sun, Oct 9, 2011 at 09:17, Greg Stark st...@mit.edu wrote: On Fri, Oct 7, 2011 at 5:45 PM, Alex Hunsaker bada...@gmail.com wrote: If I find the time maybe Ill submit something along these lines for the next commit fest. So i just picked up the non-inherited constraints patch and quickly ran into the same problem and found this thread. I think it makes sense to hold off on this patch until these issues are resolved. Because we really do need to test the cases when adding or removing child tables that have constraints with the same name as non-inherited parent tables. And I'm not sure what will happen in these cases once these issues are resolved. Doesn't someone just need to commit Roberts patch? I suppose it could do with a better review than my eyeballing... Maybe thats where the hang up is? -- 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] alter table only ... drop constraint broken in HEAD
On Sun, Oct 9, 2011 at 1:56 PM, Alex Hunsaker bada...@gmail.com wrote: So i just picked up the non-inherited constraints patch and quickly ran into the same problem and found this thread. I think it makes sense to hold off on this patch until these issues are resolved. Because we really do need to test the cases when adding or removing child tables that have constraints with the same name as non-inherited parent tables. And I'm not sure what will happen in these cases once these issues are resolved. Doesn't someone just need to commit Roberts patch? Yeah, I've just been mostly AFK for ~53 hours. It's committed now. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] alter table only ... drop constraint broken in HEAD
On Thu, Oct 6, 2011 at 11:38 PM, Alex Hunsaker bada...@gmail.com wrote: Oh, I see the problem, and I now agree that it's the DROP CONSTRAINT code that is buggy. Want me to roll this fix in as part of the alter table only constraint patch? Or keep it split out? We might want to backpatch to at least 8.3 where HOT was introduced (yes looks like the bug existed back then). I suppose its a fairly narrow chance to hit this bug so I could see the argument for not back patching... Yeah, I'm not inclined to back-patch it. The chance of hitting this in older versions must be very small, or somebody would have noticed by now. If we get a report from the field, we can always back-patch it then, but right now it doesn't seem worth taking any risks for. On a related note, your fix seems slightly fragile to me ... because we're pulling a CCI out of the innermost loop, but a CCI can still happen inside that same loop if we recurse, because the recursive call will do one before returning. Now, maybe that's OK, because the recursive call in that case will just be deleting the tuple, so there won't be a new version for us to stumble over. The only way we could trip up in that case is if there were two identically named constraints. We'd have to visit the first tuple, update it, then visit the second tuple, recurse (thus incrementing the command counter), and then visit the updated version of the first tuple. And that should be impossible, because we've got code to disallow multiple constraints on the same relation with the same name (though no unique index, for some reason). Still, that's a long chain of reasoning, so I'm wondering if we can't come up with something that is more obviously correct. If we're confident that the inner loop here should never iterate more than once (i.e. the lack of a unique index is not an ominous sign) then maybe we should just rewrite this so that the inner loop scans until it finds a match and then terminates. Then, outside the loop, we check whether a tuple was found and if so process it - but without ever going back to look for another one. See attached. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company drop-constraint.patch Description: Binary data -- 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] alter table only ... drop constraint broken in HEAD
On Fri, Oct 7, 2011 at 07:53, Robert Haas robertmh...@gmail.com wrote: The only way we could trip up in that case is if there were two identically named constraints. We'd have to visit the first tuple, update it, then visit the second tuple, recurse (thus incrementing the command counter), and then visit the updated version of the first tuple. And that should be impossible, because we've got code to disallow multiple constraints on the same relation with the same name (though no unique index, for some reason). Surely an oversight... Still, that's a long chain of reasoning, so I'm wondering if we can't come up with something that is more obviously correct. If we're confident that the inner loop here should never iterate more than once (i.e. the lack of a unique index is not an ominous sign) then maybe we should just rewrite this so that the inner loop scans until it finds a match and then terminates. Then, outside the loop, we check whether a tuple was found and if so process it - but without ever going back to look for another one. See attached. I eyeballed it and it does indeed seem simpler. My only thought is perhaps we should add that missing unique index on (conrelid, conname). If we are not going to support duplicate names in the code, we might as well enforce it. No? -- 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] alter table only ... drop constraint broken in HEAD
On Fri, Oct 7, 2011 at 11:19 AM, Alex Hunsaker bada...@gmail.com wrote: On Fri, Oct 7, 2011 at 07:53, Robert Haas robertmh...@gmail.com wrote: The only way we could trip up in that case is if there were two identically named constraints. We'd have to visit the first tuple, update it, then visit the second tuple, recurse (thus incrementing the command counter), and then visit the updated version of the first tuple. And that should be impossible, because we've got code to disallow multiple constraints on the same relation with the same name (though no unique index, for some reason). Surely an oversight... Still, that's a long chain of reasoning, so I'm wondering if we can't come up with something that is more obviously correct. If we're confident that the inner loop here should never iterate more than once (i.e. the lack of a unique index is not an ominous sign) then maybe we should just rewrite this so that the inner loop scans until it finds a match and then terminates. Then, outside the loop, we check whether a tuple was found and if so process it - but without ever going back to look for another one. See attached. I eyeballed it and it does indeed seem simpler. My only thought is perhaps we should add that missing unique index on (conrelid, conname). If we are not going to support duplicate names in the code, we might as well enforce it. No? Not sure. There could be performance or other ramifications to that. For now I'm more interested in fixing this particular bug than I am in getting into a wider world of re-engineering... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] alter table only ... drop constraint broken in HEAD
On Fri, Oct 7, 2011 at 09:50, Robert Haas robertmh...@gmail.com wrote: On Fri, Oct 7, 2011 at 11:19 AM, Alex Hunsaker bada...@gmail.com wrote: My only thought is perhaps we should add that missing unique index on (conrelid, conname). If we are not going to support duplicate names in the code, we might as well enforce it. No? Not sure. There could be performance or other ramifications to that. For now I'm more interested in fixing this particular bug than I am in getting into a wider world of re-engineering... Yeah, looking at the code a bit closer we would also want to fix various places to take advantage of the index. Seems like it could be a big win when you have thousands of constraints (albeit only in the add/drop case). If I find the time maybe Ill submit something along these lines for the next commit fest. Thanks! -- 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] alter table only ... drop constraint broken in HEAD
On Wed, Oct 5, 2011 at 10:29 PM, Robert Haas robertmh...@gmail.com wrote: On Wed, Oct 5, 2011 at 5:53 PM, Alex Hunsaker bada...@gmail.com wrote: tldr: Seems to be broken by http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=4da99ea4231e3d8bbf28b666748c1028e7b7d665 : commit 4da99ea4231e3d8bbf28b666748c1028e7b7d665 Author: Robert Haas rh...@postgresql.org Date: Mon Jun 27 10:27:17 2011 -0400 Avoid having two copies of the HOT-chain search logic. Hmm... that's pretty terrible. Yikes. That commit wasn't intended to change any behavior, just to clean up the code, so I think the right thing to do here is figure out how I changed the behavior without meaning too, rather than to start patching up all the places that might have been affected by whatever the behavior change was. I'm too tired to figure this out right now, but I'll spend some time staring at it tomorrow. Oh, I see the problem, and I now agree that it's the DROP CONSTRAINT code that is buggy. Here's what happened. In the old code, when index_getnext() is walking a HOT chain, before returning each tuple, it remembers the XMAX of that tuple and the offset of the next tuple in the chain. So in the following scenario, we can fail to walk the entire HOT chain: 1. index_getnext() returns what is currently the last tuple in the chain, remembering the xmax and next tid 2. the caller, or some other process, performs a HOT update of that tuple 3. now index_getnext() is called again, but it uses the remembered xmax and tid, which are now out-of-date The new code, on the other hand, doesn't remember the xmax and tid; instead, it waits until the next call to index_getnext(), and then fetches them from the previous-returned tuple at that time. So it always sees the most current values and, therefore, walks the entire chain. In this particular case, the DROP CONSTRAINT code is counting on the fact that it won't see its own updates, even though it is bumping the command counter after each one, which is clearly wrong. I don't think this was really safe even under the old coding, because there's no guarantee that any particular update will be HOT, so we might have found our own update anyway; it was just less likely. I took a look around for other places that might have this same problem and didn't find any, but I'm not too confident that that means there are none there, since there are a fair number of things that can call CommandCounterIncrement() indirectly. shdepReassignOwned() does a direct CCI from within a scan, but ISTM that any update we do there would produce a new tuple that doesn't match the scan, so that should be OK. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] alter table only ... drop constraint broken in HEAD
On Thu, Oct 6, 2011 at 07:24, Robert Haas robertmh...@gmail.com wrote: On Wed, Oct 5, 2011 at 10:29 PM, Robert Haas robertmh...@gmail.com wrote: On Wed, Oct 5, 2011 at 5:53 PM, Alex Hunsaker bada...@gmail.com wrote: tldr: Seems to be broken by http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=4da99ea4231e3d8bbf28b666748c1028e7b7d665 : commit 4da99ea4231e3d8bbf28b666748c1028e7b7d665 Author: Robert Haas rh...@postgresql.org Date: Mon Jun 27 10:27:17 2011 -0400 Avoid having two copies of the HOT-chain search logic. Oh, I see the problem, and I now agree that it's the DROP CONSTRAINT code that is buggy. Want me to roll this fix in as part of the alter table only constraint patch? Or keep it split out? We might want to backpatch to at least 8.3 where HOT was introduced (yes looks like the bug existed back then). I suppose its a fairly narrow chance to hit this bug so I could see the argument for not back patching... I took a look around for other places that might have this same problem and didn't find any, but I'm not too confident that that means there are none there, since there are a fair number of things that can call CommandCounterIncrement() indirectly. I skimmed for the direct easy to find ones as well. Didn't see any other than the one you note below. shdepReassignOwned() does a direct CCI from within a scan, but ISTM that any update we do there would produce a new tuple that doesn't match the scan, so that should be OK. Well its on purpose so I hope its ok :-) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] alter table only ... drop constraint broken in HEAD
tldr:
Seems to be broken by
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=4da99ea4231e3d8bbf28b666748c1028e7b7d665
:
commit 4da99ea4231e3d8bbf28b666748c1028e7b7d665
Author: Robert Haas rh...@postgresql.org
Date: Mon Jun 27 10:27:17 2011 -0400
Avoid having two copies of the HOT-chain search logic.
Full story:
While playing with the non inheritable constraints patch
(https://commitfest.postgresql.org/action/patch_view?id=611) I noticed
the following sequence was broken:
create table colors (c text check (not null));
create table colors_i () inherits (colors);
alter table only colors drop constraint colors_check;
ERROR: relation 16412 has non-inherited constraint colors_check
Naturally I assumed it was the patches fault, but after further
digging turns out it was not. I bisected it down to the above commit
(for those that have not tried git bisect and git bisect run its great
for this kind of thing).
The basic problem seems to be after we update pg_constraint to
decrement inhcount for a child table we call
CommandCounterIncrement(); then we fetch the next row from
pg_constraint... which happens to be the row we just updated. So we
try to decrement it again, only now its at 0 which shouldn't happen so
we error out.
The simple fix seemed to be to move the CommandCounterIncrement()
outside of the while(... systable_getnext()) loop. Im not sure if
that's the right thing to-do or if there is some other bug here...
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***
*** 6734,6739 ATExecDropConstraint(Relation rel, const char *constrName,
--- 6734,6740
{
Oid childrelid = lfirst_oid(child);
Relation childrel;
+ bool updated = false;
/* find_inheritance_children already got lock */
childrel = heap_open(childrelid, NoLock);
***
*** 6790,6798 ATExecDropConstraint(Relation rel, const char *constrName,
con-coninhcount--;
simple_heap_update(conrel, copy_tuple-t_self, copy_tuple);
CatalogUpdateIndexes(conrel, copy_tuple);
!
! /* Make update visible */
! CommandCounterIncrement();
}
}
else
--- 6791,6798
con-coninhcount--;
simple_heap_update(conrel, copy_tuple-t_self, copy_tuple);
CatalogUpdateIndexes(conrel, copy_tuple);
! /* need to call CommandCounterIncrement() */
! updated = true;
}
}
else
***
*** 6807,6815 ATExecDropConstraint(Relation rel, const char *constrName,
simple_heap_update(conrel, copy_tuple-t_self, copy_tuple);
CatalogUpdateIndexes(conrel, copy_tuple);
!
! /* Make update visible */
! CommandCounterIncrement();
}
heap_freetuple(copy_tuple);
--- 6807,6814
simple_heap_update(conrel, copy_tuple-t_self, copy_tuple);
CatalogUpdateIndexes(conrel, copy_tuple);
! /* need to call CommandCounterIncrement() */
! updated = true;
}
heap_freetuple(copy_tuple);
***
*** 6824,6829 ATExecDropConstraint(Relation rel, const char *constrName,
--- 6823,6832
constrName,
RelationGetRelationName(childrel;
+ /* Make update visible */
+ if (updated)
+ CommandCounterIncrement();
+
heap_close(childrel, NoLock);
}
*** a/src/test/regress/expected/alter_table.out
--- b/src/test/regress/expected/alter_table.out
***
*** 2103,2105 ALTER TABLE tt7 NOT OF;
--- 2103,2112
x | integer |
y | numeric(8,2) |
+ -- make sure we can drop a constraint on the parent but it remains on the child
+ create table test_drop_constr_parent (c text check (c is not null));
+ create table test_drop_constr_child () inherits (test_drop_constr_parent);
+ alter table only test_drop_constr_parent drop constraint test_drop_constr_parent_c_check;
+ -- should fail
+ insert into test_drop_constr_child (c) values (NULL);
+ ERROR: new row for relation test_drop_constr_child violates check constraint test_drop_constr_parent_c_check
*** a/src/test/regress/sql/alter_table.sql
--- b/src/test/regress/sql/alter_table.sql
***
*** 1456,1458 CREATE TYPE tt_t1 AS (x int, y numeric(8,2));
--- 1456,1465
ALTER TABLE tt7 OF tt_t1; -- reassign an already-typed table
ALTER TABLE tt7 NOT OF;
\d tt7
+
+ -- make sure we can drop a constraint on the parent but it remains on the child
+ create table test_drop_constr_parent (c text check (c is not null));
+ create table test_drop_constr_child () inherits (test_drop_constr_parent);
+ alter table only test_drop_constr_parent drop constraint test_drop_constr_parent_c_check;
+ -- should fail
+ insert into test_drop_constr_child (c) values (NULL);
--
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] alter table only ... drop constraint broken in HEAD
On Wed, Oct 5, 2011 at 5:53 PM, Alex Hunsaker bada...@gmail.com wrote: tldr: Seems to be broken by http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=4da99ea4231e3d8bbf28b666748c1028e7b7d665 : commit 4da99ea4231e3d8bbf28b666748c1028e7b7d665 Author: Robert Haas rh...@postgresql.org Date: Mon Jun 27 10:27:17 2011 -0400 Avoid having two copies of the HOT-chain search logic. Hmm... that's pretty terrible. Yikes. That commit wasn't intended to change any behavior, just to clean up the code, so I think the right thing to do here is figure out how I changed the behavior without meaning too, rather than to start patching up all the places that might have been affected by whatever the behavior change was. I'm too tired to figure this out right now, but I'll spend some time staring at it tomorrow. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
