Sorry about the delay in replying.
On 2016/12/23 8:08, Robert Haas wrote:
> On Thu, Dec 22, 2016 at 3:35 AM, Amit Langote
> <langote_amit...@lab.ntt.co.jp> wrote:
>> While working on that, I discovered yet-another-bug having to do with the
>> tuple descriptor that's used as we route a tuple down a partition tree. If
>> attnums of given key attribute(s) are different on different levels, it
>> would be incorrect to use the original slot's (one passed by ExecInsert())
>> tuple descriptor to inspect the original slot's heap tuple, as we go down
>> the tree. It might cause spurious "partition not found" at some level due
>> to looking at incorrect field in the input tuple because of using the
>> wrong tuple descriptor (root table's attnums not always same as other
>> partitioned tables in the tree). Patch 0001 fixes that including a test.
>
> I committed this, but I'm a bit uncomfortable with it: should the
> TupleTableSlot be part of the ModifyTableState rather than the EState?
Done that way in 0001 of the attached patches. So, instead of making the
standalone partition_tuple_slot a field of EState (with the actual
TupleTableSlot in its tupleTable), it is now allocated within
ModifyTableState and CopyState, and released when ModifyTable node or
CopyFrom finishes, respectively.
>> It also addresses the problem I mentioned previously that once
>> tuple-routing is done, we failed to switch to a slot with the leaf
>> partition's tupdesc (IOW, continued to use the original slot with root
>> table's tupdesc causing spurious failures due to differences in attums
>> between the leaf partition and the root table).
>>
>> Further patches 0002, 0003 and 0004 fix bugs that I sent one-big-patch for
>> in my previous message. Each patch has a test for the bug it's meant to fix.
>
> Regarding 0002, I think that this is kind of a strange fix. Wouldn't
> it be better to get hold of the original tuple instead of reversing
> the conversion? And what of the idea of avoiding the conversion in
> the (probably very common) case where we can?
To get hold of the original tuple, how about adding an argument orig_slot
to ExecConstraints()? I've implemented that approach in the new 0002.
Regarding the possibility of avoiding the conversion in very common cases,
I think that could be done considering the following: If the mapping from
the attribute numbers of the parent table to that of a child table is an
identity map, we don't need to convert tuples. Currently however,
convert_tuples_by_name() also requires tdtypeid of the input and output
TupleDescs to be equal. The reason cited for that is that we may fail to
"inject the right OID into the tuple datum" if the types don't match. In
case of partitioning, hasoid status must match between the parent and its
partitions at all times, so the aforementioned condition is satisfied
without requiring that tdtypeid are same. And oid column (if present) is
always located at a given position in HeapTuple, so need not map that.
Based on the above argument, patch 0006 teaches convert_tuples_by_name()
to *optionally* not require tdtypeid of input and output tuple descriptors
to be equal. It's implemented by introducing a new argument to
convert_tuples_by_name() named 'consider_typeid'. We pass 'false' only
for the partitioning cases.
(Perhaps, the following should be its own new thread)
I noticed that ExecProcessReturning() doesn't work properly after tuple
routing (example shows how returning tableoid currently fails but I
mention some other issues below):
create table p (a int, b int) partition by range (a);
create table p1 partition of p for values from (1) to (10);
insert into p values (1) returning tableoid::regclass, *;
tableoid | a | b
----------+---+---
- | 1 |
(1 row)
INSERT 0 1
I tried to fix that in 0007 to get:
insert into p values (1) returning tableoid::regclass, *;
tableoid | a | b
----------+---+---
p | 1 |
(1 row)
INSERT 0 1
But I think it *may* be wrong to return the root table OID for tuples
inserted into leaf partitions, because with select we get partition OIDs:
select tableoid::regclass, * from p;
tableoid | a | b
----------+---+---
p1 | 1 |
(1 row)
If so, that means we should build the projection info (corresponding to
the returning list) for each target partition somehow. ISTM, that's going
to have to be done within the planner by appropriate inheritance
translation of the original returning targetlist.
Thanks,
Amit
>From 89f8740195189cc77391bdb844f5092c0440f061 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Mon, 26 Dec 2016 11:53:19 +0900
Subject: [PATCH 1/7] Allocate partition_tuple_slot in respective nodes
...instead of making it part of EState and its tuple table.
Respective nodes means ModifyTableState and CopyState for now.
Reported by: n/a
Patch by: Amit Langote
Reports: n/a
---
src/backend/commands/copy.c | 30 +++++++++++++++++-------------
src/backend/executor/execMain.c | 12 ++++++++++++
src/backend/executor/nodeModifyTable.c | 17 ++++++++---------
src/include/executor/executor.h | 1 +
src/include/nodes/execnodes.h | 6 +++---
5 files changed, 41 insertions(+), 25 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index aa25a23336..e5a0f1bf80 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -161,11 +161,18 @@ typedef struct CopyStateData
ExprState **defexprs; /* array of default att expressions */
bool volatile_defexprs; /* is any of defexprs volatile? */
List *range_table;
+
PartitionDispatch *partition_dispatch_info;
- int num_dispatch;
- int num_partitions;
- ResultRelInfo *partitions;
+ /* Tuple-routing support info */
+ int num_dispatch; /* Number of entries in the above array */
+ int num_partitions; /* Number of members in the following
+ * arrays */
+ ResultRelInfo *partitions; /* Per partition result relation */
TupleConversionMap **partition_tupconv_maps;
+ /* Per partition tuple conversion map */
+ TupleTableSlot *partition_tuple_slot;
+ /* Slot used to manipulate a tuple after
+ * it is routed to a partition */
/*
* These variables are used to reduce overhead in textual COPY FROM.
@@ -1409,6 +1416,7 @@ BeginCopy(ParseState *pstate,
PartitionDispatch *partition_dispatch_info;
ResultRelInfo *partitions;
TupleConversionMap **partition_tupconv_maps;
+ TupleTableSlot *partition_tuple_slot;
int num_parted,
num_partitions;
@@ -1416,12 +1424,14 @@ BeginCopy(ParseState *pstate,
&partition_dispatch_info,
&partitions,
&partition_tupconv_maps,
+ &partition_tuple_slot,
&num_parted, &num_partitions);
cstate->partition_dispatch_info = partition_dispatch_info;
cstate->num_dispatch = num_parted;
cstate->partitions = partitions;
cstate->num_partitions = num_partitions;
cstate->partition_tupconv_maps = partition_tupconv_maps;
+ cstate->partition_tuple_slot = partition_tuple_slot;
}
}
else
@@ -2436,15 +2446,6 @@ CopyFrom(CopyState cstate)
estate->es_trig_tuple_slot = ExecInitExtraTupleSlot(estate);
/*
- * Initialize a dedicated slot to manipulate tuples of any given
- * partition's rowtype.
- */
- if (cstate->partition_dispatch_info)
- estate->es_partition_tuple_slot = ExecInitExtraTupleSlot(estate);
- else
- estate->es_partition_tuple_slot = NULL;
-
- /*
* It's more efficient to prepare a bunch of tuples for insertion, and
* insert them in one heap_multi_insert() call, than call heap_insert()
* separately for every tuple. However, we can't do that if there are
@@ -2591,7 +2592,7 @@ CopyFrom(CopyState cstate)
* we're finished dealing with the partition.
*/
oldslot = slot;
- slot = estate->es_partition_tuple_slot;
+ slot = cstate->partition_tuple_slot;
Assert(slot != NULL);
ExecSetSlotDescriptor(slot, RelationGetDescr(partrel));
ExecStoreTuple(tuple, slot, InvalidBuffer, true);
@@ -2756,6 +2757,9 @@ CopyFrom(CopyState cstate)
ExecCloseIndices(resultRelInfo);
heap_close(resultRelInfo->ri_RelationDesc, NoLock);
}
+
+ /* Release the standalone partition tuple descriptor */
+ ExecDropSingleTupleTableSlot(cstate->partition_tuple_slot);
}
FreeExecutorState(estate);
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index bca34a509c..97c729d6b7 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -3012,6 +3012,9 @@ EvalPlanQualEnd(EPQState *epqstate)
* entry for every leaf partition (required to convert input tuple based
* on the root table's rowtype to a leaf partition's rowtype after tuple
* routing is done
+ * 'partition_tuple_slot' receives a standalone TupleTableSlot to be used
+ * to manipulate any given leaf partition's rowtype after that partition
+ * is chosen by tuple-routing.
* 'num_parted' receives the number of partitioned tables in the partition
* tree (= the number of entries in the 'pd' output array)
* 'num_partitions' receives the number of leaf partitions in the partition
@@ -3026,6 +3029,7 @@ ExecSetupPartitionTupleRouting(Relation rel,
PartitionDispatch **pd,
ResultRelInfo **partitions,
TupleConversionMap ***tup_conv_maps,
+ TupleTableSlot **partition_tuple_slot,
int *num_parted, int *num_partitions)
{
TupleDesc tupDesc = RelationGetDescr(rel);
@@ -3043,6 +3047,14 @@ ExecSetupPartitionTupleRouting(Relation rel,
*tup_conv_maps = (TupleConversionMap **) palloc0(*num_partitions *
sizeof(TupleConversionMap *));
+ /*
+ * Initialize an empty slot that will be used to manipulate tuples of any
+ * given partition's rowtype. It is attached to the caller-specified node
+ * (such as ModifyTableState) and released when the node finishes
+ * processing.
+ */
+ *partition_tuple_slot = MakeTupleTableSlot();
+
leaf_part_rri = *partitions;
i = 0;
foreach(cell, leaf_parts)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 0d85b151c2..df21f66df8 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -329,7 +329,7 @@ ExecInsert(ModifyTableState *mtstate,
* Use the dedicated slot for that.
*/
oldslot = slot;
- slot = estate->es_partition_tuple_slot;
+ slot = mtstate->mt_partition_tuple_slot;
Assert(slot != NULL);
ExecSetSlotDescriptor(slot, RelationGetDescr(partrel));
ExecStoreTuple(tuple, slot, InvalidBuffer, true);
@@ -1738,6 +1738,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
PartitionDispatch *partition_dispatch_info;
ResultRelInfo *partitions;
TupleConversionMap **partition_tupconv_maps;
+ TupleTableSlot *partition_tuple_slot;
int num_parted,
num_partitions;
@@ -1745,21 +1746,15 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
&partition_dispatch_info,
&partitions,
&partition_tupconv_maps,
+ &partition_tuple_slot,
&num_parted, &num_partitions);
mtstate->mt_partition_dispatch_info = partition_dispatch_info;
mtstate->mt_num_dispatch = num_parted;
mtstate->mt_partitions = partitions;
mtstate->mt_num_partitions = num_partitions;
mtstate->mt_partition_tupconv_maps = partition_tupconv_maps;
-
- /*
- * Initialize a dedicated slot to manipulate tuples of any given
- * partition's rowtype.
- */
- estate->es_partition_tuple_slot = ExecInitExtraTupleSlot(estate);
+ mtstate->mt_partition_tuple_slot = partition_tuple_slot;
}
- else
- estate->es_partition_tuple_slot = NULL;
/*
* Initialize any WITH CHECK OPTION constraints if needed.
@@ -2100,6 +2095,10 @@ ExecEndModifyTable(ModifyTableState *node)
heap_close(resultRelInfo->ri_RelationDesc, NoLock);
}
+ /* Release the standalone partition tuple descriptor, if any */
+ if (node->mt_partition_tuple_slot)
+ ExecDropSingleTupleTableSlot(node->mt_partition_tuple_slot);
+
/*
* Free the exprcontext
*/
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index b74fa5eb5d..c217bd30cb 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -217,6 +217,7 @@ extern void ExecSetupPartitionTupleRouting(Relation rel,
PartitionDispatch **pd,
ResultRelInfo **partitions,
TupleConversionMap ***tup_conv_maps,
+ TupleTableSlot **partition_tuple_slot,
int *num_parted, int *num_partitions);
extern int ExecFindPartition(ResultRelInfo *resultRelInfo,
PartitionDispatch *pd,
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index d43ec56a2b..3624660861 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -384,9 +384,6 @@ typedef struct EState
TupleTableSlot *es_trig_oldtup_slot; /* for TriggerEnabled */
TupleTableSlot *es_trig_newtup_slot; /* for TriggerEnabled */
- /* Slot used to manipulate a tuple after it is routed to a partition */
- TupleTableSlot *es_partition_tuple_slot;
-
/* Parameter info: */
ParamListInfo es_param_list_info; /* values of external params */
ParamExecData *es_param_exec_vals; /* values of internal params */
@@ -1165,6 +1162,9 @@ typedef struct ModifyTableState
ResultRelInfo *mt_partitions; /* Per partition result relation */
TupleConversionMap **mt_partition_tupconv_maps;
/* Per partition tuple conversion map */
+ TupleTableSlot *mt_partition_tuple_slot;
+ /* Slot used to manipulate a tuple after
+ * it is routed to a partition */
} ModifyTableState;
/* ----------------
--
2.11.0
>From 41d3c159e9ec1770900638469c8354ff739af025 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Thu, 15 Dec 2016 18:00:47 +0900
Subject: [PATCH 2/7] Make ExecConstraints() show the correct row in error msgs
After a tuple is routed to a partition, it has been converted from the
root table's rowtype to the partition's. If such a tuple causes an
error in ExecConstraints(), the row shown in error messages might not
match the input row due to possible differences between the root table's
(ie, the table into which the row is inserted in a given query) rowtype
and the partition's.
To fix, also pass the original slot to ExecConstraints and use it to
build the val_desc to be shown in the messages.
Reported by: n/a
Patch by: Amit Langote
Reports: n/a
---
src/backend/commands/copy.c | 11 ++----
src/backend/commands/tablecmds.c | 1 +
src/backend/executor/execMain.c | 67 +++++++++++++++++++++++++++++-----
src/backend/executor/nodeModifyTable.c | 15 +++-----
src/include/executor/executor.h | 4 +-
src/include/nodes/execnodes.h | 1 +
src/test/regress/expected/insert.out | 7 ++++
src/test/regress/sql/insert.sql | 6 +++
8 files changed, 86 insertions(+), 26 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index e5a0f1bf80..afbfb9f6e8 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2430,6 +2430,7 @@ CopyFrom(CopyState cstate)
cstate->rel,
1, /* dummy rangetable index */
true, /* do load partition check expression */
+ NULL,
0);
ExecOpenIndices(resultRelInfo, false);
@@ -2495,7 +2496,7 @@ CopyFrom(CopyState cstate)
for (;;)
{
TupleTableSlot *slot,
- *oldslot = NULL;
+ *oldslot;
bool skip_tuple;
Oid loaded_oid = InvalidOid;
@@ -2537,6 +2538,7 @@ CopyFrom(CopyState cstate)
ExecStoreTuple(tuple, slot, InvalidBuffer, false);
/* Determine the partition to heap_insert the tuple into */
+ oldslot = slot;
if (cstate->partition_dispatch_info)
{
int leaf_part_index;
@@ -2591,7 +2593,6 @@ CopyFrom(CopyState cstate)
* point on. Use a dedicated slot from this point on until
* we're finished dealing with the partition.
*/
- oldslot = slot;
slot = cstate->partition_tuple_slot;
Assert(slot != NULL);
ExecSetSlotDescriptor(slot, RelationGetDescr(partrel));
@@ -2628,7 +2629,7 @@ CopyFrom(CopyState cstate)
/* Check the constraints of the tuple */
if (cstate->rel->rd_att->constr ||
resultRelInfo->ri_PartitionCheck)
- ExecConstraints(resultRelInfo, slot, estate);
+ ExecConstraints(resultRelInfo, slot, oldslot, estate);
if (useHeapMultiInsert)
{
@@ -2690,10 +2691,6 @@ CopyFrom(CopyState cstate)
{
resultRelInfo = saved_resultRelInfo;
estate->es_result_relation_info = resultRelInfo;
-
- /* Switch back to the slot corresponding to the root table */
- Assert(oldslot != NULL);
- slot = oldslot;
}
}
}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index a7ac85e7ab..c03edea18d 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1324,6 +1324,7 @@ ExecuteTruncate(TruncateStmt *stmt)
rel,
0, /* dummy rangetable index */
false,
+ NULL,
0);
resultRelInfo++;
}
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 97c729d6b7..32c8f28beb 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -828,6 +828,7 @@ InitPlan(QueryDesc *queryDesc, int eflags)
resultRelation,
resultRelationIndex,
true,
+ NULL,
estate->es_instrument);
resultRelInfo++;
}
@@ -1218,6 +1219,7 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
bool load_partition_check,
+ Relation partition_root,
int instrument_options)
{
MemSet(resultRelInfo, 0, sizeof(ResultRelInfo));
@@ -1259,6 +1261,11 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
resultRelInfo->ri_PartitionCheck =
RelationGetPartitionQual(resultRelationDesc,
true);
+ /*
+ * The following gets set to NULL unless we are initializing leaf
+ * partitions for tuple-routing.
+ */
+ resultRelInfo->ri_PartitionRoot = partition_root;
}
/*
@@ -1322,6 +1329,7 @@ ExecGetTriggerResultRel(EState *estate, Oid relid)
rel,
0, /* dummy rangetable index */
true,
+ NULL,
estate->es_instrument);
estate->es_trig_target_relations =
lappend(estate->es_trig_target_relations, rInfo);
@@ -1743,9 +1751,21 @@ ExecPartitionCheck(ResultRelInfo *resultRelInfo, TupleTableSlot *slot,
return ExecQual(resultRelInfo->ri_PartitionCheckExpr, econtext, true);
}
+/*
+ * ExecConstraints - check constraints of the tuple in 'slot'
+ *
+ * This checks the traditional NOT NULL and check constraints, as well as
+ * the partition constraint, if any.
+ *
+ * Note: 'slot' contains the tuple to check the constraints of, which may
+ * have been converted from the original input tuple after tuple routing,
+ * while 'orig_slot' contains the original tuple to be shown in the message,
+ * if an error occurs.
+ */
void
ExecConstraints(ResultRelInfo *resultRelInfo,
- TupleTableSlot *slot, EState *estate)
+ TupleTableSlot *slot, TupleTableSlot *orig_slot,
+ EState *estate)
{
Relation rel = resultRelInfo->ri_RelationDesc;
TupleDesc tupdesc = RelationGetDescr(rel);
@@ -1767,12 +1787,24 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
slot_attisnull(slot, attrChk))
{
char *val_desc;
+ Relation orig_rel = rel;
+ TupleDesc orig_tupdesc = tupdesc;
+
+ /*
+ * choose the correct relation to build val_desc from the
+ * tuple contained in orig_slot
+ */
+ if (resultRelInfo->ri_PartitionRoot)
+ {
+ rel = resultRelInfo->ri_PartitionRoot;
+ tupdesc = RelationGetDescr(rel);
+ }
insertedCols = GetInsertedColumns(resultRelInfo, estate);
updatedCols = GetUpdatedColumns(resultRelInfo, estate);
modifiedCols = bms_union(insertedCols, updatedCols);
val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel),
- slot,
+ orig_slot,
tupdesc,
modifiedCols,
64);
@@ -1780,9 +1812,9 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
ereport(ERROR,
(errcode(ERRCODE_NOT_NULL_VIOLATION),
errmsg("null value in column \"%s\" violates not-null constraint",
- NameStr(tupdesc->attrs[attrChk - 1]->attname)),
+ NameStr(orig_tupdesc->attrs[attrChk - 1]->attname)),
val_desc ? errdetail("Failing row contains %s.", val_desc) : 0,
- errtablecol(rel, attrChk)));
+ errtablecol(orig_rel, attrChk)));
}
}
}
@@ -1794,21 +1826,29 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
if ((failed = ExecRelCheck(resultRelInfo, slot, estate)) != NULL)
{
char *val_desc;
+ Relation orig_rel = rel;
+
+ /* See the comment above. */
+ if (resultRelInfo->ri_PartitionRoot)
+ {
+ rel = resultRelInfo->ri_PartitionRoot;
+ tupdesc = RelationGetDescr(rel);
+ }
insertedCols = GetInsertedColumns(resultRelInfo, estate);
updatedCols = GetUpdatedColumns(resultRelInfo, estate);
modifiedCols = bms_union(insertedCols, updatedCols);
val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel),
- slot,
+ orig_slot,
tupdesc,
modifiedCols,
64);
ereport(ERROR,
(errcode(ERRCODE_CHECK_VIOLATION),
errmsg("new row for relation \"%s\" violates check constraint \"%s\"",
- RelationGetRelationName(rel), failed),
+ RelationGetRelationName(orig_rel), failed),
val_desc ? errdetail("Failing row contains %s.", val_desc) : 0,
- errtableconstraint(rel, failed)));
+ errtableconstraint(orig_rel, failed)));
}
}
@@ -1816,19 +1856,27 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
!ExecPartitionCheck(resultRelInfo, slot, estate))
{
char *val_desc;
+ Relation orig_rel = rel;
+
+ /* See the comment above. */
+ if (resultRelInfo->ri_PartitionRoot)
+ {
+ rel = resultRelInfo->ri_PartitionRoot;
+ tupdesc = RelationGetDescr(rel);
+ }
insertedCols = GetInsertedColumns(resultRelInfo, estate);
updatedCols = GetUpdatedColumns(resultRelInfo, estate);
modifiedCols = bms_union(insertedCols, updatedCols);
val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel),
- slot,
+ orig_slot,
tupdesc,
modifiedCols,
64);
ereport(ERROR,
(errcode(ERRCODE_CHECK_VIOLATION),
errmsg("new row for relation \"%s\" violates partition constraint",
- RelationGetRelationName(rel)),
+ RelationGetRelationName(orig_rel)),
val_desc ? errdetail("Failing row contains %s.", val_desc) : 0));
}
}
@@ -3086,6 +3134,7 @@ ExecSetupPartitionTupleRouting(Relation rel,
partrel,
1, /* dummy */
false,
+ rel,
0);
/*
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index df21f66df8..825a15f42d 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -262,7 +262,7 @@ ExecInsert(ModifyTableState *mtstate,
Relation resultRelationDesc;
Oid newId;
List *recheckIndexes = NIL;
- TupleTableSlot *oldslot = NULL;
+ TupleTableSlot *oldslot = slot;
/*
* get the heap tuple out of the tuple table slot, making sure we have a
@@ -328,7 +328,6 @@ ExecInsert(ModifyTableState *mtstate,
* point on, until we're finished dealing with the partition.
* Use the dedicated slot for that.
*/
- oldslot = slot;
slot = mtstate->mt_partition_tuple_slot;
Assert(slot != NULL);
ExecSetSlotDescriptor(slot, RelationGetDescr(partrel));
@@ -434,7 +433,7 @@ ExecInsert(ModifyTableState *mtstate,
* Check the constraints of the tuple
*/
if (resultRelationDesc->rd_att->constr || resultRelInfo->ri_PartitionCheck)
- ExecConstraints(resultRelInfo, slot, estate);
+ ExecConstraints(resultRelInfo, slot, oldslot, estate);
if (onconflict != ONCONFLICT_NONE && resultRelInfo->ri_NumIndices > 0)
{
@@ -579,10 +578,6 @@ ExecInsert(ModifyTableState *mtstate,
{
resultRelInfo = saved_resultRelInfo;
estate->es_result_relation_info = resultRelInfo;
-
- /* Switch back to the slot corresponding to the root table */
- Assert(oldslot != NULL);
- slot = oldslot;
}
/*
@@ -994,10 +989,12 @@ lreplace:;
resultRelInfo, slot, estate);
/*
- * Check the constraints of the tuple
+ * Check the constraints of the tuple. Note that we pass the same
+ * slot for the orig_slot argument, because unlike ExecInsert(), no
+ * tuple-routing is performed here, hence the slot remains unchanged.
*/
if (resultRelationDesc->rd_att->constr || resultRelInfo->ri_PartitionCheck)
- ExecConstraints(resultRelInfo, slot, estate);
+ ExecConstraints(resultRelInfo, slot, slot, estate);
/*
* replace the heap tuple
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index c217bd30cb..70ecf108a3 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -190,11 +190,13 @@ extern void InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
bool load_partition_check,
+ Relation partition_root,
int instrument_options);
extern ResultRelInfo *ExecGetTriggerResultRel(EState *estate, Oid relid);
extern bool ExecContextForcesOids(PlanState *planstate, bool *hasoids);
extern void ExecConstraints(ResultRelInfo *resultRelInfo,
- TupleTableSlot *slot, EState *estate);
+ TupleTableSlot *slot, TupleTableSlot *orig_slot,
+ EState *estate);
extern void ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo,
TupleTableSlot *slot, EState *estate);
extern LockTupleMode ExecUpdateLockMode(EState *estate, ResultRelInfo *relinfo);
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 3624660861..633c5cc107 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -349,6 +349,7 @@ typedef struct ResultRelInfo
List *ri_onConflictSetWhere;
List *ri_PartitionCheck;
List *ri_PartitionCheckExpr;
+ Relation ri_PartitionRoot;
} ResultRelInfo;
/* ----------------
diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out
index 49f667b119..b120954997 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -332,6 +332,13 @@ select tableoid::regclass, * from p;
p11 | 1 | 2
(1 row)
+truncate p;
+alter table p add constraint check_b check (b = 3);
+-- check that correct input row is shown when constraint check_b fails on p11
+-- after "(1, 2)" is routed to it
+insert into p values (1, 2);
+ERROR: new row for relation "p11" violates check constraint "check_b"
+DETAIL: Failing row contains (1, 2).
-- cleanup
drop table p cascade;
NOTICE: drop cascades to 2 other objects
diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql
index 08dc068de8..3d2fdb92c5 100644
--- a/src/test/regress/sql/insert.sql
+++ b/src/test/regress/sql/insert.sql
@@ -194,5 +194,11 @@ alter table p attach partition p1 for values from (1, 2) to (1, 10);
insert into p values (1, 2);
select tableoid::regclass, * from p;
+truncate p;
+alter table p add constraint check_b check (b = 3);
+-- check that correct input row is shown when constraint check_b fails on p11
+-- after "(1, 2)" is routed to it
+insert into p values (1, 2);
+
-- cleanup
drop table p cascade;
--
2.11.0
>From 4ba765e88b2a3569690ded7277edb276c1200f8f Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Thu, 15 Dec 2016 16:27:04 +0900
Subject: [PATCH 3/7] Fix a bug in how we generate partition constraints
Firstly, since we always want to recurse when calling
RelationGetPartitionQual(), ie, consider the parent's partition
constraint (if any), get rid of the argument recurse; also in the
module-local generate_partition_qual() that it calls.
Move the code for doing parent attnos to child attnos mapping for Vars
in partition constraint expressions to a separate function
map_partition_varattnos() and call it from the appropriate places.
Doing it in get_qual_from_partbound(), as is now, would produce wrong
result in certain multi-level partitioning cases, because it only
considers the current pair of parent-child relations. In certain
multi-level partitioning cases, attnums for the same key attribute(s)
might differ between different pairs of consecutive levels causing the
same attribute to be numbered differently in different Vars of the same
expression tree. Remember that we apply the whole partition constraint
(list of constraints of partitions at various levels) to a single (leaf
partition) relation.
With this commit, in generate_partition_qual(), we first generate the
the whole partition constraint (considering all levels of partitioning)
and then do the mapping from the root parent attnums to leaf partition
attnums.
Reported by: n/a
Patch by: Amit Langote
Reports: n/a
---
src/backend/catalog/partition.c | 103 ++++++++++++++++--------------
src/backend/commands/tablecmds.c | 9 ++-
src/backend/executor/execMain.c | 4 +-
src/backend/optimizer/util/plancat.c | 2 +-
src/include/catalog/partition.h | 3 +-
src/test/regress/expected/alter_table.out | 30 +++++++++
src/test/regress/sql/alter_table.sql | 25 ++++++++
7 files changed, 122 insertions(+), 54 deletions(-)
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index fca874752f..34ab812b44 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -122,7 +122,7 @@ static List *get_qual_for_list(PartitionKey key, PartitionBoundSpec *spec);
static List *get_qual_for_range(PartitionKey key, PartitionBoundSpec *spec);
static Oid get_partition_operator(PartitionKey key, int col,
StrategyNumber strategy, bool *need_relabel);
-static List *generate_partition_qual(Relation rel, bool recurse);
+static List *generate_partition_qual(Relation rel);
static PartitionRangeBound *make_one_range_bound(PartitionKey key, int index,
List *datums, bool lower);
@@ -850,10 +850,6 @@ get_qual_from_partbound(Relation rel, Relation parent, Node *bound)
PartitionBoundSpec *spec = (PartitionBoundSpec *) bound;
PartitionKey key = RelationGetPartitionKey(parent);
List *my_qual = NIL;
- TupleDesc parent_tupdesc = RelationGetDescr(parent);
- AttrNumber parent_attno;
- AttrNumber *partition_attnos;
- bool found_whole_row;
Assert(key != NULL);
@@ -874,38 +870,51 @@ get_qual_from_partbound(Relation rel, Relation parent, Node *bound)
(int) key->strategy);
}
- /*
- * Translate vars in the generated expression to have correct attnos. Note
- * that the vars in my_qual bear attnos dictated by key which carries
- * physical attnos of the parent. We must allow for a case where physical
- * attnos of a partition can be different from the parent.
- */
- partition_attnos = (AttrNumber *)
- palloc0(parent_tupdesc->natts * sizeof(AttrNumber));
- for (parent_attno = 1; parent_attno <= parent_tupdesc->natts;
- parent_attno++)
+ return my_qual;
+}
+
+/*
+ * map_partition_varattnos - maps varattno of any Vars in expr from the
+ * parent attno to partition attno.
+ *
+ * We must allow for a case where physical attnos of a partition can be
+ * different from the parent's.
+ */
+List *
+map_partition_varattnos(List *expr, Relation partrel, Relation parent)
+{
+ TupleDesc tupdesc = RelationGetDescr(parent);
+ AttrNumber attno;
+ AttrNumber *part_attnos;
+ bool found_whole_row;
+
+ if (expr == NIL)
+ return NIL;
+
+ part_attnos = (AttrNumber *) palloc0(tupdesc->natts * sizeof(AttrNumber));
+ for (attno = 1; attno <= tupdesc->natts; attno++)
{
- Form_pg_attribute attribute = parent_tupdesc->attrs[parent_attno - 1];
+ Form_pg_attribute attribute = tupdesc->attrs[attno - 1];
char *attname = NameStr(attribute->attname);
- AttrNumber partition_attno;
+ AttrNumber part_attno;
if (attribute->attisdropped)
continue;
- partition_attno = get_attnum(RelationGetRelid(rel), attname);
- partition_attnos[parent_attno - 1] = partition_attno;
+ part_attno = get_attnum(RelationGetRelid(partrel), attname);
+ part_attnos[attno - 1] = part_attno;
}
- my_qual = (List *) map_variable_attnos((Node *) my_qual,
- 1, 0,
- partition_attnos,
- parent_tupdesc->natts,
- &found_whole_row);
- /* there can never be a whole-row reference here */
+ expr = (List *) map_variable_attnos((Node *) expr,
+ 1, 0,
+ part_attnos,
+ tupdesc->natts,
+ &found_whole_row);
+ /* There can never be a whole-row reference here */
if (found_whole_row)
elog(ERROR, "unexpected whole-row reference found in partition key");
- return my_qual;
+ return expr;
}
/*
@@ -914,13 +923,13 @@ get_qual_from_partbound(Relation rel, Relation parent, Node *bound)
* Returns a list of partition quals
*/
List *
-RelationGetPartitionQual(Relation rel, bool recurse)
+RelationGetPartitionQual(Relation rel)
{
/* Quick exit */
if (!rel->rd_rel->relispartition)
return NIL;
- return generate_partition_qual(rel, recurse);
+ return generate_partition_qual(rel);
}
/*
@@ -1480,7 +1489,7 @@ get_partition_operator(PartitionKey key, int col, StrategyNumber strategy,
* into cache memory.
*/
static List *
-generate_partition_qual(Relation rel, bool recurse)
+generate_partition_qual(Relation rel)
{
HeapTuple tuple;
MemoryContext oldcxt;
@@ -1494,6 +1503,10 @@ generate_partition_qual(Relation rel, bool recurse)
/* Guard against stack overflow due to overly deep partition tree */
check_stack_depth();
+ /* Recursive callers may not have checked themselves */
+ if (!rel->rd_rel->relispartition)
+ return NIL;
+
/* Grab at least an AccessShareLock on the parent table */
parent = heap_open(get_partition_parent(RelationGetRelid(rel)),
AccessShareLock);
@@ -1501,20 +1514,18 @@ generate_partition_qual(Relation rel, bool recurse)
/* Quick copy */
if (rel->rd_partcheck)
{
- if (parent->rd_rel->relispartition && recurse)
- result = list_concat(generate_partition_qual(parent, true),
- copyObject(rel->rd_partcheck));
- else
- result = copyObject(rel->rd_partcheck);
+ result = list_concat(generate_partition_qual(parent),
+ copyObject(rel->rd_partcheck));
- heap_close(parent, AccessShareLock);
+ /* Mark Vars with correct attnos */
+ result = map_partition_varattnos(result, rel, parent);
+
+ /* Keep the parent locked until commit */
+ heap_close(parent, NoLock);
return result;
}
/* Get pg_class.relpartbound */
- if (!rel->rd_rel->relispartition) /* should not happen */
- elog(ERROR, "relation \"%s\" has relispartition = false",
- RelationGetRelationName(rel));
tuple = SearchSysCache1(RELOID, RelationGetRelid(rel));
boundDatum = SysCacheGetAttr(RELOID, tuple,
Anum_pg_class_relpartbound,
@@ -1527,18 +1538,16 @@ generate_partition_qual(Relation rel, bool recurse)
my_qual = get_qual_from_partbound(rel, parent, bound);
- /* If requested, add parent's quals to the list (if any) */
- if (parent->rd_rel->relispartition && recurse)
- {
- List *parent_check;
-
- parent_check = generate_partition_qual(parent, true);
- result = list_concat(parent_check, my_qual);
- }
+ /* Add the parent's quals to the list (if any) */
+ if (parent->rd_rel->relispartition)
+ result = list_concat(generate_partition_qual(parent), my_qual);
else
result = my_qual;
- /* Save a copy of my_qual in the relcache */
+ /* Mark Vars with correct attnos */
+ result = map_partition_varattnos(result, rel, parent);
+
+ /* Save a copy of *only* this rel's partition qual in the relcache */
oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
rel->rd_partcheck = copyObject(my_qual);
MemoryContextSwitchTo(oldcxt);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index c03edea18d..3c08551d38 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13151,7 +13151,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
*/
partConstraint = list_concat(get_qual_from_partbound(attachRel, rel,
cmd->bound),
- RelationGetPartitionQual(rel, true));
+ RelationGetPartitionQual(rel));
partConstraint = (List *) eval_const_expressions(NULL,
(Node *) partConstraint);
partConstraint = (List *) canonicalize_qual((Expr *) partConstraint);
@@ -13325,6 +13325,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
Oid part_relid = lfirst_oid(lc);
Relation part_rel;
Expr *constr;
+ List *my_constr;
/* Lock already taken */
if (part_relid != RelationGetRelid(attachRel))
@@ -13347,8 +13348,10 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
tab = ATGetQueueEntry(wqueue, part_rel);
constr = linitial(partConstraint);
- tab->partition_constraint = make_ands_implicit((Expr *) constr);
-
+ my_constr = make_ands_implicit((Expr *) constr);
+ tab->partition_constraint = map_partition_varattnos(my_constr,
+ part_rel,
+ rel);
/* keep our lock until commit */
if (part_rel != attachRel)
heap_close(part_rel, NoLock);
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 32c8f28beb..6a82c18571 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1259,8 +1259,8 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
resultRelInfo->ri_projectReturning = NULL;
if (load_partition_check)
resultRelInfo->ri_PartitionCheck =
- RelationGetPartitionQual(resultRelationDesc,
- true);
+ RelationGetPartitionQual(resultRelationDesc);
+
/*
* The following gets set to NULL unless we are initializing leaf
* partitions for tuple-routing.
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index 72272d9bb7..150229ed6d 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -1228,7 +1228,7 @@ get_relation_constraints(PlannerInfo *root,
}
/* Append partition predicates, if any */
- pcqual = RelationGetPartitionQual(relation, true);
+ pcqual = RelationGetPartitionQual(relation);
if (pcqual)
{
/*
diff --git a/src/include/catalog/partition.h b/src/include/catalog/partition.h
index bf38df5d29..78220d6ac6 100644
--- a/src/include/catalog/partition.h
+++ b/src/include/catalog/partition.h
@@ -77,7 +77,8 @@ extern bool partition_bounds_equal(PartitionKey key,
extern void check_new_partition_bound(char *relname, Relation parent, Node *bound);
extern Oid get_partition_parent(Oid relid);
extern List *get_qual_from_partbound(Relation rel, Relation parent, Node *bound);
-extern List *RelationGetPartitionQual(Relation rel, bool recurse);
+extern List *map_partition_varattnos(List *expr, Relation partrel, Relation parent);
+extern List *RelationGetPartitionQual(Relation rel);
/* For tuple routing */
extern PartitionDispatch *RelationGetPartitionDispatchInfo(Relation rel,
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 62e18961d3..0a1d1db54e 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3317,3 +3317,33 @@ drop cascades to table part_2
drop cascades to table part_5
drop cascades to table part_5_a
drop cascades to table part_1
+-- more tests for certain multi-level partitioning scenarios
+create table p (a int, b int) partition by range (a, b);
+create table p1 (b int, a int not null) partition by range (b);
+create table p11 (like p1);
+alter table p11 drop a;
+alter table p11 add a int;
+alter table p11 drop a;
+alter table p11 add a int not null;
+-- attnum for key attribute 'a' is different in p, p1, and p11
+select attrelid::regclass, attname, attnum
+from pg_attribute
+where attname = 'a'
+ and (attrelid = 'p'::regclass
+ or attrelid = 'p1'::regclass
+ or attrelid = 'p11'::regclass);
+ attrelid | attname | attnum
+----------+---------+--------
+ p | a | 1
+ p1 | a | 2
+ p11 | a | 4
+(3 rows)
+
+alter table p1 attach partition p11 for values from (2) to (5);
+insert into p1 (a, b) values (2, 3);
+-- check that partition validation scan correctly detects violating rows
+alter table p attach partition p1 for values from (1, 2) to (1, 10);
+ERROR: partition constraint is violated by some row
+-- cleanup
+drop table p, p1 cascade;
+NOTICE: drop cascades to table p11
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index b285a406d9..ce7e85b6ad 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2169,3 +2169,28 @@ ALTER TABLE list_parted2 ALTER COLUMN b TYPE text;
-- cleanup
DROP TABLE list_parted, list_parted2, range_parted CASCADE;
+
+-- more tests for certain multi-level partitioning scenarios
+create table p (a int, b int) partition by range (a, b);
+create table p1 (b int, a int not null) partition by range (b);
+create table p11 (like p1);
+alter table p11 drop a;
+alter table p11 add a int;
+alter table p11 drop a;
+alter table p11 add a int not null;
+-- attnum for key attribute 'a' is different in p, p1, and p11
+select attrelid::regclass, attname, attnum
+from pg_attribute
+where attname = 'a'
+ and (attrelid = 'p'::regclass
+ or attrelid = 'p1'::regclass
+ or attrelid = 'p11'::regclass);
+
+alter table p1 attach partition p11 for values from (2) to (5);
+
+insert into p1 (a, b) values (2, 3);
+-- check that partition validation scan correctly detects violating rows
+alter table p attach partition p1 for values from (1, 2) to (1, 10);
+
+-- cleanup
+drop table p, p1 cascade;
--
2.11.0
>From b8269426ef2120ba9d5f981839e958871d9af901 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Tue, 13 Dec 2016 15:07:41 +0900
Subject: [PATCH 4/7] Fix a bug of insertion into an internal partition.
Since implicit partition constraints are not inherited, an internal
partition's constraint was not being enforced when targeted directly.
So, include such constraint when setting up leaf partition result
relations for tuple-routing.
Reported by: n/a
Patch by: Amit Langote
Reports: n/a
---
src/backend/commands/copy.c | 1 -
src/backend/commands/tablecmds.c | 1 -
src/backend/executor/execMain.c | 41 ++++++++++++++++++++++++++++--------
src/include/executor/executor.h | 1 -
src/test/regress/expected/insert.out | 6 ++++++
src/test/regress/sql/insert.sql | 5 +++++
6 files changed, 43 insertions(+), 12 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index afbfb9f6e8..5aa4449e3e 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2429,7 +2429,6 @@ CopyFrom(CopyState cstate)
InitResultRelInfo(resultRelInfo,
cstate->rel,
1, /* dummy rangetable index */
- true, /* do load partition check expression */
NULL,
0);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3c08551d38..2e51124eb3 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1323,7 +1323,6 @@ ExecuteTruncate(TruncateStmt *stmt)
InitResultRelInfo(resultRelInfo,
rel,
0, /* dummy rangetable index */
- false,
NULL,
0);
resultRelInfo++;
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 6a82c18571..c991a18ce8 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -824,10 +824,10 @@ InitPlan(QueryDesc *queryDesc, int eflags)
resultRelationOid = getrelid(resultRelationIndex, rangeTable);
resultRelation = heap_open(resultRelationOid, RowExclusiveLock);
+
InitResultRelInfo(resultRelInfo,
resultRelation,
resultRelationIndex,
- true,
NULL,
estate->es_instrument);
resultRelInfo++;
@@ -1218,10 +1218,11 @@ void
InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
- bool load_partition_check,
Relation partition_root,
int instrument_options)
{
+ List *partition_check = NIL;
+
MemSet(resultRelInfo, 0, sizeof(ResultRelInfo));
resultRelInfo->type = T_ResultRelInfo;
resultRelInfo->ri_RangeTableIndex = resultRelationIndex;
@@ -1257,14 +1258,38 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
resultRelInfo->ri_ConstraintExprs = NULL;
resultRelInfo->ri_junkFilter = NULL;
resultRelInfo->ri_projectReturning = NULL;
- if (load_partition_check)
- resultRelInfo->ri_PartitionCheck =
- RelationGetPartitionQual(resultRelationDesc);
/*
- * The following gets set to NULL unless we are initializing leaf
- * partitions for tuple-routing.
+ * If partition_root has been specified, that means we are builiding the
+ * ResultRelationInfo for one of its leaf partitions. In that case, we
+ * need *not* initialize the leaf partition's constraint, but rather the
+ * the partition_root's (if any). We must do that explicitly like this,
+ * because implicit partition constraints are not inherited like user-
+ * defined constraints and would fail to be enforced by ExecConstraints()
+ * after a tuple is routed to a leaf partition.
*/
+ if (partition_root)
+ {
+ /*
+ * Root table itself may or may not be a partition; partition_check
+ * would be NIL in the latter case.
+ */
+ partition_check = RelationGetPartitionQual(partition_root);
+
+ /*
+ * This is not our own partition constraint, but rather an ancestor's.
+ * So any Vars in it bear the ancestor's attribute numbers. We must
+ * switch them to our own.
+ */
+ if (partition_check != NIL)
+ partition_check = map_partition_varattnos(partition_check,
+ resultRelationDesc,
+ partition_root);
+ }
+ else
+ partition_check = RelationGetPartitionQual(resultRelationDesc);
+
+ resultRelInfo->ri_PartitionCheck = partition_check;
resultRelInfo->ri_PartitionRoot = partition_root;
}
@@ -1328,7 +1353,6 @@ ExecGetTriggerResultRel(EState *estate, Oid relid)
InitResultRelInfo(rInfo,
rel,
0, /* dummy rangetable index */
- true,
NULL,
estate->es_instrument);
estate->es_trig_target_relations =
@@ -3133,7 +3157,6 @@ ExecSetupPartitionTupleRouting(Relation rel,
InitResultRelInfo(leaf_part_rri,
partrel,
1, /* dummy */
- false,
rel,
0);
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 70ecf108a3..4fed4e101f 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -189,7 +189,6 @@ extern void CheckValidResultRel(Relation resultRel, CmdType operation);
extern void InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
- bool load_partition_check,
Relation partition_root,
int instrument_options);
extern ResultRelInfo *ExecGetTriggerResultRel(EState *estate, Oid relid);
diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out
index b120954997..6a6aac5a88 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -339,6 +339,12 @@ alter table p add constraint check_b check (b = 3);
insert into p values (1, 2);
ERROR: new row for relation "p11" violates check constraint "check_b"
DETAIL: Failing row contains (1, 2).
+-- check that inserting into an internal partition successfully results in
+-- checking its partition constraint before inserting into the leaf partition
+-- selected by tuple-routing
+insert into p1 (a, b) values (2, 3);
+ERROR: new row for relation "p11" violates partition constraint
+DETAIL: Failing row contains (3, 2).
-- cleanup
drop table p cascade;
NOTICE: drop cascades to 2 other objects
diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql
index 3d2fdb92c5..171196df6d 100644
--- a/src/test/regress/sql/insert.sql
+++ b/src/test/regress/sql/insert.sql
@@ -200,5 +200,10 @@ alter table p add constraint check_b check (b = 3);
-- after "(1, 2)" is routed to it
insert into p values (1, 2);
+-- check that inserting into an internal partition successfully results in
+-- checking its partition constraint before inserting into the leaf partition
+-- selected by tuple-routing
+insert into p1 (a, b) values (2, 3);
+
-- cleanup
drop table p cascade;
--
2.11.0
>From 10b23c7759111091173380a4806f75e38ec4b805 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Wed, 21 Dec 2016 10:51:32 +0900
Subject: [PATCH 5/7] Add some more tests for tuple-routing
We fixed some issues with how PartitionDispatch related code handled
multi-level partitioned tables in commit a25665088d, but didn't add
any tests.
Reported by: Dmitry Ivanov, Robert Haas
Patch by: Amit Langote
Reports: https://www.postgresql.org/message-id/0d5b64c9-fa05-4dab-93e7-56576d1193ca%40postgrespro.ru
https://www.postgresql.org/message-id/CA%2BTgmoZ86v1G%2Bzx9etMiSQaBBvYMKfU-iitqZArSh5z0n8Q4cA%40mail.gmail.com
---
src/test/regress/expected/insert.out | 38 +++++++++++++++++++++++++++++++++++-
src/test/regress/sql/insert.sql | 18 +++++++++++++++++
2 files changed, 55 insertions(+), 1 deletion(-)
diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out
index 6a6aac5a88..ed0513d2ff 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -285,6 +285,34 @@ select tableoid::regclass, * from list_parted;
part_ee_ff2 | EE | 10
(8 rows)
+-- some more tests to exercise tuple-routing with multi-level partitioning
+create table part_gg partition of list_parted for values in ('gg') partition by range (b);
+create table part_gg1 partition of part_gg for values from (unbounded) to (1);
+create table part_gg2 partition of part_gg for values from (1) to (10) partition by range (b);
+create table part_gg2_1 partition of part_gg2 for values from (1) to (5);
+create table part_gg2_2 partition of part_gg2 for values from (5) to (10);
+create table part_ee_ff3 partition of part_ee_ff for values from (20) to (30) partition by range (b);
+create table part_ee_ff3_1 partition of part_ee_ff3 for values from (20) to (25);
+create table part_ee_ff3_2 partition of part_ee_ff3 for values from (25) to (30);
+truncate list_parted;
+insert into list_parted values ('aa'), ('cc');
+insert into list_parted select 'Ff', s.a from generate_series(1, 29) s(a);
+insert into list_parted select 'gg', s.a from generate_series(1, 9) s(a);
+insert into list_parted (b) values (1);
+select tableoid::regclass::text, a, min(b) as min_b, max(b) as max_b from list_parted group by 1, 2 order by 1;
+ tableoid | a | min_b | max_b
+---------------+----+-------+-------
+ part_aa_bb | aa | |
+ part_cc_dd | cc | |
+ part_ee_ff1 | Ff | 1 | 9
+ part_ee_ff2 | Ff | 10 | 19
+ part_ee_ff3_1 | Ff | 20 | 24
+ part_ee_ff3_2 | Ff | 25 | 29
+ part_gg2_1 | gg | 1 | 4
+ part_gg2_2 | gg | 5 | 9
+ part_null | | 1 | 1
+(9 rows)
+
-- cleanup
drop table range_parted cascade;
NOTICE: drop cascades to 4 other objects
@@ -293,13 +321,21 @@ drop cascades to table part2
drop cascades to table part3
drop cascades to table part4
drop table list_parted cascade;
-NOTICE: drop cascades to 6 other objects
+NOTICE: drop cascades to 14 other objects
DETAIL: drop cascades to table part_aa_bb
drop cascades to table part_cc_dd
drop cascades to table part_null
drop cascades to table part_ee_ff
drop cascades to table part_ee_ff1
drop cascades to table part_ee_ff2
+drop cascades to table part_ee_ff3
+drop cascades to table part_ee_ff3_1
+drop cascades to table part_ee_ff3_2
+drop cascades to table part_gg
+drop cascades to table part_gg1
+drop cascades to table part_gg2
+drop cascades to table part_gg2_1
+drop cascades to table part_gg2_2
-- more tests for certain multi-level partitioning scenarios
create table p (a int, b int) partition by range (a, b);
create table p1 (b int, a int not null) partition by range (b);
diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql
index 171196df6d..dca89b9286 100644
--- a/src/test/regress/sql/insert.sql
+++ b/src/test/regress/sql/insert.sql
@@ -167,6 +167,24 @@ insert into list_parted values ('EE', 1);
insert into part_ee_ff values ('EE', 10);
select tableoid::regclass, * from list_parted;
+-- some more tests to exercise tuple-routing with multi-level partitioning
+create table part_gg partition of list_parted for values in ('gg') partition by range (b);
+create table part_gg1 partition of part_gg for values from (unbounded) to (1);
+create table part_gg2 partition of part_gg for values from (1) to (10) partition by range (b);
+create table part_gg2_1 partition of part_gg2 for values from (1) to (5);
+create table part_gg2_2 partition of part_gg2 for values from (5) to (10);
+
+create table part_ee_ff3 partition of part_ee_ff for values from (20) to (30) partition by range (b);
+create table part_ee_ff3_1 partition of part_ee_ff3 for values from (20) to (25);
+create table part_ee_ff3_2 partition of part_ee_ff3 for values from (25) to (30);
+
+truncate list_parted;
+insert into list_parted values ('aa'), ('cc');
+insert into list_parted select 'Ff', s.a from generate_series(1, 29) s(a);
+insert into list_parted select 'gg', s.a from generate_series(1, 9) s(a);
+insert into list_parted (b) values (1);
+select tableoid::regclass::text, a, min(b) as min_b, max(b) as max_b from list_parted group by 1, 2 order by 1;
+
-- cleanup
drop table range_parted cascade;
drop table list_parted cascade;
--
2.11.0
>From ca228186ae6943099ef2f421d90a9097a2356357 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Mon, 26 Dec 2016 17:44:14 +0900
Subject: [PATCH 6/7] Avoid tuple coversion in common partitioning cases
Currently, the tuple conversion is performed after a tuple is routed,
even if the attributes of a target leaf partition map one-to-one with
those of the root table, which is wasteful. Avoid that by making
convert_tuples_by_name() return a NULL map for such cases.
Reported by: n/a
Patch by: Amit Langote
Reports: n/a
---
src/backend/access/common/tupconvert.c | 8 ++++++--
src/backend/catalog/partition.c | 5 ++---
src/backend/commands/analyze.c | 1 +
src/backend/executor/execMain.c | 1 +
src/backend/executor/execQual.c | 2 +-
src/include/access/tupconvert.h | 1 +
6 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/src/backend/access/common/tupconvert.c b/src/backend/access/common/tupconvert.c
index 4787d4ca98..9cda7cbd15 100644
--- a/src/backend/access/common/tupconvert.c
+++ b/src/backend/access/common/tupconvert.c
@@ -202,6 +202,7 @@ convert_tuples_by_position(TupleDesc indesc,
TupleConversionMap *
convert_tuples_by_name(TupleDesc indesc,
TupleDesc outdesc,
+ bool consider_typeid,
const char *msg)
{
TupleConversionMap *map;
@@ -259,11 +260,14 @@ convert_tuples_by_name(TupleDesc indesc,
/*
* Check to see if the map is one-to-one and the tuple types are the same.
* (We check the latter because if they're not, we want to do conversion
- * to inject the right OID into the tuple datum.)
+ * to inject the right OID into the tuple datum. In the partitioning
+ * case (!consider_typeid), tdhasoids must always match between indesc
+ * and outdesc, so we need not require tdtypeid's to be the same.)
*/
if (indesc->natts == outdesc->natts &&
- indesc->tdtypeid == outdesc->tdtypeid)
+ (!consider_typeid || indesc->tdtypeid == outdesc->tdtypeid))
{
+ Assert(!consider_typeid && indesc->tdhasoid == outdesc->tdhasoid);
same = true;
for (i = 0; i < n; i++)
{
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 34ab812b44..23eaaf062f 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1052,7 +1052,7 @@ RelationGetPartitionDispatchInfo(Relation rel, int lockmode,
*/
pd[i]->tupslot = MakeSingleTupleTableSlot(tupdesc);
pd[i]->tupmap = convert_tuples_by_name(RelationGetDescr(parent),
- tupdesc,
+ tupdesc, false,
gettext_noop("could not convert row type"));
}
else
@@ -1664,12 +1664,11 @@ get_partition_for_tuple(PartitionDispatch *pd,
return -1;
}
- if (myslot != NULL)
+ if (myslot != NULL && map != NULL)
{
HeapTuple tuple = ExecFetchSlotTuple(slot);
ExecClearTuple(myslot);
- Assert(map != NULL);
tuple = do_convert_tuple(tuple, map);
ExecStoreTuple(tuple, myslot, InvalidBuffer, true);
slot = myslot;
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index f4afcd9aae..3042406b8a 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -1419,6 +1419,7 @@ acquire_inherited_sample_rows(Relation onerel, int elevel,
map = convert_tuples_by_name(RelationGetDescr(childrel),
RelationGetDescr(onerel),
+ true,
gettext_noop("could not convert row type"));
if (map != NULL)
{
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index c991a18ce8..dfa8bdff92 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -3152,6 +3152,7 @@ ExecSetupPartitionTupleRouting(Relation rel,
* partition from the parent's type to the partition's.
*/
(*tup_conv_maps)[i] = convert_tuples_by_name(tupDesc, part_tupdesc,
+ false,
gettext_noop("could not convert row type"));
InitResultRelInfo(leaf_part_rri,
diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index ec1ca01c5a..65d72d5fa6 100644
--- a/src/backend/executor/execQual.c
+++ b/src/backend/executor/execQual.c
@@ -2928,7 +2928,7 @@ ExecEvalConvertRowtype(ConvertRowtypeExprState *cstate,
/* prepare map from old to new attribute numbers */
cstate->map = convert_tuples_by_name(cstate->indesc,
- cstate->outdesc,
+ cstate->outdesc, true,
gettext_noop("could not convert row type"));
cstate->initialized = true;
diff --git a/src/include/access/tupconvert.h b/src/include/access/tupconvert.h
index 10556eec7e..2478e98160 100644
--- a/src/include/access/tupconvert.h
+++ b/src/include/access/tupconvert.h
@@ -36,6 +36,7 @@ extern TupleConversionMap *convert_tuples_by_position(TupleDesc indesc,
extern TupleConversionMap *convert_tuples_by_name(TupleDesc indesc,
TupleDesc outdesc,
+ bool consider_typeid,
const char *msg);
extern HeapTuple do_convert_tuple(HeapTuple tuple, TupleConversionMap *map);
--
2.11.0
>From d36a4803e1a3c79f208906c8826aa679a5c6f0cf Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Mon, 26 Dec 2016 16:49:06 +0900
Subject: [PATCH 7/7] Fix RETURNING to work correctly after tuple-routing
Restore the original tuple slot to be used by ExecProcessReturning().
Also, set the original slot's tuple's t_tableOid so that requesting it
in the RETURNING list works (instead of retuning 0 for the same as
happens now).
Reported by: n/a
Patch by: Amit Langote
Reports: n/a
---
src/backend/executor/nodeModifyTable.c | 7 +++++++
src/test/regress/expected/insert.out | 7 +++++++
src/test/regress/sql/insert.sql | 3 +++
3 files changed, 17 insertions(+)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 825a15f42d..97f03737f5 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -274,6 +274,7 @@ ExecInsert(ModifyTableState *mtstate,
* get information on the (current) result relation
*/
resultRelInfo = estate->es_result_relation_info;
+ tuple->t_tableOid = RelationGetRelid(resultRelInfo->ri_RelationDesc);
/* Determine the partition to heap_insert the tuple into */
if (mtstate->mt_partition_dispatch_info)
@@ -578,6 +579,12 @@ ExecInsert(ModifyTableState *mtstate,
{
resultRelInfo = saved_resultRelInfo;
estate->es_result_relation_info = resultRelInfo;
+
+ /*
+ * Switch back to the original slot for the following steps to work
+ * sanely.
+ */
+ slot = oldslot;
}
/*
diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out
index ed0513d2ff..3fa9315826 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -381,6 +381,13 @@ DETAIL: Failing row contains (1, 2).
insert into p1 (a, b) values (2, 3);
ERROR: new row for relation "p11" violates partition constraint
DETAIL: Failing row contains (3, 2).
+-- check that RETURNING works correctly
+insert into p values (1, 3) returning tableoid::regclass, *;
+ tableoid | a | b
+----------+---+---
+ p | 1 | 3
+(1 row)
+
-- cleanup
drop table p cascade;
NOTICE: drop cascades to 2 other objects
diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql
index dca89b9286..6831bfe0d5 100644
--- a/src/test/regress/sql/insert.sql
+++ b/src/test/regress/sql/insert.sql
@@ -223,5 +223,8 @@ insert into p values (1, 2);
-- selected by tuple-routing
insert into p1 (a, b) values (2, 3);
+-- check that RETURNING works correctly
+insert into p values (1, 3) returning tableoid::regclass, *;
+
-- cleanup
drop table p cascade;
--
2.11.0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
