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

Reply via email to