On 2018/06/07 13:10, David Rowley wrote:
> On 7 June 2018 at 16:05, Amit Langote <langote_amit...@lab.ntt.co.jp> wrote:
>> Or we could just not have a separate function and put the logic that
>> determines whether or not to check the partition constraint right before
>> the following piece of code in ExecConstraints
>>
>> if (check_partition_constraint && resultRelInfo->ri_PartitionCheck &&
>> !ExecPartitionCheck(resultRelInfo, slot, estate))
>> ExecPartitionCheckEmitError(resultRelInfo, slot, estate);
>
> I don't think so. The function Alvaro is talking about is specific to
> inserting a tuple into a table. The place you're proposing to put the
> call to it is not.
Well, we need to determine whether or not to check the partition
constraint exactly before inserting a tuple into a table and that's also
when ExecConstraints is called, so this objection is not clear to me.
I'm saying that instead of encapsulating that logic in a new function and
calling it from a number of places, refactor things such that we call
ExecConstraints unconditionally and decide there which constraints to
check and which ones to not. Having that logic separated from
ExecConstraints is what caused us to have this discussion in the first place.
I'm attaching a patch here to show what I'm saying.
Thanks,
Amit
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,
