On 2018-Jun-09, David Rowley wrote:
> Of course, that could be fixed by adding something like "bool
> isinsert" to ExecConstraints(), so that it does not do the needless
> check on UPDATE,
Yeah, that was my actual suggestion rather than using Amit's patch
verbatim.
> but I'm strongly against the reduction in modularity.
> I quite like ExecConstraints() the way it is.
I see.
Maybe this business of sticking the partition constraint check in
ExecConstraints was a bad idea all along. How about we rip it out and
move the responsibility of doing ExecPartitionCheck to the caller
instead? See attached. This whole business looks much cleaner to me
this way.
BTW, while working on this, I was a bit disturbed by the
execReplication.c changes (namely: if the partitioning is not identical
on both sides, things are likely to blow up pretty badly), but that's a
separate topic. I didn't see any tests of logical replication with
partitioned tables ... Is the current understanding that we don't
promise those things to work terribly well together?
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 5d57489c6d8c5e1b10413362005ae333acd6c243 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Wed, 6 Jun 2018 15:08:23 -0400
Subject: [PATCH] Fix situation with partition constraints
---
src/backend/commands/copy.c | 30 +++++++++------------------
src/backend/executor/execMain.c | 32 ++++++++++++++++-------------
src/backend/executor/execPartition.c | 5 ++---
src/backend/executor/execReplication.c | 8 ++++++--
src/backend/executor/nodeModifyTable.c | 37 +++++++++++-----------------------
src/include/executor/executor.h | 5 ++---
6 files changed, 49 insertions(+), 68 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 0a1706c0d1..5f4ffd2975 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2726,29 +2726,17 @@ 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.
+ * Check the tuple is valid against all
constraints, and the
+ * partition constraint, as required.
*/
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);
+ 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 3d12f9c76f..c394b5dbed 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1856,14 +1856,17 @@ 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 +1893,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 */
+ if (!success && emitError)
+ ExecPartitionCheckEmitError(resultRelInfo, slot, estate);
+
+ return success;
}
/*
@@ -1951,17 +1960,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 +2085,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 c83991c93c..bb9bf5afa4 100644
--- 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 4fbdfc0a09..41e857e378 100644
--- 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 c4c841cdd7..c75d66ab83 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
@@ -402,17 +392,16 @@ 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 tuple is valid against all constraints, and the
partition
+ * constraint, as required.
*/
- 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);
+ if (resultRelationDesc->rd_att->constr)
+ ExecConstraints(resultRelInfo, slot, estate);
+ 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 +1026,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)
@@ -1166,12 +1155,10 @@ lreplace:;
* slot for the orig_slot argument, because unlike
ExecInsert(), no
* tuple-routing is performed here, hence the slot remains
unchanged.
* We've already checked the partition constraint above;
however, we
- * must still ensure the tuple passes all other constraints, so
we
- * will call ExecConstraints() and have it validate all
remaining
- * checks.
+ * must still ensure the tuple passes all other constraints.
*/
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..f82b51667f 100644
--- 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,
--
2.11.0
