at-index-tighten-v2.patch

text/plain

Filename: at-index-tighten-v2.patch
Type: text/plain
Part: 0
Message: Re: Second thoughts on CheckIndexCompatible() vs. operator families

Patch

Same data as JSON: GET /api/v1/attachments/:id/patch the parsed metadata as JSON — format, series position, per-file stats; never the diff bytes. API reference →
Format: unified
Series: patch v2
File+
src/backend/commands/indexcmds.c 0 0
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 712b0b0..1bf1de5 100644
*** a/src/backend/commands/indexcmds.c
--- b/src/backend/commands/indexcmds.c
***************
*** 23,28 ****
--- 23,29 ----
  #include "catalog/pg_opclass.h"
  #include "catalog/pg_opfamily.h"
  #include "catalog/pg_tablespace.h"
+ #include "catalog/pg_type.h"
  #include "commands/dbcommands.h"
  #include "commands/defrem.h"
  #include "commands/tablecmds.h"
***************
*** 52,57 ****
--- 53,59 ----
  /* non-export function prototypes */
  static void CheckPredicate(Expr *predicate);
  static void ComputeIndexAttrs(IndexInfo *indexInfo,
+ 				  Oid *typeOidP,
  				  Oid *collationOidP,
  				  Oid *classOidP,
  				  int16 *colOptionP,
***************
*** 87,104 **** static void RangeVarCallbackForReindexIndex(const RangeVar *relation,
   * of columns and that if one has an expression column or predicate, both do.
   * Errors arising from the attribute list still apply.
   *
!  * Most column type changes that can skip a table rewrite will not invalidate
!  * indexes.  For btree and hash indexes, we assume continued validity when
!  * each column of an index would have the same operator family before and
!  * after the change.  Since we do not document a contract for GIN or GiST
!  * operator families, we require an exact operator class match for them and
!  * for any other access methods.
!  *
!  * DefineIndex always verifies that each exclusion operator shares an operator
!  * family with its corresponding index operator class.  For access methods
!  * having no operator family contract, confirm that the old and new indexes
!  * use the exact same exclusion operator.  For btree and hash, there's nothing
!  * more to check.
   *
   * We do not yet implement a test to verify compatibility of expression
   * columns or predicates, so assume any such index is incompatible.
--- 89,105 ----
   * of columns and that if one has an expression column or predicate, both do.
   * Errors arising from the attribute list still apply.
   *
!  * Most column type changes that can skip a table rewrite do not invalidate
!  * indexes.  We ackowledge this when all operator classes, collations and
!  * exclusion operators match.  Though we could further permit intra-opfamily
!  * changes for btree and hash indexes, that adds subtle complexity with no
!  * concrete benefit for core types.
! 
!  * When a comparison or exclusion operator has a polymorphic input type, the
!  * actual input types must also match.  This defends against the possibility
!  * that operators could vary behavior in response to get_fn_expr_argtype().
!  * At present, this hazard is theoretical: check_exclusion_constraint() and
!  * all core index access methods decline to set fn_expr for such calls.
   *
   * We do not yet implement a test to verify compatibility of expression
   * columns or predicates, so assume any such index is incompatible.
***************
*** 111,116 **** CheckIndexCompatible(Oid oldId,
--- 112,118 ----
  					 List *exclusionOpNames)
  {
  	bool		isconstraint;
+ 	Oid		   *typeObjectId;
  	Oid		   *collationObjectId;
  	Oid		   *classObjectId;
  	Oid			accessMethodId;
***************
*** 123,132 **** CheckIndexCompatible(Oid oldId,
  	int			numberOfAttributes;
  	int			old_natts;
  	bool		isnull;
- 	bool		family_am;
  	bool		ret = true;
  	oidvector  *old_indclass;
  	oidvector  *old_indcollation;
  	int			i;
  	Datum		d;
  
--- 125,134 ----
  	int			numberOfAttributes;
  	int			old_natts;
  	bool		isnull;
  	bool		ret = true;
  	oidvector  *old_indclass;
  	oidvector  *old_indcollation;
+ 	Relation	irel;
  	int			i;
  	Datum		d;
  
***************
*** 168,177 **** CheckIndexCompatible(Oid oldId,
  	indexInfo->ii_ExclusionOps = NULL;
  	indexInfo->ii_ExclusionProcs = NULL;
  	indexInfo->ii_ExclusionStrats = NULL;
  	collationObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
  	classObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
  	coloptions = (int16 *) palloc(numberOfAttributes * sizeof(int16));
! 	ComputeIndexAttrs(indexInfo, collationObjectId, classObjectId,
  					  coloptions, attributeList,
  					  exclusionOpNames, relationId,
  					  accessMethodName, accessMethodId,
--- 170,181 ----
  	indexInfo->ii_ExclusionOps = NULL;
  	indexInfo->ii_ExclusionProcs = NULL;
  	indexInfo->ii_ExclusionStrats = NULL;
+ 	typeObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
  	collationObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
  	classObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
  	coloptions = (int16 *) palloc(numberOfAttributes * sizeof(int16));
! 	ComputeIndexAttrs(indexInfo,
! 					  typeObjectId, collationObjectId, classObjectId,
  					  coloptions, attributeList,
  					  exclusionOpNames, relationId,
  					  accessMethodName, accessMethodId,
***************
*** 191,202 **** CheckIndexCompatible(Oid oldId,
  		return false;
  	}
  
! 	/*
! 	 * If the old and new operator class of any index column differ in
! 	 * operator family or collation, regard the old index as incompatible.
! 	 * For access methods other than btree and hash, a family match has no
! 	 * defined meaning; require an exact operator class match.
! 	 */
  	old_natts = ((Form_pg_index) GETSTRUCT(tuple))->indnatts;
  	Assert(old_natts == numberOfAttributes);
  
--- 195,201 ----
  		return false;
  	}
  
! 	/* Any change in operator class or collation breaks compatibility. */
  	old_natts = ((Form_pg_index) GETSTRUCT(tuple))->indnatts;
  	Assert(old_natts == numberOfAttributes);
  
***************
*** 208,259 **** CheckIndexCompatible(Oid oldId,
  	Assert(!isnull);
  	old_indclass = (oidvector *) DatumGetPointer(d);
  
! 	family_am = accessMethodId == BTREE_AM_OID || accessMethodId == HASH_AM_OID;
! 
! 	for (i = 0; i < old_natts; i++)
! 	{
! 		Oid			old_class = old_indclass->values[i];
! 		Oid			new_class = classObjectId[i];
! 
! 		if (!(old_indcollation->values[i] == collationObjectId[i]
! 			  && (old_class == new_class
! 				  || (family_am && (get_opclass_family(old_class)
! 									== get_opclass_family(new_class))))))
! 		{
! 			ret = false;
! 			break;
! 		}
! 	}
  
  	ReleaseSysCache(tuple);
  
! 	/*
! 	 * For btree and hash, exclusion operators need only fall in the same
! 	 * operator family; ComputeIndexAttrs already verified that much.  If we
! 	 * get this far, we know that the index operator family has not changed,
! 	 * and we're done.  For other access methods, require exact matches for
! 	 * all exclusion operators.
! 	 */
! 	if (ret && !family_am && indexInfo->ii_ExclusionOps != NULL)
  	{
- 		Relation	irel;
  		Oid		   *old_operators, *old_procs;
  		uint16	   *old_strats;
  
- 		/* Caller probably already holds a stronger lock. */
- 		irel = index_open(oldId, AccessShareLock);
  		RelationGetExclusionInfo(irel, &old_operators, &old_procs, &old_strats);
  
! 		for (i = 0; i < old_natts; i++)
! 			if (old_operators[i] != indexInfo->ii_ExclusionOps[i])
! 			{
! 				ret = false;
! 				break;
! 			}
  
! 		index_close(irel, NoLock);
  	}
  
  	return ret;
  }
  
--- 207,248 ----
  	Assert(!isnull);
  	old_indclass = (oidvector *) DatumGetPointer(d);
  
! 	ret = (memcmp(old_indclass->values, classObjectId,
! 				  old_natts * sizeof(Oid)) == 0 &&
! 		   memcmp(old_indcollation->values, collationObjectId,
! 				  old_natts * sizeof(Oid)) == 0);
  
  	ReleaseSysCache(tuple);
  
! 	/* For polymorphic opcintype, column type changes break compatibility. */
! 	irel = index_open(oldId, AccessShareLock); /* caller probably has a lock */
! 	for (i = 0; i < old_natts && ret; i++)
! 		ret = (!IsPolymorphicType(get_opclass_input_type(classObjectId[i])) ||
! 			   irel->rd_att->attrs[i]->atttypid == typeObjectId[i]);
! 
! 	/* Any change in exclusion operator selections breaks compatibility. */
! 	if (ret && indexInfo->ii_ExclusionOps != NULL)
  	{
  		Oid		   *old_operators, *old_procs;
  		uint16	   *old_strats;
  
  		RelationGetExclusionInfo(irel, &old_operators, &old_procs, &old_strats);
+ 		ret = memcmp(old_operators, indexInfo->ii_ExclusionOps,
+ 					 old_natts * sizeof(Oid)) == 0;
  
! 		/* Require an exact input type match for polymorphic operators. */
! 		for (i = 0; i < old_natts && ret; i++)
! 		{
! 			Oid			left,
! 						right;
  
! 			op_input_types(indexInfo->ii_ExclusionOps[i], &left, &right);
! 			ret = (!(IsPolymorphicType(left) || IsPolymorphicType(right)) ||
! 				   irel->rd_att->attrs[i]->atttypid == typeObjectId[i]);
! 		}
  	}
  
+ 	index_close(irel, NoLock);
  	return ret;
  }
  
***************
*** 315,320 **** DefineIndex(RangeVar *heapRelation,
--- 304,310 ----
  			bool quiet,
  			bool concurrent)
  {
+ 	Oid		   *typeObjectId;
  	Oid		   *collationObjectId;
  	Oid		   *classObjectId;
  	Oid			accessMethodId;
***************
*** 550,559 **** DefineIndex(RangeVar *heapRelation,
  	indexInfo->ii_Concurrent = concurrent;
  	indexInfo->ii_BrokenHotChain = false;
  
  	collationObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
  	classObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
  	coloptions = (int16 *) palloc(numberOfAttributes * sizeof(int16));
! 	ComputeIndexAttrs(indexInfo, collationObjectId, classObjectId,
  					  coloptions, attributeList,
  					  exclusionOpNames, relationId,
  					  accessMethodName, accessMethodId,
--- 540,551 ----
  	indexInfo->ii_Concurrent = concurrent;
  	indexInfo->ii_BrokenHotChain = false;
  
+ 	typeObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
  	collationObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
  	classObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
  	coloptions = (int16 *) palloc(numberOfAttributes * sizeof(int16));
! 	ComputeIndexAttrs(indexInfo,
! 					  typeObjectId, collationObjectId, classObjectId,
  					  coloptions, attributeList,
  					  exclusionOpNames, relationId,
  					  accessMethodName, accessMethodId,
***************
*** 980,985 **** CheckPredicate(Expr *predicate)
--- 972,978 ----
   */
  static void
  ComputeIndexAttrs(IndexInfo *indexInfo,
+ 				  Oid *typeOidP,
  				  Oid *collationOidP,
  				  Oid *classOidP,
  				  int16 *colOptionP,
***************
*** 1108,1113 **** ComputeIndexAttrs(IndexInfo *indexInfo,
--- 1101,1108 ----
  			}
  		}
  
+ 		typeOidP[attn] = atttype;
+ 
  		/*
  		 * Apply collation override if any
  		 */
diff --git a/src/test/regress/expected/index 57096f2..58762a3 100644
*** a/src/test/regress/expected/alter_table.out
--- b/src/test/regress/expected/alter_table.out
***************
*** 1656,1661 **** where oid = 'test_storage'::regclass;
--- 1656,1670 ----
   t
  (1 row)
  
+ -- SET DATA TYPE without a rewrite
+ CREATE DOMAIN other_textarr AS text[];
+ CREATE TABLE norewrite_array(c text[] PRIMARY KEY);
+ NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "norewrite_array_pkey" for table "norewrite_array"
+ SET client_min_messages = debug1;
+ ALTER TABLE norewrite_array ALTER c TYPE text[]; -- no work
+ ALTER TABLE norewrite_array ALTER c TYPE other_textarr; -- rebuild index
+ DEBUG:  building index "norewrite_array_pkey" on table "norewrite_array"
+ RESET client_min_messages;
  --
  -- lock levels
  --
diff --git a/src/test/regress/sql/alter_table.sqindex faafb22..585cf95 100644
*** a/src/test/regress/sql/alter_table.sql
--- b/src/test/regress/sql/alter_table.sql
***************
*** 1190,1195 **** select reltoastrelid <> 0 as has_toast_table
--- 1190,1203 ----
  from pg_class
  where oid = 'test_storage'::regclass;
  
+ -- SET DATA TYPE without a rewrite
+ CREATE DOMAIN other_textarr AS text[];
+ CREATE TABLE norewrite_array(c text[] PRIMARY KEY);
+ SET client_min_messages = debug1;
+ ALTER TABLE norewrite_array ALTER c TYPE text[]; -- no work
+ ALTER TABLE norewrite_array ALTER c TYPE other_textarr; -- rebuild index
+ RESET client_min_messages;
+ 
  --
  -- lock levels
  --