This patch needs more work. How carefully did you test it?

* the patch failed to copy index constraints if there was not also at
least one CHECK constraint defined on the table

* the patch is broken for expressional indexes, and silently omits
copying the predicate that may be associated with an index. It also
doesn't copy the index's amoptions (WITH clause), or the NULLS
FIRST/etc. options that may be associated with any of the index's
columns.

In other words, copying column names does not suffice to duplicate the
index constraint. Perhaps the right fix is to implement support for
INCLUDING INDEXES, and then use that code to copy the definition of any
constraint indexes in INCLUDING INDEXES?

* should we copy invalid indexes? I suppose there's nothing wrong with
copying them...

* we should probably hold the AccessShareLock on copied indexes till end
of xact, per the comments at the end of transformInhRelation()

* index_open() should be matched with index_close(), not
relation_close(). (In the current implementation, index_close() and
relation_close() happen to be identical, so it still worked.)

I've attached a revised version of the patch, but I didn't fix the
second or third bullets in the list.

-Neil

Index: doc/src/sgml/ref/create_table.sgml
===================================================================
RCS file: /home/neilc/postgres/cvs_root/pgsql/doc/src/sgml/ref/create_table.sgml,v
retrieving revision 1.107
diff -c -p -r1.107 create_table.sgml
*** doc/src/sgml/ref/create_table.sgml	1 Feb 2007 00:28:18 -0000	1.107
--- doc/src/sgml/ref/create_table.sgml	27 Apr 2007 21:49:20 -0000
*************** and <replaceable class="PARAMETER">table
*** 259,269 ****
       </para>
       <para>
        Not-null constraints are always copied to the new table.
!       <literal>CHECK</literal> constraints will only be copied if
!       <literal>INCLUDING CONSTRAINTS</literal> is specified; other types of
!       constraints will never be copied. Also, no distinction is made between
!       column constraints and table constraints &mdash; when constraints are
!       requested, all check constraints are copied.
       </para>
       <para>
        Note also that unlike <literal>INHERITS</literal>, copied columns and
--- 259,268 ----
       </para>
       <para>
        Not-null constraints are always copied to the new table.
!       <literal>CHECK, UNIQUE, and PRIMARY KEY</literal> constraints will only
!       be copied if <literal>INCLUDING CONSTRAINTS</literal> is specified. Also, 
!       no distinction is made between column constraints and table constraints
!       &mdash; when constraints are requested, all check constraints are copied.
       </para>
       <para>
        Note also that unlike <literal>INHERITS</literal>, copied columns and
Index: src/backend/parser/analyze.c
===================================================================
RCS file: /home/neilc/postgres/cvs_root/pgsql/src/backend/parser/analyze.c,v
retrieving revision 1.362
diff -c -p -r1.362 analyze.c
*** src/backend/parser/analyze.c	13 Mar 2007 00:33:41 -0000	1.362
--- src/backend/parser/analyze.c	27 Apr 2007 22:50:32 -0000
***************
*** 27,32 ****
--- 27,33 ----
  
  #include "postgres.h"
  
+ #include "access/genam.h"
  #include "access/heapam.h"
  #include "catalog/heap.h"
  #include "catalog/index.h"
***************
*** 54,59 ****
--- 55,61 ----
  #include "utils/acl.h"
  #include "utils/builtins.h"
  #include "utils/lsyscache.h"
+ #include "utils/relcache.h"
  #include "utils/syscache.h"
  
  
*************** transformInhRelation(ParseState *pstate,
*** 1325,1359 ****
  			 * If default expr could contain any vars, we'd need to fix 'em,
  			 * but it can't; so default is ready to apply to child.
  			 */
- 
  			def->cooked_default = pstrdup(this_default);
  		}
  	}
  
! 	/*
! 	 * Copy CHECK constraints if requested, being careful to adjust
! 	 * attribute numbers
! 	 */
! 	if (including_constraints && tupleDesc->constr)
  	{
! 		AttrNumber *attmap = varattnos_map_schema(tupleDesc, cxt->columns);
! 		int			ccnum;
  
! 		for (ccnum = 0; ccnum < tupleDesc->constr->num_check; ccnum++)
  		{
! 			char	   *ccname = tupleDesc->constr->check[ccnum].ccname;
! 			char	   *ccbin = tupleDesc->constr->check[ccnum].ccbin;
! 			Node	   *ccbin_node = stringToNode(ccbin);
! 			Constraint *n = makeNode(Constraint);
  
! 			change_varattnos_of_a_node(ccbin_node, attmap);
  
! 			n->contype = CONSTR_CHECK;
! 			n->name = pstrdup(ccname);
! 			n->raw_expr = NULL;
! 			n->cooked_expr = nodeToString(ccbin_node);
! 			n->indexspace = NULL;
! 			cxt->ckconstraints = lappend(cxt->ckconstraints, (Node *) n);
  		}
  	}
  
--- 1327,1424 ----
  			 * If default expr could contain any vars, we'd need to fix 'em,
  			 * but it can't; so default is ready to apply to child.
  			 */
  			def->cooked_default = pstrdup(this_default);
  		}
  	}
  
! 	if (including_constraints)
  	{
! 		/*
! 		 * Copy CHECK constraints, being careful to adjust attribute
! 		 * numbers.
! 		 */
! 		if (tupleDesc->constr)
! 		{
! 			AttrNumber *attmap;
! 			int			ccnum;
! 
! 			attmap = varattnos_map_schema(tupleDesc, cxt->columns);
! 			for (ccnum = 0; ccnum < tupleDesc->constr->num_check; ccnum++)
! 			{
! 				char	   *ccname = tupleDesc->constr->check[ccnum].ccname;
! 				char	   *ccbin = tupleDesc->constr->check[ccnum].ccbin;
! 				Node	   *ccbin_node = stringToNode(ccbin);
! 				Constraint *n = makeNode(Constraint);
! 
! 				change_varattnos_of_a_node(ccbin_node, attmap);
! 
! 				n->contype = CONSTR_CHECK;
! 				n->name = pstrdup(ccname);
! 				n->raw_expr = NULL;
! 				n->cooked_expr = nodeToString(ccbin_node);
! 				n->indexspace = NULL;
! 				cxt->ckconstraints = lappend(cxt->ckconstraints, (Node *) n);
! 			}
! 		}
  
! 		/* Copy constraint indexes, if any */
! 		if (relation->rd_rel->relhasindex)
  		{
! 			List	   *parent_index_list = RelationGetIndexList(relation);
! 			ListCell   *parent_index_scan;
! 			
! 			foreach(parent_index_scan, parent_index_list)
! 			{
! 				Oid        		 parent_index_oid = lfirst_oid(parent_index_scan);
! 				Relation   		 parent_index;
! 				AttrNumber  	 parent_attno;
! 				Constraint 		*constr;
! 
! 				parent_index = index_open(parent_index_oid, AccessShareLock);
! 
! 				/* Skip non-constraint indexes */
! 				if (!parent_index->rd_index->indisunique)
! 				{
! 					index_close(parent_index, AccessShareLock);
! 					continue;
! 				}
! 
! 				constr = makeNode(Constraint);
! 
! 				if (parent_index->rd_index->indisprimary)
! 					constr->contype = CONSTR_PRIMARY;
! 				else
! 					constr->contype = CONSTR_UNIQUE;
! 
! 				/* Let DefineIndex name it */
! 				constr->name = NULL;
! 				constr->raw_expr = NULL;
! 				constr->cooked_expr = NULL;
! 
! 				/*
! 				 * Search through the possible index keys, and
! 				 * append the names of simple columns to the new
! 				 * index key list.
! 				 */
! 				for (parent_attno = 1; parent_attno <= parent_index->rd_att->natts;
! 					 parent_attno++)
! 				{
! 					Form_pg_attribute  attr = parent_index->rd_att->attrs[parent_attno - 1];
! 					char              *attname = NameStr(attr->attname);
  
! 					/*
! 					 * Ignore dropped columns in the parent.
! 					 */
! 					if (!attr->attisdropped)
! 						constr->keys = lappend(constr->keys, makeString(attname));
! 				}
! 
! 				/* Add the new index constraint to the create context */
! 				cxt->ixconstraints = lappend(cxt->ixconstraints, constr);
  
! 				/* Keep our lock on the index till xact commit */
! 				relation_close(parent_index, NoLock);
! 			}
  		}
  	}
  
Index: src/test/regress/expected/inherit.out
===================================================================
RCS file: /home/neilc/postgres/cvs_root/pgsql/src/test/regress/expected/inherit.out,v
retrieving revision 1.20
diff -c -p -r1.20 inherit.out
*** src/test/regress/expected/inherit.out	27 Jun 2006 03:43:20 -0000	1.20
--- src/test/regress/expected/inherit.out	27 Apr 2007 21:49:20 -0000
*************** CREATE TABLE inhg (LIKE inhx); /* Doesn'
*** 621,638 ****
  INSERT INTO inhg VALUES ('foo');
  DROP TABLE inhg;
  CREATE TABLE inhg (x text, LIKE inhx INCLUDING CONSTRAINTS, y text); /* Copies constraints */
  INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds */
! INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds -- Unique constraints not copied */
  INSERT INTO inhg VALUES ('x', 'foo',  'y');  /* fails due to constraint */
  ERROR:  new row for relation "inhg" violates check constraint "foo"
  SELECT * FROM inhg; /* Two records with three columns in order x=x, xx=text, y=y */
   x |  xx  | y 
  ---+------+---
   x | text | y
!  x | text | y
! (2 rows)
  
  DROP TABLE inhg;
  -- Test changing the type of inherited columns
  insert into d values('test','one','two','three');
  alter table a alter column aa type integer using bit_length(aa);
--- 621,641 ----
  INSERT INTO inhg VALUES ('foo');
  DROP TABLE inhg;
  CREATE TABLE inhg (x text, LIKE inhx INCLUDING CONSTRAINTS, y text); /* Copies constraints */
+ NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "inhg_pkey" for table "inhg"
  INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds */
! INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Fails -- Unique constraints copied */
! ERROR:  duplicate key violates unique constraint "inhg_pkey"
  INSERT INTO inhg VALUES ('x', 'foo',  'y');  /* fails due to constraint */
  ERROR:  new row for relation "inhg" violates check constraint "foo"
  SELECT * FROM inhg; /* Two records with three columns in order x=x, xx=text, y=y */
   x |  xx  | y 
  ---+------+---
   x | text | y
! (1 row)
  
  DROP TABLE inhg;
+ CREATE TABLE inhg (x text, LIKE inhx INCLUDING INDEXES, y text); /* Unimplemented */
+ ERROR:  LIKE INCLUDING INDEXES is not implemented
  -- Test changing the type of inherited columns
  insert into d values('test','one','two','three');
  alter table a alter column aa type integer using bit_length(aa);
Index: src/test/regress/sql/inherit.sql
===================================================================
RCS file: /home/neilc/postgres/cvs_root/pgsql/src/test/regress/sql/inherit.sql,v
retrieving revision 1.10
diff -c -p -r1.10 inherit.sql
*** src/test/regress/sql/inherit.sql	27 Jun 2006 03:43:20 -0000	1.10
--- src/test/regress/sql/inherit.sql	27 Apr 2007 21:49:20 -0000
*************** INSERT INTO inhg VALUES ('foo');
*** 151,160 ****
  DROP TABLE inhg;
  CREATE TABLE inhg (x text, LIKE inhx INCLUDING CONSTRAINTS, y text); /* Copies constraints */
  INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds */
! INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds -- Unique constraints not copied */
  INSERT INTO inhg VALUES ('x', 'foo',  'y');  /* fails due to constraint */
  SELECT * FROM inhg; /* Two records with three columns in order x=x, xx=text, y=y */
  DROP TABLE inhg;
  
  
  -- Test changing the type of inherited columns
--- 151,161 ----
  DROP TABLE inhg;
  CREATE TABLE inhg (x text, LIKE inhx INCLUDING CONSTRAINTS, y text); /* Copies constraints */
  INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds */
! INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Fails -- Unique constraints copied */
  INSERT INTO inhg VALUES ('x', 'foo',  'y');  /* fails due to constraint */
  SELECT * FROM inhg; /* Two records with three columns in order x=x, xx=text, y=y */
  DROP TABLE inhg;
+ CREATE TABLE inhg (x text, LIKE inhx INCLUDING INDEXES, y text); /* Unimplemented */
  
  
  -- Test changing the type of inherited columns
---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

Reply via email to