On 2016/12/22 1:50, Robert Haas wrote:
> On Wed, Dec 21, 2016 at 5:33 AM, Amit Langote
> <langote_amit...@lab.ntt.co.jp> wrote:
>> Breaking changes into multiple commits/patches does not seem to work for
>> adding regression tests. So, I've combined multiple patches into a single
>> patch which is now patch 0002 in the attached set of patches.
>
> Ugh, seriously? It's fine to combine closely related bug fixes but
> not all of these are. I don't see why you can't add some regression
> tests in one patch and then add some more in the next patch.
I managed to do that this time around with the attached set of patches.
Guess I gave up too easily in the previous attempt.
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.
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.
Patch 0005 is the same old "Add some more tests for tuple-routing" per [1]:
> Meanwhile, committed the latest 0001 and the elog() patch.
Thanks!
Regards,
Amit
[1]
https://www.postgresql.org/message-id/CA%2BTgmoZ86v1G%2Bzx9etMiSQaBBvYMKfU-iitqZArSh5z0n8Q4cA%40mail.gmail.com
>From cac625b22990c12a537eaa4c77434017a2303b92 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Thu, 22 Dec 2016 15:33:41 +0900
Subject: [PATCH 1/5] Use correct tuple descriptor before and after routing a
tuple
When we have a multi-level partitioned hierarchy where tables at
successive levels have different attnums for a partition key attribute(s),
we must use the tuple descriptor of a partitioned table of the given level
to inspect the appropriately converted representation of the input tuple
before computing the partition key to perform tuple-routing.
So, store in each PartitionDispatch object, a standalone TupleTableSlot
initialized with the TupleDesc of the corresponding partitioned table,
along with a TupleConversionMap to map tuples from the its parent's
rowtype to own rowtype.
After the routing is done and a leaf partition returned, we must use the
leaf partition's tuple descriptor, not the root table's. For roughly
same reasons as above. For the ext row and so on, we must then switch
back to the root table's tuple descriptor. To that end, a dedicated
TupleTableSlot is allocated in EState called es_partition_tuple_slot,
whose descriptor is set to a given leaf partition for every input tuple
after it's routed.
Reported by: n/a
Patch by: Amit Langote
Reports: n/a
---
src/backend/catalog/partition.c | 74 ++++++++++++++++++++++++++++------
src/backend/commands/copy.c | 31 +++++++++++++-
src/backend/executor/nodeModifyTable.c | 27 +++++++++++++
src/include/catalog/partition.h | 7 ++++
src/include/nodes/execnodes.h | 3 ++
src/test/regress/expected/insert.out | 37 +++++++++++++++++
src/test/regress/sql/insert.sql | 26 ++++++++++++
7 files changed, 190 insertions(+), 15 deletions(-)
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 9980582b77..fca874752f 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -923,13 +923,19 @@ RelationGetPartitionQual(Relation rel, bool recurse)
return generate_partition_qual(rel, recurse);
}
-/* Turn an array of OIDs with N elements into a list */
-#define OID_ARRAY_TO_LIST(arr, N, list) \
+/*
+ * Append OIDs of rel's partitions to the list 'partoids' and for each OID,
+ * append pointer rel to the list 'parents'.
+ */
+#define APPEND_REL_PARTITION_OIDS(rel, partoids, parents) \
do\
{\
int i;\
- for (i = 0; i < (N); i++)\
- (list) = lappend_oid((list), (arr)[i]);\
+ for (i = 0; i < (rel)->rd_partdesc->nparts; i++)\
+ {\
+ (partoids) = lappend_oid((partoids), (rel)->rd_partdesc->oids[i]);\
+ (parents) = lappend((parents), (rel));\
+ }\
} while(0)
/*
@@ -944,11 +950,13 @@ PartitionDispatch *
RelationGetPartitionDispatchInfo(Relation rel, int lockmode,
int *num_parted, List **leaf_part_oids)
{
- PartitionDesc rootpartdesc = RelationGetPartitionDesc(rel);
PartitionDispatchData **pd;
List *all_parts = NIL,
- *parted_rels;
- ListCell *lc;
+ *all_parents = NIL,
+ *parted_rels,
+ *parted_rel_parents;
+ ListCell *lc1,
+ *lc2;
int i,
k,
offset;
@@ -965,10 +973,13 @@ RelationGetPartitionDispatchInfo(Relation rel, int lockmode,
*/
*num_parted = 1;
parted_rels = list_make1(rel);
- OID_ARRAY_TO_LIST(rootpartdesc->oids, rootpartdesc->nparts, all_parts);
- foreach(lc, all_parts)
+ /* Root partitioned table has no parent, so NULL for parent */
+ parted_rel_parents = list_make1(NULL);
+ APPEND_REL_PARTITION_OIDS(rel, all_parts, all_parents);
+ forboth(lc1, all_parts, lc2, all_parents)
{
- Relation partrel = heap_open(lfirst_oid(lc), lockmode);
+ Relation partrel = heap_open(lfirst_oid(lc1), lockmode);
+ Relation parent = lfirst(lc2);
PartitionDesc partdesc = RelationGetPartitionDesc(partrel);
/*
@@ -979,7 +990,8 @@ RelationGetPartitionDispatchInfo(Relation rel, int lockmode,
{
(*num_parted)++;
parted_rels = lappend(parted_rels, partrel);
- OID_ARRAY_TO_LIST(partdesc->oids, partdesc->nparts, all_parts);
+ parted_rel_parents = lappend(parted_rel_parents, parent);
+ APPEND_REL_PARTITION_OIDS(partrel, all_parts, all_parents);
}
else
heap_close(partrel, NoLock);
@@ -1004,10 +1016,12 @@ RelationGetPartitionDispatchInfo(Relation rel, int lockmode,
sizeof(PartitionDispatchData *));
*leaf_part_oids = NIL;
i = k = offset = 0;
- foreach(lc, parted_rels)
+ forboth(lc1, parted_rels, lc2, parted_rel_parents)
{
- Relation partrel = lfirst(lc);
+ Relation partrel = lfirst(lc1);
+ Relation parent = lfirst(lc2);
PartitionKey partkey = RelationGetPartitionKey(partrel);
+ TupleDesc tupdesc = RelationGetDescr(partrel);
PartitionDesc partdesc = RelationGetPartitionDesc(partrel);
int j,
m;
@@ -1017,6 +1031,27 @@ RelationGetPartitionDispatchInfo(Relation rel, int lockmode,
pd[i]->key = partkey;
pd[i]->keystate = NIL;
pd[i]->partdesc = partdesc;
+ if (parent != NULL)
+ {
+ /*
+ * For every partitioned table other than root, we must store
+ * a tuple table slot initialized with its tuple descriptor and
+ * a tuple conversion map to convert a tuple from its parent's
+ * rowtype to its own. That is to make sure that we are looking
+ * at the correct row using the correct tuple descriptor when
+ * computing its partition key for tuple routing.
+ */
+ pd[i]->tupslot = MakeSingleTupleTableSlot(tupdesc);
+ pd[i]->tupmap = convert_tuples_by_name(RelationGetDescr(parent),
+ tupdesc,
+ gettext_noop("could not convert row type"));
+ }
+ else
+ {
+ /* Not required for the root partitioned table */
+ pd[i]->tupslot = NULL;
+ pd[i]->tupmap = NULL;
+ }
pd[i]->indexes = (int *) palloc(partdesc->nparts * sizeof(int));
/*
@@ -1610,6 +1645,8 @@ get_partition_for_tuple(PartitionDispatch *pd,
{
PartitionKey key = parent->key;
PartitionDesc partdesc = parent->partdesc;
+ TupleTableSlot *myslot = parent->tupslot;
+ TupleConversionMap *map = parent->tupmap;
/* Quick exit */
if (partdesc->nparts == 0)
@@ -1618,6 +1655,17 @@ get_partition_for_tuple(PartitionDispatch *pd,
return -1;
}
+ if (myslot != NULL)
+ {
+ HeapTuple tuple = ExecFetchSlotTuple(slot);
+
+ ExecClearTuple(myslot);
+ Assert(map != NULL);
+ tuple = do_convert_tuple(tuple, map);
+ ExecStoreTuple(tuple, myslot, InvalidBuffer, true);
+ slot = myslot;
+ }
+
/* Extract partition key from tuple */
FormPartitionKeyDatum(parent, slot, estate, values, isnull);
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index d5901651db..aa25a23336 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2436,6 +2436,15 @@ 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
@@ -2484,7 +2493,8 @@ CopyFrom(CopyState cstate)
for (;;)
{
- TupleTableSlot *slot;
+ TupleTableSlot *slot,
+ *oldslot = NULL;
bool skip_tuple;
Oid loaded_oid = InvalidOid;
@@ -2571,7 +2581,19 @@ CopyFrom(CopyState cstate)
map = cstate->partition_tupconv_maps[leaf_part_index];
if (map)
{
+ Relation partrel = resultRelInfo->ri_RelationDesc;
+
tuple = do_convert_tuple(tuple, map);
+
+ /*
+ * We must use the partition's tuple descriptor from this
+ * point on. Use a dedicated slot from this point on until
+ * we're finished dealing with the partition.
+ */
+ oldslot = slot;
+ slot = estate->es_partition_tuple_slot;
+ Assert(slot != NULL);
+ ExecSetSlotDescriptor(slot, RelationGetDescr(partrel));
ExecStoreTuple(tuple, slot, InvalidBuffer, true);
}
@@ -2667,6 +2689,10 @@ 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;
}
}
}
@@ -2714,13 +2740,14 @@ CopyFrom(CopyState cstate)
* Remember cstate->partition_dispatch_info[0] corresponds to the root
* partitioned table, which we must not try to close, because it is
* the main target table of COPY that will be closed eventually by
- * DoCopy().
+ * DoCopy(). Also, tupslot is NULL for the root partitioned table.
*/
for (i = 1; i < cstate->num_dispatch; i++)
{
PartitionDispatch pd = cstate->partition_dispatch_info[i];
heap_close(pd->reldesc, NoLock);
+ ExecDropSingleTupleTableSlot(pd->tupslot);
}
for (i = 0; i < cstate->num_partitions; i++)
{
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index a9546106ce..0d85b151c2 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -262,6 +262,7 @@ ExecInsert(ModifyTableState *mtstate,
Relation resultRelationDesc;
Oid newId;
List *recheckIndexes = NIL;
+ TupleTableSlot *oldslot = NULL;
/*
* get the heap tuple out of the tuple table slot, making sure we have a
@@ -318,7 +319,19 @@ ExecInsert(ModifyTableState *mtstate,
map = mtstate->mt_partition_tupconv_maps[leaf_part_index];
if (map)
{
+ Relation partrel = resultRelInfo->ri_RelationDesc;
+
tuple = do_convert_tuple(tuple, map);
+
+ /*
+ * We must use the partition's tuple descriptor from this
+ * point on, until we're finished dealing with the partition.
+ * Use the dedicated slot for that.
+ */
+ oldslot = slot;
+ slot = estate->es_partition_tuple_slot;
+ Assert(slot != NULL);
+ ExecSetSlotDescriptor(slot, RelationGetDescr(partrel));
ExecStoreTuple(tuple, slot, InvalidBuffer, true);
}
}
@@ -566,6 +579,10 @@ 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;
}
/*
@@ -1734,7 +1751,15 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
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);
}
+ else
+ estate->es_partition_tuple_slot = NULL;
/*
* Initialize any WITH CHECK OPTION constraints if needed.
@@ -2058,12 +2083,14 @@ ExecEndModifyTable(ModifyTableState *node)
* Remember node->mt_partition_dispatch_info[0] corresponds to the root
* partitioned table, which we must not try to close, because it is the
* main target table of the query that will be closed by ExecEndPlan().
+ * Also, tupslot is NULL for the root partitioned table.
*/
for (i = 1; i < node->mt_num_dispatch; i++)
{
PartitionDispatch pd = node->mt_partition_dispatch_info[i];
heap_close(pd->reldesc, NoLock);
+ ExecDropSingleTupleTableSlot(pd->tupslot);
}
for (i = 0; i < node->mt_num_partitions; i++)
{
diff --git a/src/include/catalog/partition.h b/src/include/catalog/partition.h
index 21effbf87b..bf38df5d29 100644
--- a/src/include/catalog/partition.h
+++ b/src/include/catalog/partition.h
@@ -47,6 +47,11 @@ typedef struct PartitionDescData *PartitionDesc;
* key Partition key information of the table
* keystate Execution state required for expressions in the partition key
* partdesc Partition descriptor of the table
+ * tupslot A standalone TupleTableSlot initialized with this table's tuple
+ * descriptor
+ * tupmap TupleConversionMap to convert from the parent's rowtype to
+ * this table's rowtype (when extracting the partition key of a
+ * tuple just before routing it through this table)
* indexes Array with partdesc->nparts members (for details on what
* individual members represent, see how they are set in
* RelationGetPartitionDispatchInfo())
@@ -58,6 +63,8 @@ typedef struct PartitionDispatchData
PartitionKey key;
List *keystate; /* list of ExprState */
PartitionDesc partdesc;
+ TupleTableSlot *tupslot;
+ TupleConversionMap *tupmap;
int *indexes;
} PartitionDispatchData;
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 5c3b8683f5..9073a9a1bc 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -384,6 +384,9 @@ 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 */
diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out
index 561cefa3c4..49f667b119 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -300,3 +300,40 @@ 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
+-- 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);
+alter table p attach partition p1 for values from (1, 2) to (1, 10);
+-- check that "(1, 2)" is correctly routed to p11.
+insert into p values (1, 2);
+select tableoid::regclass, * from p;
+ tableoid | a | b
+----------+---+---
+ p11 | 1 | 2
+(1 row)
+
+-- cleanup
+drop table p cascade;
+NOTICE: drop cascades to 2 other objects
+DETAIL: drop cascades to table p1
+drop cascades to table p11
diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql
index 846bb5897a..08dc068de8 100644
--- a/src/test/regress/sql/insert.sql
+++ b/src/test/regress/sql/insert.sql
@@ -170,3 +170,29 @@ select tableoid::regclass, * from list_parted;
-- cleanup
drop table range_parted cascade;
drop table list_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);
+alter table p attach partition p1 for values from (1, 2) to (1, 10);
+
+-- check that "(1, 2)" is correctly routed to p11.
+insert into p values (1, 2);
+select tableoid::regclass, * from p;
+
+-- cleanup
+drop table p cascade;
--
2.11.0
>From c6df09f19533d2ee22882138eafa26c2fc82e261 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Thu, 15 Dec 2016 18:00:47 +0900
Subject: [PATCH 2/5] 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, convert the erring row back to root table's format before
printing the error message showing the row.
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 | 75 +++++++++++++++++++++++++++++++++---
src/include/executor/executor.h | 1 +
src/include/nodes/execnodes.h | 1 +
src/test/regress/expected/insert.out | 7 ++++
src/test/regress/sql/insert.sql | 6 +++
7 files changed, 87 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index aa25a23336..a20e22daaf 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2420,6 +2420,7 @@ CopyFrom(CopyState cstate)
cstate->rel,
1, /* dummy rangetable index */
true, /* do load partition check expression */
+ NULL,
0);
ExecOpenIndices(resultRelInfo, false);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 115b98313e..338ff08ed4 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 bca34a509c..55febd7bc1 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);
@@ -1767,6 +1775,28 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
slot_attisnull(slot, attrChk))
{
char *val_desc;
+ Relation orig_rel = rel;
+ TupleDesc orig_tupdesc = tupdesc;
+
+ /*
+ * In case where the tuple is routed, it's been converted
+ * to the partition's rowtype, which might differ from the
+ * root table's. We must convert it back to the root table's
+ * type so that it's shown correctly in the error message.
+ */
+ if (resultRelInfo->ri_PartitionRoot)
+ {
+ HeapTuple tuple = ExecFetchSlotTuple(slot);
+ TupleConversionMap *map;
+
+ rel = resultRelInfo->ri_PartitionRoot;
+ tupdesc = RelationGetDescr(rel);
+ /* a reverse map */
+ map = convert_tuples_by_name(orig_tupdesc, tupdesc,
+ gettext_noop("could not convert row type"));
+ tuple = do_convert_tuple(tuple, map);
+ ExecStoreTuple(tuple, slot, InvalidBuffer, false);
+ }
insertedCols = GetInsertedColumns(resultRelInfo, estate);
updatedCols = GetUpdatedColumns(resultRelInfo, estate);
@@ -1780,9 +1810,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,6 +1824,23 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
if ((failed = ExecRelCheck(resultRelInfo, slot, estate)) != NULL)
{
char *val_desc;
+ Relation orig_rel = rel;
+ TupleDesc orig_tupdesc = tupdesc;
+
+ /* See the comment above. */
+ if (resultRelInfo->ri_PartitionRoot)
+ {
+ HeapTuple tuple = ExecFetchSlotTuple(slot);
+ TupleConversionMap *map;
+
+ rel = resultRelInfo->ri_PartitionRoot;
+ tupdesc = RelationGetDescr(rel);
+ /* a reverse map */
+ map = convert_tuples_by_name(orig_tupdesc, tupdesc,
+ gettext_noop("could not convert row type"));
+ tuple = do_convert_tuple(tuple, map);
+ ExecStoreTuple(tuple, slot, InvalidBuffer, false);
+ }
insertedCols = GetInsertedColumns(resultRelInfo, estate);
updatedCols = GetUpdatedColumns(resultRelInfo, estate);
@@ -1806,9 +1853,9 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
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,6 +1863,23 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
!ExecPartitionCheck(resultRelInfo, slot, estate))
{
char *val_desc;
+ Relation orig_rel = rel;
+ TupleDesc orig_tupdesc = tupdesc;
+
+ /* See the comment above. */
+ if (resultRelInfo->ri_PartitionRoot)
+ {
+ HeapTuple tuple = ExecFetchSlotTuple(slot);
+ TupleConversionMap *map;
+
+ rel = resultRelInfo->ri_PartitionRoot;
+ tupdesc = RelationGetDescr(rel);
+ /* a reverse map */
+ map = convert_tuples_by_name(orig_tupdesc, tupdesc,
+ gettext_noop("could not convert row type"));
+ tuple = do_convert_tuple(tuple, map);
+ ExecStoreTuple(tuple, slot, InvalidBuffer, false);
+ }
insertedCols = GetInsertedColumns(resultRelInfo, estate);
updatedCols = GetUpdatedColumns(resultRelInfo, estate);
@@ -1828,7 +1892,7 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
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));
}
}
@@ -3074,6 +3138,7 @@ ExecSetupPartitionTupleRouting(Relation rel,
partrel,
1, /* dummy */
false,
+ rel,
0);
/*
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index b74fa5eb5d..f85b26b00a 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -190,6 +190,7 @@ 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);
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 9073a9a1bc..97b4083f14 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 1df7ca48a471b19aa61f11cb9c2390d6bbbce980 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Thu, 15 Dec 2016 16:27:04 +0900
Subject: [PATCH 3/5] 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 338ff08ed4..3db830f0c7 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13152,7 +13152,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);
@@ -13327,6 +13327,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))
@@ -13349,8 +13350,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 55febd7bc1..6954ba8d32 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 7bcec1a4ddb0b397232033e8faa0533e9bdf26a1 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Tue, 13 Dec 2016 15:07:41 +0900
Subject: [PATCH 4/5] 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 a20e22daaf..2cb89190e7 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2419,7 +2419,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 3db830f0c7..096ca181be 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 6954ba8d32..3e2c1cb790 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 =
@@ -3137,7 +3161,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 f85b26b00a..9c7878f847 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 19a99fb5b550f032bc8586932f67bba024858c5e Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Wed, 21 Dec 2016 10:51:32 +0900
Subject: [PATCH 5/5] 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
