On 2016/12/26 19:46, Amit Langote wrote:
> (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.
Turns out getting the 2nd result may not require planner tweaks after all.
Unless I'm missing something, translation of varattnos of the RETURNING
target list can be done as late as ExecInitModifyTable() for the insert
case, unlike update/delete (which do require planner's attention).
I updated the patch 0007 to implement the same, including the test. While
doing that, I realized map_partition_varattnos introduced in 0003 is
rather restrictive in its applicability, because it assumes varno = 1 for
the expressions it accepts as input for the mapping. Mapping returning
(target) list required modifying map_partition_varattnos to accept
target_varno as additional argument. That way, we can map arbitrary
expressions from the parent attributes numbers to partition attribute
numbers for expressions not limited to partition constraints.
Patches 0001 to 0006 unchanged.
Thanks,
Amit
>From fcfe08948d31802547e93ac6551873afd554bc36 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 0c46b6fcfc2e1d28a6fc4967befb5437214c658a 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 346bde316b558e87f08d74ac118a57aa676d1059 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 8d3a6e9474c9209ad8985998080f3d9ce3bd50b7 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 b9353c9b7c548cc21ec1860940ed0c6ecd919485 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 9614d04bb9505ee4c4c94fd8899b8c3578a4dbd7 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 98f3be1315ca534c844287695d94efa990be11f1 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Tue, 27 Dec 2016 16:56:58 +0900
Subject: [PATCH 7/7] Fix RETURNING to work correctly after tuple-routing
In ExecInsert(), do not switch back to the original resultRelInfo until
after we finish ExecProcessReturning(), so that RETURNING projection
is done considering the partition the tuple was routed to. For the
projection to work correctly, we must initialize the same for each
leaf partition during ModifyTableState initialization.
With this commit, map_partition_varattnos() now accepts one more argument
viz. target_varno. Previously, it assumed varno = 1 for its input
expressions, which limited its applicability. It was enought so far
since its usage was limited to partition constraints. To use it with
expressions such as an INSERT's returning list, we must be prepared for
varnos != 1 as in the change above.
Reported by: n/a
Patch by: Amit Langote
Reports: n/a
---
src/backend/catalog/partition.c | 13 +++++-----
src/backend/commands/tablecmds.c | 1 +
src/backend/executor/execMain.c | 4 +--
src/backend/executor/nodeModifyTable.c | 46 +++++++++++++++++++++++++++-------
src/include/catalog/partition.h | 3 ++-
src/test/regress/expected/insert.out | 28 ++++++++++++++++++++-
src/test/regress/sql/insert.sql | 14 +++++++++++
7 files changed, 90 insertions(+), 19 deletions(-)
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 23eaaf062f..bbc010105e 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -881,7 +881,8 @@ get_qual_from_partbound(Relation rel, Relation parent, Node *bound)
* different from the parent's.
*/
List *
-map_partition_varattnos(List *expr, Relation partrel, Relation parent)
+map_partition_varattnos(List *expr, int target_varno,
+ Relation partrel, Relation parent)
{
TupleDesc tupdesc = RelationGetDescr(parent);
AttrNumber attno;
@@ -906,7 +907,7 @@ map_partition_varattnos(List *expr, Relation partrel, Relation parent)
}
expr = (List *) map_variable_attnos((Node *) expr,
- 1, 0,
+ target_varno, 0,
part_attnos,
tupdesc->natts,
&found_whole_row);
@@ -1517,8 +1518,8 @@ generate_partition_qual(Relation rel)
result = list_concat(generate_partition_qual(parent),
copyObject(rel->rd_partcheck));
- /* Mark Vars with correct attnos */
- result = map_partition_varattnos(result, rel, parent);
+ /* Mark Vars with correct attnos (dummy varno = 1) */
+ result = map_partition_varattnos(result, 1, rel, parent);
/* Keep the parent locked until commit */
heap_close(parent, NoLock);
@@ -1544,8 +1545,8 @@ generate_partition_qual(Relation rel)
else
result = my_qual;
- /* Mark Vars with correct attnos */
- result = map_partition_varattnos(result, rel, parent);
+ /* Mark Vars with correct attnos (dummy varno = 1) */
+ result = map_partition_varattnos(result, 1, rel, parent);
/* Save a copy of *only* this rel's partition qual in the relcache */
oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 2e51124eb3..caf6b36255 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13349,6 +13349,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
constr = linitial(partConstraint);
my_constr = make_ands_implicit((Expr *) constr);
tab->partition_constraint = map_partition_varattnos(my_constr,
+ 1,
part_rel,
rel);
/* keep our lock until commit */
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index dfa8bdff92..45940706d7 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1279,10 +1279,10 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
/*
* 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.
+ * switch them to our own. (dummy varno = 1)
*/
if (partition_check != NIL)
- partition_check = map_partition_varattnos(partition_check,
+ partition_check = map_partition_varattnos(partition_check, 1,
resultRelationDesc,
partition_root);
}
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 825a15f42d..95262114f1 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -262,7 +262,8 @@ ExecInsert(ModifyTableState *mtstate,
Relation resultRelationDesc;
Oid newId;
List *recheckIndexes = NIL;
- TupleTableSlot *oldslot = slot;
+ TupleTableSlot *oldslot = slot,
+ *result = NULL;
/*
* get the heap tuple out of the tuple table slot, making sure we have a
@@ -574,12 +575,6 @@ ExecInsert(ModifyTableState *mtstate,
list_free(recheckIndexes);
- if (saved_resultRelInfo)
- {
- resultRelInfo = saved_resultRelInfo;
- estate->es_result_relation_info = resultRelInfo;
- }
-
/*
* Check any WITH CHECK OPTION constraints from parent views. We are
* required to do this after testing all constraints and uniqueness
@@ -597,9 +592,15 @@ ExecInsert(ModifyTableState *mtstate,
/* Process RETURNING if present */
if (resultRelInfo->ri_projectReturning)
- return ExecProcessReturning(resultRelInfo, slot, planSlot);
+ result = ExecProcessReturning(resultRelInfo, slot, planSlot);
- return NULL;
+ if (saved_resultRelInfo)
+ {
+ resultRelInfo = saved_resultRelInfo;
+ estate->es_result_relation_info = resultRelInfo;
+ }
+
+ return result;
}
/* ----------------------------------------------------------------
@@ -1786,6 +1787,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
{
TupleTableSlot *slot;
ExprContext *econtext;
+ List *returningList;
/*
* Initialize result tuple slot and assign its rowtype using the first
@@ -1818,6 +1820,32 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
resultRelInfo->ri_RelationDesc->rd_att);
resultRelInfo++;
}
+
+ /*
+ * Build a projection for each leaf partition rel. Note that we
+ * didn't build the returningList for each partition within the
+ * planner, but simple translation of the varattnos for each
+ * partition will suffice. This only occurs for the INSERT case;
+ * UPDATE/DELETE are handled above.
+ */
+ resultRelInfo = mtstate->mt_partitions;
+ returningList = linitial(node->returningLists);
+ for (i = 0; i < mtstate->mt_num_partitions; i++)
+ {
+ Relation partrel = resultRelInfo->ri_RelationDesc;
+ List *rlist,
+ *rliststate;
+
+ /* varno = node->nominalRelation */
+ rlist = map_partition_varattnos(returningList,
+ node->nominalRelation,
+ partrel, rel);
+ rliststate = (List *) ExecInitExpr((Expr *) rlist, &mtstate->ps);
+ resultRelInfo->ri_projectReturning =
+ ExecBuildProjectionInfo(rliststate, econtext, slot,
+ resultRelInfo->ri_RelationDesc->rd_att);
+ resultRelInfo++;
+ }
}
else
{
diff --git a/src/include/catalog/partition.h b/src/include/catalog/partition.h
index 78220d6ac6..ba334328f3 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 *map_partition_varattnos(List *expr, Relation partrel, Relation parent);
+extern List *map_partition_varattnos(List *expr, int target_varno,
+ Relation partrel, Relation parent);
extern List *RelationGetPartitionQual(Relation rel);
/* For tuple routing */
diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out
index ed0513d2ff..c8c47ba829 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -381,8 +381,34 @@ 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 with tuple-routing
+alter table p drop constraint check_b;
+create table p12 partition of p1 for values from (5) to (10);
+create table p2 (b int not null, a int not null);
+alter table p attach partition p2 for values from (1, 10) to (1, 20);
+create table p3 partition of p for values from (1, 20) to (1, 30);
+create table p4 (like p);
+alter table p4 drop a;
+alter table p4 add a int not null;
+alter table p attach partition p4 for values from (1, 30) to (1, 40);
+with ins (a, b, c) as
+ (insert into p (b, a) select s.a, 1 from generate_series(2, 39) s(a) returning tableoid::regclass, *)
+ select a, b, min(c), max(c) from ins group by a, b order by 1;
+ a | b | min | max
+-----+---+-----+-----
+ p11 | 1 | 2 | 4
+ p12 | 1 | 5 | 9
+ p2 | 1 | 10 | 19
+ p3 | 1 | 20 | 29
+ p4 | 1 | 30 | 39
+(5 rows)
+
-- cleanup
drop table p cascade;
-NOTICE: drop cascades to 2 other objects
+NOTICE: drop cascades to 6 other objects
DETAIL: drop cascades to table p1
drop cascades to table p11
+drop cascades to table p12
+drop cascades to table p2
+drop cascades to table p3
+drop cascades to table p4
diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql
index dca89b9286..2bf8cc1659 100644
--- a/src/test/regress/sql/insert.sql
+++ b/src/test/regress/sql/insert.sql
@@ -223,5 +223,19 @@ insert into p values (1, 2);
-- selected by tuple-routing
insert into p1 (a, b) values (2, 3);
+-- check that RETURNING works correctly with tuple-routing
+alter table p drop constraint check_b;
+create table p12 partition of p1 for values from (5) to (10);
+create table p2 (b int not null, a int not null);
+alter table p attach partition p2 for values from (1, 10) to (1, 20);
+create table p3 partition of p for values from (1, 20) to (1, 30);
+create table p4 (like p);
+alter table p4 drop a;
+alter table p4 add a int not null;
+alter table p attach partition p4 for values from (1, 30) to (1, 40);
+with ins (a, b, c) as
+ (insert into p (b, a) select s.a, 1 from generate_series(2, 39) s(a) returning tableoid::regclass, *)
+ select a, b, min(c), max(c) from ins group by a, b order by 1;
+
-- 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
