ExecConstraints-refactor.patch

text/plain

Filename: ExecConstraints-refactor.patch
Type: text/plain
Part: 0
Message: Re: Needless additional partition check in INSERT?

Patch

Format: unified
File+
src/backend/commands/copy.c 2 20
src/backend/executor/execMain.c 13 6
src/backend/executor/execReplication.c 2 4
src/backend/executor/nodeModifyTable.c 2 23
src/include/executor/executor.h 1 2
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 0a1706c0d1..7560de3054 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2726,29 +2726,11 @@ CopyFrom(CopyState cstate)
 			else
 			{
 				/*
-				 * We always check the partition constraint, including when
-				 * the tuple got here via tuple-routing.  However we don't
-				 * need to in the latter case if no BR trigger is defined on
-				 * the partition.  Note that a BR trigger might modify the
-				 * tuple such that the partition constraint is no longer
-				 * satisfied, so we need to check in that case.
-				 */
-				bool		check_partition_constr =
-				(resultRelInfo->ri_PartitionCheck != NIL);
-
-				if (saved_resultRelInfo != NULL &&
-					!(resultRelInfo->ri_TrigDesc &&
-					  resultRelInfo->ri_TrigDesc->trig_insert_before_row))
-					check_partition_constr = false;
-
-				/*
 				 * If the target is a plain table, check the constraints of
 				 * the tuple.
 				 */
-				if (resultRelInfo->ri_FdwRoutine == NULL &&
-					(resultRelInfo->ri_RelationDesc->rd_att->constr ||
-					 check_partition_constr))
-					ExecConstraints(resultRelInfo, slot, estate, true);
+				if (resultRelInfo->ri_FdwRoutine == NULL)
+					ExecConstraints(resultRelInfo, slot, estate);
 
 				if (useHeapMultiInsert)
 				{
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 3d12f9c76f..411ee4e2af 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1952,7 +1952,7 @@ ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo,
  * ExecConstraints - check constraints of the tuple in 'slot'
  *
  * This checks the traditional NOT NULL and check constraints, and if
- * requested, checks the partition constraint.
+ * needed, checks the partition constraint.
  *
  * Note: 'slot' contains the tuple to check the constraints of, which may
  * have been converted from the original input tuple after tuple routing.
@@ -1960,8 +1960,7 @@ ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo,
  */
 void
 ExecConstraints(ResultRelInfo *resultRelInfo,
-				TupleTableSlot *slot, EState *estate,
-				bool check_partition_constraint)
+				TupleTableSlot *slot, EState *estate)
 {
 	Relation	rel = resultRelInfo->ri_RelationDesc;
 	TupleDesc	tupdesc = RelationGetDescr(rel);
@@ -1970,8 +1969,6 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
 	Bitmapset  *insertedCols;
 	Bitmapset  *updatedCols;
 
-	Assert(constr || resultRelInfo->ri_PartitionCheck);
-
 	if (constr && constr->has_not_null)
 	{
 		int			natts = tupdesc->natts;
@@ -2077,7 +2074,17 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
 		}
 	}
 
-	if (check_partition_constraint && resultRelInfo->ri_PartitionCheck &&
+	/*
+	 * We must validate the partition constraint when inserting directly into
+	 * a partition, or when the tuple has been routed here and a BR trigger
+	 * exists.  Such a trigger could have changed the tuple so that it
+	 * violates the partition constraint.  If no such trigger exists then the
+	 * correct partition has already been selected, so we can skip the check.
+	 */
+	if (resultRelInfo->ri_PartitionCheck != NIL &&
+		(resultRelInfo->ri_PartitionRoot == NULL ||
+		 (resultRelInfo->ri_TrigDesc &&
+		  resultRelInfo->ri_TrigDesc->trig_insert_before_row)) &&
 		!ExecPartitionCheck(resultRelInfo, slot, estate))
 		ExecPartitionCheckEmitError(resultRelInfo, slot, estate);
 }
diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c
index 4fbdfc0a09..bbee8b9c9b 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -412,8 +412,7 @@ ExecSimpleRelationInsert(EState *estate, TupleTableSlot *slot)
 		List	   *recheckIndexes = NIL;
 
 		/* Check the constraints of the tuple */
-		if (rel->rd_att->constr)
-			ExecConstraints(resultRelInfo, slot, estate, true);
+		ExecConstraints(resultRelInfo, slot, estate);
 
 		/* Store the slot into tuple that we can inspect. */
 		tuple = ExecMaterializeSlot(slot);
@@ -477,8 +476,7 @@ ExecSimpleRelationUpdate(EState *estate, EPQState *epqstate,
 		List	   *recheckIndexes = NIL;
 
 		/* Check the constraints of the tuple */
-		if (rel->rd_att->constr)
-			ExecConstraints(resultRelInfo, slot, estate, true);
+		ExecConstraints(resultRelInfo, slot, estate);
 
 		/* Store the slot into tuple that we can write. */
 		tuple = ExecMaterializeSlot(slot);
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index c4c841cdd7..070920e4b5 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -365,16 +365,6 @@ ExecInsert(ModifyTableState *mtstate,
 	else
 	{
 		WCOKind		wco_kind;
-		bool		check_partition_constr;
-
-		/*
-		 * We always check the partition constraint, including when the tuple
-		 * got here via tuple-routing.  However we don't need to in the latter
-		 * case if no BR trigger is defined on the partition.  Note that a BR
-		 * trigger might modify the tuple such that the partition constraint
-		 * is no longer satisfied, so we need to check in that case.
-		 */
-		check_partition_constr = (resultRelInfo->ri_PartitionCheck != NIL);
 
 		/*
 		 * Constraints might reference the tableoid column, so initialize
@@ -401,18 +391,8 @@ ExecInsert(ModifyTableState *mtstate,
 		if (resultRelInfo->ri_WithCheckOptions != NIL)
 			ExecWithCheckOptions(wco_kind, resultRelInfo, slot, estate);
 
-		/*
-		 * No need though if the tuple has been routed, and a BR trigger
-		 * doesn't exist.
-		 */
-		if (resultRelInfo->ri_PartitionRoot != NULL &&
-			!(resultRelInfo->ri_TrigDesc &&
-			  resultRelInfo->ri_TrigDesc->trig_insert_before_row))
-			check_partition_constr = false;
-
 		/* Check the constraints of the tuple */
-		if (resultRelationDesc->rd_att->constr || check_partition_constr)
-			ExecConstraints(resultRelInfo, slot, estate, true);
+		ExecConstraints(resultRelInfo, slot, estate);
 
 		if (onconflict != ONCONFLICT_NONE && resultRelInfo->ri_NumIndices > 0)
 		{
@@ -1170,8 +1150,7 @@ lreplace:;
 		 * will call ExecConstraints() and have it validate all remaining
 		 * checks.
 		 */
-		if (resultRelationDesc->rd_att->constr)
-			ExecConstraints(resultRelInfo, slot, estate, false);
+		ExecConstraints(resultRelInfo, slot, estate);
 
 		/*
 		 * replace the heap tuple
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index a7ea3c7d10..d1e9471ae8 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -180,8 +180,7 @@ extern ResultRelInfo *ExecGetTriggerResultRel(EState *estate, Oid relid);
 extern void ExecCleanUpTriggerState(EState *estate);
 extern bool ExecContextForcesOids(PlanState *planstate, bool *hasoids);
 extern void ExecConstraints(ResultRelInfo *resultRelInfo,
-				TupleTableSlot *slot, EState *estate,
-				bool check_partition_constraint);
+				TupleTableSlot *slot, EState *estate);
 extern bool ExecPartitionCheck(ResultRelInfo *resultRelInfo,
 				   TupleTableSlot *slot, EState *estate);
 extern void ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo,