Don't needlessly check the partition contraint twice

Starting with commit f0e44751d717, ExecConstraints was in charge of
running the partition constraint; commit 19c47e7c8202 modified that so
that caller could request to skip that checking depending on some
conditions, but that commit and 15ce775faa42 together introduced a small
bug there which caused ExecInsert to request skipping the constraint
check but have this not be honored -- in effect doing the check twice.
This could have been fixed in a very small patch, but on further
analysis of the involved function and its callsites, it turns out to be
simpler to give the responsibility of checking the partition constraint
fully to the caller, and return ExecConstraints to its original
(pre-partitioning) shape where it only checked tuple descriptor-related
constraints.  Each caller must do partition constraint checking on its
own schedule, which is more convenient after commit 2f178441044 anyway.

Reported-by: David Rowley
Author: David Rowley, Álvaro Herrera
Reviewed-by: Amit Langote, Amit Khandekar, Simon Riggs
Discussion: https://postgr.es/m/CAKJS1f8w8+awsxgea8wt7_UX8qzOQ=Tm1LD+U1fHqBAkXxkW2w@mail.gmail.com

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 0a1706c0d135a906fc10cd1fc42a157122d96be8..3a66cb5025f16aee75637916329479a596905238 100644 (file)
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2725,30 +2725,25 @@ 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);
+                                       resultRelInfo->ri_RelationDesc->rd_att->constr)
+                                       ExecConstraints(resultRelInfo, slot, estate);
+
+                               /*
+                                * Also check the tuple against the partition constraint, if
+                                * there is one; except that if we got here via tuple-routing,
+                                * we don't need to if there's no BR trigger defined on the
+                                * partition.
+                                */
+                               if (resultRelInfo->ri_PartitionCheck &&
+                                       (saved_resultRelInfo == NULL ||
+                                        (resultRelInfo->ri_TrigDesc &&
+                                         resultRelInfo->ri_TrigDesc->trig_insert_before_row)))
+                                       ExecPartitionCheck(resultRelInfo, slot, estate, true);
 
                                if (useHeapMultiInsert)
                                {

diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 1dbaa6597ba4fdd8d0144c9e852af4c2b6ea52fa..969944cc12ace12cfd70f58a6553f12be17d23fb 100644 (file)
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1856,14 +1856,16 @@ ExecRelCheck(ResultRelInfo *resultRelInfo,
 /*
  * ExecPartitionCheck --- check that tuple meets the partition constraint.
  *
- * Exported in executor.h for outside use.
- * Returns true if it meets the partition constraint, else returns false.
+ * Returns true if it meets the partition constraint.  If the constraint
+ * fails and we're asked to emit to error, do so and don't return; otherwise
+ * return false.
  */
 bool
 ExecPartitionCheck(ResultRelInfo *resultRelInfo, TupleTableSlot *slot,
-                                  EState *estate)
+                                  EState *estate, bool emitError)
 {
        ExprContext *econtext;
+       bool    success;
 
        /*
         * If first time through, build expression state tree for the partition
@@ -1890,7 +1892,13 @@ ExecPartitionCheck(ResultRelInfo *resultRelInfo, TupleTableSlot *slot,
         * As in case of the catalogued constraints, we treat a NULL result as
         * success here, not a failure.
         */
-       return ExecCheck(resultRelInfo->ri_PartitionCheckExpr, econtext);
+       success = ExecCheck(resultRelInfo->ri_PartitionCheckExpr, econtext);
+
+       /* if asked to emit error, don't actually return on failure */
+       if (!success && emitError)
+               ExecPartitionCheckEmitError(resultRelInfo, slot, estate);
+
+       return success;
 }
 
 /*
@@ -1951,17 +1959,17 @@ 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.
+ * This checks the traditional NOT NULL and check constraints.
+ *
+ * The partition constraint is *NOT* checked.
  *
  * Note: 'slot' contains the tuple to check the constraints of, which may
  * have been converted from the original input tuple after tuple routing.
- * 'resultRelInfo' is the original result relation, before tuple routing.
+ * 'resultRelInfo' is the final result relation, after tuple routing.
  */
 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);
@@ -2076,13 +2084,8 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
                                         errtableconstraint(orig_rel, failed)));
                }
        }
-
-       if (check_partition_constraint && resultRelInfo->ri_PartitionCheck &&
-               !ExecPartitionCheck(resultRelInfo, slot, estate))
-               ExecPartitionCheckEmitError(resultRelInfo, slot, estate);
 }
 
-
 /*
  * ExecWithCheckOptions -- check that tuple satisfies any WITH CHECK OPTIONs
  * of the specified kind.

diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index 0a003d993513bdc379340f94be8cef0cdbd7b651..80c12cfcc0deb0947e963d44a6bbd529b568fbab 100644 (file)
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -201,9 +201,8 @@ ExecFindPartition(ResultRelInfo *resultRelInfo, PartitionDispatch *pd,
         * First check the root table's partition constraint, if any.  No point in
         * routing the tuple if it doesn't belong in the root table itself.
         */
-       if (resultRelInfo->ri_PartitionCheck &&
-               !ExecPartitionCheck(resultRelInfo, slot, estate))
-               ExecPartitionCheckEmitError(resultRelInfo, slot, estate);
+       if (resultRelInfo->ri_PartitionCheck)
+               ExecPartitionCheck(resultRelInfo, slot, estate, true);
 
        /* start with the root partitioned table */
        parent = pd[0];

diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c
index 4fbdfc0a099838ba85c48a9d7775570d5149f6f4..41e857e3781c228a474638c980fe5a26827a8de1 100644 (file)
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -413,7 +413,9 @@ ExecSimpleRelationInsert(EState *estate, TupleTableSlot *slot)
 
                /* Check the constraints of the tuple */
                if (rel->rd_att->constr)
-                       ExecConstraints(resultRelInfo, slot, estate, true);
+                       ExecConstraints(resultRelInfo, slot, estate);
+               if (resultRelInfo->ri_PartitionCheck)
+                       ExecPartitionCheck(resultRelInfo, slot, estate, true);
 
                /* Store the slot into tuple that we can inspect. */
                tuple = ExecMaterializeSlot(slot);
@@ -478,7 +480,9 @@ ExecSimpleRelationUpdate(EState *estate, EPQState *epqstate,
 
                /* Check the constraints of the tuple */
                if (rel->rd_att->constr)
-                       ExecConstraints(resultRelInfo, slot, estate, true);
+                       ExecConstraints(resultRelInfo, slot, estate);
+               if (resultRelInfo->ri_PartitionCheck)
+                       ExecPartitionCheck(resultRelInfo, slot, estate, true);
 
                /* 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 2a4dfea1511522d4880166779f9ee7ae67a45bf6..7e0b8679717a952822f39d884a62b60012092c7b 100644 (file)
--- 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
@@ -402,17 +392,21 @@ ExecInsert(ModifyTableState *mtstate,
                        ExecWithCheckOptions(wco_kind, resultRelInfo, slot, estate);
 
                /*
-                * No need though if the tuple has been routed, and a BR trigger
-                * doesn't exist.
+                * Check the constraints of the tuple.
                 */
-               if (resultRelInfo->ri_PartitionRoot != NULL &&
-                       !(resultRelInfo->ri_TrigDesc &&
-                         resultRelInfo->ri_TrigDesc->trig_insert_before_row))
-                       check_partition_constr = false;
+               if (resultRelationDesc->rd_att->constr)
+                       ExecConstraints(resultRelInfo, slot, estate);
 
-               /* Check the constraints of the tuple */
-               if (resultRelationDesc->rd_att->constr || check_partition_constr)
-                       ExecConstraints(resultRelInfo, slot, estate, true);
+               /*
+                * Also check the tuple against the partition constraint, if there is
+                * one; except that if we got here via tuple-routing, we don't need to
+                * if there's no BR trigger defined on the partition.
+                */
+               if (resultRelInfo->ri_PartitionCheck &&
+                       (resultRelInfo->ri_PartitionRoot == NULL ||
+                        (resultRelInfo->ri_TrigDesc &&
+                         resultRelInfo->ri_TrigDesc->trig_insert_before_row)))
+                       ExecPartitionCheck(resultRelInfo, slot, estate, true);
 
                if (onconflict != ONCONFLICT_NONE && resultRelInfo->ri_NumIndices > 0)
                {
@@ -1037,7 +1031,7 @@ lreplace:;
                 */
                partition_constraint_failed =
                        resultRelInfo->ri_PartitionCheck &&
-                       !ExecPartitionCheck(resultRelInfo, slot, estate);
+                       !ExecPartitionCheck(resultRelInfo, slot, estate, false);
 
                if (!partition_constraint_failed &&
                        resultRelInfo->ri_WithCheckOptions != NIL)
@@ -1168,7 +1162,7 @@ lreplace:;
                 * 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 a7ea3c7d109ef8443ee038981bb5b6d305284d98..f82b51667f453f56278f7c8f9f8ea181927a97e8 100644 (file)
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -180,10 +180,9 @@ 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);
+                                  TupleTableSlot *slot, EState *estate, bool emitError);
 extern void ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo,
                                                        TupleTableSlot *slot, EState *estate);
 extern void ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo,