at-foreign-key-v4.patch

text/plain

Filename: at-foreign-key-v4.patch
Type: text/plain
Part: 0
Message: Re: Avoid FK validations for no-rewrite ALTER TABLE ALTER TYPE

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 v4
File+
src/backend/commands/tablecmds.c 0 0
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index cb8ac67..ba20950 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 276,281 **** static Oid transformFkeyCheckAttrs(Relation pkrel,
--- 276,282 ----
  						int numattrs, int16 *attnums,
  						Oid *opclasses);
  static void checkFkeyPermissions(Relation rel, int16 *attnums, int natts);
+ static CoercionPathType findFkeyCast(Oid targetTypeId, Oid sourceTypeId, Oid *funcid);
  static void validateCheckConstraint(Relation rel, HeapTuple constrtup);
  static void validateForeignKeyConstraint(char *conname,
  							 Relation rel, Relation pkrel,
***************
*** 357,362 **** static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMOD
--- 358,364 ----
  static void ATPostAlterTypeParse(Oid oldId, char *cmd,
  					 List **wqueue, LOCKMODE lockmode, bool rewrite);
  static void TryReuseIndex(Oid oldId, IndexStmt *stmt);
+ static void TryReuseForeignKey(Oid oldId, Constraint *con);
  static void change_owner_fix_column_acls(Oid relationOid,
  							 Oid oldOwnerId, Oid newOwnerId);
  static void change_owner_recurse_to_sequences(Oid relationOid,
***************
*** 5591,5596 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
--- 5593,5600 ----
  				numpks;
  	Oid			indexOid;
  	Oid			constrOid;
+ 	bool		old_check_ok;
+ 	ListCell   *old_pfeqop_item = list_head(fkconstraint->old_conpfeqop);
  
  	/*
  	 * Grab an exclusive lock on the pk table, so that someone doesn't delete
***************
*** 5707,5712 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
--- 5711,5723 ----
  				(errcode(ERRCODE_INVALID_FOREIGN_KEY),
  				 errmsg("number of referencing and referenced columns for foreign key disagree")));
  
+ 	/*
+ 	 * On the strength of a previous constraint, we might avoid scanning
+ 	 * tables to validate this one.  See below.
+ 	 */
+ 	old_check_ok = (fkconstraint->old_conpfeqop != NIL);
+ 	Assert(!old_check_ok || numfks == list_length(fkconstraint->old_conpfeqop));
+ 
  	for (i = 0; i < numpks; i++)
  	{
  		Oid			pktype = pktypoid[i];
***************
*** 5721,5726 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
--- 5732,5738 ----
  		Oid			ppeqop;
  		Oid			ffeqop;
  		int16		eqstrategy;
+ 		Oid			pfeqop_right;
  
  		/* We need several fields out of the pg_opclass entry */
  		cla_ht = SearchSysCache1(CLAOID, ObjectIdGetDatum(opclasses[i]));
***************
*** 5763,5772 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
  		pfeqop = get_opfamily_member(opfamily, opcintype, fktyped,
  									 eqstrategy);
  		if (OidIsValid(pfeqop))
  			ffeqop = get_opfamily_member(opfamily, fktyped, fktyped,
  										 eqstrategy);
  		else
! 			ffeqop = InvalidOid;	/* keep compiler quiet */
  
  		if (!(OidIsValid(pfeqop) && OidIsValid(ffeqop)))
  		{
--- 5775,5791 ----
  		pfeqop = get_opfamily_member(opfamily, opcintype, fktyped,
  									 eqstrategy);
  		if (OidIsValid(pfeqop))
+ 		{
+ 			pfeqop_right = fktyped;
  			ffeqop = get_opfamily_member(opfamily, fktyped, fktyped,
  										 eqstrategy);
+ 		}
  		else
! 		{
! 			/* keep compiler quiet */
! 			pfeqop_right = InvalidOid;
! 			ffeqop = InvalidOid;
! 		}
  
  		if (!(OidIsValid(pfeqop) && OidIsValid(ffeqop)))
  		{
***************
*** 5788,5794 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
--- 5807,5816 ----
  			target_typeids[1] = opcintype;
  			if (can_coerce_type(2, input_typeids, target_typeids,
  								COERCION_IMPLICIT))
+ 			{
  				pfeqop = ffeqop = ppeqop;
+ 				pfeqop_right = opcintype;
+ 			}
  		}
  
  		if (!(OidIsValid(pfeqop) && OidIsValid(ffeqop)))
***************
*** 5804,5809 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
--- 5826,5902 ----
  							   format_type_be(fktype),
  							   format_type_be(pktype))));
  
+ 		if (old_check_ok)
+ 		{
+ 			/*
+ 			 * When a pfeqop changes, revalidate the constraint.  We could
+ 			 * permit intra-opfamily changes, but that adds subtle complexity
+ 			 * without any concrete benefit for core types.  We need not
+ 			 * assess ppeqop or ffeqop, which RI_Initial_Check() does not use.
+ 			 */
+ 			old_check_ok = (pfeqop == lfirst_oid(old_pfeqop_item));
+ 			old_pfeqop_item = lnext(old_pfeqop_item);
+ 		}
+ 		if (old_check_ok)
+ 		{
+ 			Oid			old_fktype;
+ 			Oid			new_fktype;
+ 			CoercionPathType old_pathtype;
+ 			CoercionPathType new_pathtype;
+ 			Oid			old_castfunc;
+ 			Oid			new_castfunc;
+ 
+ 			/*
+ 			 * Identify coercion pathways from each of the old and new FK-side
+ 			 * column types to the right (foreign) operand type of the pfeqop.
+ 			 * We may assume that pg_constraint.conkey is not changing.
+ 			 */
+ 			old_fktype = tab->oldDesc->attrs[fkattnum[i] - 1]->atttypid;
+ 			new_fktype = fktype;
+ 			old_pathtype = findFkeyCast(pfeqop_right, old_fktype,
+ 										&old_castfunc);
+ 			new_pathtype = findFkeyCast(pfeqop_right, new_fktype,
+ 										&new_castfunc);
+ 
+ 			/*
+ 			 * Upon a change to the cast from the FK column to its pfeqop
+ 			 * operand, revalidate the constraint.  For this evaluation, a
+ 			 * binary coercion cast is equivalent to no cast at all.  While
+ 			 * type implementors should design implicit casts with an eye
+ 			 * toward consistency of operations like equality, we cannot
+ 			 * assume here that they have done so.
+ 			 *
+ 			 * A function with a polymorphic argument could change behavior
+ 			 * arbitrarily in response to get_fn_expr_argtype().  Therefore,
+ 			 * when the cast destination is polymorphic, we only avoid
+ 			 * revalidation if the input type has not changed at all.  Given
+ 			 * just the core data types and operator classes, this requirement
+ 			 * prevents no would-be optimizations.
+ 			 *
+ 			 * If the cast converts from a base type to a domain thereon, then
+ 			 * that domain type must be the opcintype of the unique index.
+ 			 * Necessarily, the primary key column must then be of the domain
+ 			 * type.  Since the constraint was previously valid, all values on
+ 			 * the foreign side necessarily exist on the primary side and in
+ 			 * turn conform to the domain.  Consequently, we need not treat
+ 			 * domains specially here.
+ 			 *
+ 			 * Since we require that all collations share the same notion of
+ 			 * equality (see comment at ri_GenerateQualCollation()), don't
+ 			 * compare collation here.
+ 			 *
+ 			 * We need not directly consider the PK type.  It's necessarily
+ 			 * binary coercible to the opcintype of the unique index column,
+ 			 * and ri_triggers.c will only deal with PK datums in terms of
+ 			 * that opcintype.  Changing the opcintype also changes pfeqop.
+ 			 */
+ 			old_check_ok = (new_pathtype == old_pathtype &&
+ 							new_castfunc == old_castfunc &&
+ 							(!IsPolymorphicType(pfeqop_right) ||
+ 							 new_fktype == old_fktype));
+ 
+ 		}
+ 
  		pfeqoperators[i] = pfeqop;
  		ppeqoperators[i] = ppeqop;
  		ffeqoperators[i] = ffeqop;
***************
*** 5847,5857 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
  	createForeignKeyTriggers(rel, fkconstraint, constrOid, indexOid);
  
  	/*
! 	 * Tell Phase 3 to check that the constraint is satisfied by existing rows.
! 	 * We can skip this during table creation, or if requested explicitly by
! 	 * specifying NOT VALID in an ADD FOREIGN KEY command.
  	 */
! 	if (!fkconstraint->skip_validation)
  	{
  		NewConstraint *newcon;
  
--- 5940,5952 ----
  	createForeignKeyTriggers(rel, fkconstraint, constrOid, indexOid);
  
  	/*
! 	 * Tell Phase 3 to check that the constraint is satisfied by existing
! 	 * rows.  We can skip this during table creation, when requested
! 	 * explicitly by specifying NOT VALID in an ADD FOREIGN KEY command, and
! 	 * when we're recreating a constraint following a SET DATA TYPE operation
! 	 * that did not impugn its validity.
  	 */
! 	if (!old_check_ok && !fkconstraint->skip_validation)
  	{
  		NewConstraint *newcon;
  
***************
*** 6301,6306 **** transformFkeyCheckAttrs(Relation pkrel,
--- 6396,6430 ----
  	return indexoid;
  }
  
+ /*
+  * findFkeyCast -
+  *
+  *	Wrapper around find_coercion_pathway() for ATAddForeignKeyConstraint().
+  *	Caller has equal regard for binary coercibility and for an exact match.
+ */
+ static CoercionPathType
+ findFkeyCast(Oid targetTypeId, Oid sourceTypeId, Oid *funcid)
+ {
+ 	CoercionPathType ret;
+ 
+ 	if (targetTypeId == sourceTypeId)
+ 	{
+ 		ret = COERCION_PATH_RELABELTYPE;
+ 		*funcid = InvalidOid;
+ 	}
+ 	else
+ 	{
+ 		ret = find_coercion_pathway(targetTypeId, sourceTypeId,
+ 									COERCION_IMPLICIT, funcid);
+ 		if (ret == COERCION_PATH_NONE)
+ 			/* A previously-relied-upon cast is now gone. */
+ 			elog(ERROR, "could not find cast from %u to %u",
+ 				 sourceTypeId, targetTypeId);
+ 	}
+ 
+ 	return ret;
+ }
+ 
  /* Permissions checks for ADD FOREIGN KEY */
  static void
  checkFkeyPermissions(Relation rel, int16 *attnums, int natts)
***************
*** 7687,7692 **** ATPostAlterTypeParse(Oid oldId, char *cmd,
--- 7811,7817 ----
  					foreach(lcmd, stmt->cmds)
  					{
  						AlterTableCmd *cmd = (AlterTableCmd *) lfirst(lcmd);
+ 						Constraint *con;
  
  						switch (cmd->subtype)
  						{
***************
*** 7700,7705 **** ATPostAlterTypeParse(Oid oldId, char *cmd,
--- 7825,7836 ----
  									lappend(tab->subcmds[AT_PASS_OLD_INDEX], cmd);
  								break;
  							case AT_AddConstraint:
+ 								Assert(IsA(cmd->def, Constraint));
+ 								con = (Constraint *) cmd->def;
+ 								/* rewriting neither side of a FK */
+ 								if (con->contype == CONSTR_FOREIGN &&
+ 									!rewrite && !tab->rewrite)
+ 									TryReuseForeignKey(oldId, con);
  								tab->subcmds[AT_PASS_OLD_CONSTR] =
  									lappend(tab->subcmds[AT_PASS_OLD_CONSTR], cmd);
  								break;
***************
*** 7738,7743 **** TryReuseIndex(Oid oldId, IndexStmt *stmt)
--- 7869,7917 ----
  	}
  }
  
+ /*
+  * Subroutine for ATPostAlterTypeParse().  ATAddForeignKeyConstraint() will
+  * make the final ruling on whether to skip revalidating this constraint's
+  * successor.  Here, we just stash the old conpfeqop for its use.
+ */
+ static void
+ TryReuseForeignKey(Oid oldId, Constraint *con)
+ {
+ 	HeapTuple	tup;
+ 	Datum		adatum;
+ 	bool		isNull;
+ 	ArrayType  *arr;
+ 	Oid		   *rawarr;
+ 	int			numkeys;
+ 	int			i;
+ 
+ 	Assert(con->contype == CONSTR_FOREIGN);
+ 	Assert(con->old_conpfeqop == NIL); /* already prepared this node */
+ 
+ 	tup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(oldId));
+ 	if (!HeapTupleIsValid(tup)) /* should not happen */
+ 		elog(ERROR, "cache lookup failed for constraint %u", oldId);
+ 
+ 	adatum = SysCacheGetAttr(CONSTROID, tup,
+ 							 Anum_pg_constraint_conpfeqop, &isNull);
+ 	if (isNull)
+ 		elog(ERROR, "null conpfeqop for constraint %u", oldId);
+ 	arr = DatumGetArrayTypeP(adatum);	/* ensure not toasted */
+ 	numkeys = ARR_DIMS(arr)[0];
+ 	/* test follows the one in ri_FetchConstraintInfo() */
+ 	if (ARR_NDIM(arr) != 1 ||
+ 		ARR_HASNULL(arr) ||
+ 		ARR_ELEMTYPE(arr) != OIDOID)
+ 		elog(ERROR, "conpfeqop is not a 1-D Oid array");
+ 	rawarr = (Oid *) ARR_DATA_PTR(arr);
+ 
+ 	/* stash a List of the operator Oids in our Constraint node */
+ 	for (i = 0; i < numkeys; i++)
+ 		con->old_conpfeqop = lcons_oid(rawarr[i], con->old_conpfeqop);
+ 
+ 	ReleaseSysCache(tup);
+ }
+ 
  
  /*
   * ALTER TABLE OWNER
diff --git a/src/backend/nodes/copyfuncindex cc3168d..7fec4db 100644
*** a/src/backend/nodes/copyfuncs.c
--- b/src/backend/nodes/copyfuncs.c
***************
*** 2364,2369 **** _copyConstraint(const Constraint *from)
--- 2364,2370 ----
  	COPY_SCALAR_FIELD(fk_matchtype);
  	COPY_SCALAR_FIELD(fk_upd_action);
  	COPY_SCALAR_FIELD(fk_del_action);
+ 	COPY_NODE_FIELD(old_conpfeqop);
  	COPY_SCALAR_FIELD(skip_validation);
  	COPY_SCALAR_FIELD(initially_valid);
  
diff --git a/src/backend/nodes/equalindex 2295195..d2a79eb 100644
*** a/src/backend/nodes/equalfuncs.c
--- b/src/backend/nodes/equalfuncs.c
***************
*** 2199,2204 **** _equalConstraint(const Constraint *a, const Constraint *b)
--- 2199,2205 ----
  	COMPARE_SCALAR_FIELD(fk_matchtype);
  	COMPARE_SCALAR_FIELD(fk_upd_action);
  	COMPARE_SCALAR_FIELD(fk_del_action);
+ 	COMPARE_NODE_FIELD(old_conpfeqop);
  	COMPARE_SCALAR_FIELD(skip_validation);
  	COMPARE_SCALAR_FIELD(initially_valid);
  
diff --git a/src/backend/nodes/outfunindex 8bc1947..8971846 100644
*** a/src/backend/nodes/outfuncs.c
--- b/src/backend/nodes/outfuncs.c
***************
*** 2638,2643 **** _outConstraint(StringInfo str, const Constraint *node)
--- 2638,2644 ----
  			WRITE_CHAR_FIELD(fk_matchtype);
  			WRITE_CHAR_FIELD(fk_upd_action);
  			WRITE_CHAR_FIELD(fk_del_action);
+ 			WRITE_NODE_FIELD(old_conpfeqop);
  			WRITE_BOOL_FIELD(skip_validation);
  			WRITE_BOOL_FIELD(initially_valid);
  			break;
diff --git a/src/include/nodes/parsindex 1d33ceb..ab55639 100644
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
***************
*** 1552,1557 **** typedef struct Constraint
--- 1552,1558 ----
  	char		fk_matchtype;	/* FULL, PARTIAL, UNSPECIFIED */
  	char		fk_upd_action;	/* ON UPDATE action */
  	char		fk_del_action;	/* ON DELETE action */
+ 	List	   *old_conpfeqop;	/* pg_constraint.conpfeqop of my former self */
  
  	/* Fields used for constraints that allow a NOT VALID specification */
  	bool		skip_validation;	/* skip validation of existing rows? */
diff --git a/src/test/regress/expecteindex 41bfa85..3387516 100644
*** a/src/test/regress/expected/alter_table.out
--- b/src/test/regress/expected/alter_table.out
***************
*** 1674,1679 **** ALTER TABLE norewrite_array ALTER c TYPE text[]; -- no work
--- 1674,1705 ----
  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;
+ CREATE TABLE norewrite1_parent(c name PRIMARY KEY);
+ NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "norewrite1_parent_pkey" for table "norewrite1_parent"
+ CREATE TABLE norewrite1_child(c text REFERENCES norewrite1_parent);
+ SET client_min_messages = debug1;
+ ALTER TABLE norewrite1_child ALTER c TYPE varchar; -- recheck FK
+ DEBUG:  validating foreign key constraint "norewrite1_child_c_fkey"
+ RESET client_min_messages;
+ CREATE DOMAIN other_int AS int;
+ CREATE TABLE norewrite2_parent(c bigint PRIMARY KEY);
+ NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "norewrite2_parent_pkey" for table "norewrite2_parent"
+ CREATE TABLE norewrite2_child(c int REFERENCES norewrite2_parent);
+ SET client_min_messages = debug1;
+ ALTER TABLE norewrite2_child ALTER c TYPE other_int; -- no work
+ ALTER TABLE norewrite2_parent ALTER c TYPE int; -- full work
+ DEBUG:  rewriting table "norewrite2_parent"
+ DEBUG:  building index "norewrite2_parent_pkey" on table "norewrite2_parent"
+ DEBUG:  validating foreign key constraint "norewrite2_child_c_fkey"
+ ALTER TABLE norewrite2_parent ALTER c TYPE oid; -- rebuild index, recheck FK
+ DEBUG:  building index "norewrite2_parent_pkey" on table "norewrite2_parent"
+ DEBUG:  validating foreign key constraint "norewrite2_child_c_fkey"
+ ALTER TABLE norewrite2_child ALTER c TYPE oid; -- no work
+ ALTER TABLE norewrite2_parent INHERIT norewrite2_child;
+ ALTER TABLE norewrite2_child ALTER c TYPE int; -- rebuild index, recheck FK
+ DEBUG:  building index "norewrite2_parent_pkey" on table "norewrite2_parent"
+ DEBUG:  validating foreign key constraint "norewrite2_child_c_fkey"
+ RESET client_min_messages;
  --
  -- lock levels
  --
diff --git a/src/test/regress/sql/alter_table.sqindex 494c150..465ebdd 100644
*** a/src/test/regress/sql/alter_table.sql
--- b/src/test/regress/sql/alter_table.sql
***************
*** 1205,1210 **** ALTER TABLE norewrite_array ALTER c TYPE text[]; -- no work
--- 1205,1228 ----
  ALTER TABLE norewrite_array ALTER c TYPE other_textarr; -- rebuild index
  RESET client_min_messages;
  
+ CREATE TABLE norewrite1_parent(c name PRIMARY KEY);
+ CREATE TABLE norewrite1_child(c text REFERENCES norewrite1_parent);
+ SET client_min_messages = debug1;
+ ALTER TABLE norewrite1_child ALTER c TYPE varchar; -- recheck FK
+ RESET client_min_messages;
+ 
+ CREATE DOMAIN other_int AS int;
+ CREATE TABLE norewrite2_parent(c bigint PRIMARY KEY);
+ CREATE TABLE norewrite2_child(c int REFERENCES norewrite2_parent);
+ SET client_min_messages = debug1;
+ ALTER TABLE norewrite2_child ALTER c TYPE other_int; -- no work
+ ALTER TABLE norewrite2_parent ALTER c TYPE int; -- full work
+ ALTER TABLE norewrite2_parent ALTER c TYPE oid; -- rebuild index, recheck FK
+ ALTER TABLE norewrite2_child ALTER c TYPE oid; -- no work
+ ALTER TABLE norewrite2_parent INHERIT norewrite2_child;
+ ALTER TABLE norewrite2_child ALTER c TYPE int; -- rebuild index, recheck FK
+ RESET client_min_messages;
+ 
  --
  -- lock levels
  --