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,