On 2016/12/17 11:32, Amit Langote wrote:
> On Sat, Dec 17, 2016 at 1:07 AM, Robert Haas <robertmh...@gmail.com> wrote:
>> On Fri, Dec 16, 2016 at 3:02 AM, Amit Langote
>> <langote_amit...@lab.ntt.co.jp> wrote:
>>> Aside from the above, I found few other issues and fixed them in the
>>> attached patches. Descriptions follow:
>>
>> To avoid any further mistakes on my part, can you please resubmit
>> these with each patch file containing a proposed commit message
>> including patch authorship information, who reported the issue, links
>> to relevant discussion if any, and any other attribution information
>> which I should not fail to include when committing?
>
> I think it's a good advice and will keep in mind for any patches I
> post henceforth.
>
> In this particular case, I found all the issues myself while working
> with some more esoteric test scenarios, except the first patch (1/7),
> where I have mentioned in the description of the patch in the email,
> that there were independent reports of the issue by Tomas Vondra and
> David Fetter.
Here are updated patches including the additional information.
Thanks,
Amit
>From c2afac3447a8bd2f7f9004e8b7e258039ca2296b Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Tue, 13 Dec 2016 15:07:06 +0900
Subject: [PATCH 1/7] Invalidate the parent's relcache after partition
creation.
CREATE TABLE PARTITION OF failed to invalidate the parent table's
relcache, causing the subsequent commands in the same transaction to
not see the new partition.
Reported by: Tomas Vondra and David Fetter
Patch by: Amit Langote
Reports: https://www.postgresql.org/message-id/22dd313b-d7fd-22b5-0787-654845c8f849%402ndquadrant.com
https://www.postgresql.org/message-id/20161215090916.GB20659%40fetter.org
---
src/backend/catalog/heap.c | 7 ++++++-
src/backend/commands/tablecmds.c | 13 ++++---------
src/include/catalog/heap.h | 2 +-
3 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index c09c9f28a7..e5d6aecc3f 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -3230,9 +3230,12 @@ RemovePartitionKeyByRelId(Oid relid)
* StorePartitionBound
* Update pg_class tuple of rel to store the partition bound and set
* relispartition to true
+ *
+ * Also, invalidate the parent's relcache, so that the next rebuild will load
+ * the new partition's info into its partition descriptor.
*/
void
-StorePartitionBound(Relation rel, Node *bound)
+StorePartitionBound(Relation rel, Relation parent, Node *bound)
{
Relation classRel;
HeapTuple tuple,
@@ -3273,4 +3276,6 @@ StorePartitionBound(Relation rel, Node *bound)
CatalogUpdateIndexes(classRel, newtuple);
heap_freetuple(newtuple);
heap_close(classRel, RowExclusiveLock);
+
+ CacheInvalidateRelcache(parent);
}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7a574dc50d..1c219b03dd 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -777,10 +777,11 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
* it does not return on error.
*/
check_new_partition_bound(relname, parent, bound);
- heap_close(parent, NoLock);
/* Update the pg_class entry. */
- StorePartitionBound(rel, bound);
+ StorePartitionBound(rel, parent, bound);
+
+ heap_close(parent, NoLock);
/*
* The code that follows may also update the pg_class tuple to update
@@ -13141,7 +13142,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
cmd->bound);
/* Update the pg_class entry. */
- StorePartitionBound(attachRel, cmd->bound);
+ StorePartitionBound(attachRel, rel, cmd->bound);
/*
* Generate partition constraint from the partition bound specification.
@@ -13352,12 +13353,6 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
}
}
- /*
- * Invalidate the parent's relcache so that the new partition is now
- * included its partition descriptor.
- */
- CacheInvalidateRelcache(rel);
-
ObjectAddressSet(address, RelationRelationId, RelationGetRelid(attachRel));
/* keep our lock until commit */
diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h
index 77dc1983e8..0e4262f611 100644
--- a/src/include/catalog/heap.h
+++ b/src/include/catalog/heap.h
@@ -143,6 +143,6 @@ extern void StorePartitionKey(Relation rel,
Oid *partopclass,
Oid *partcollation);
extern void RemovePartitionKeyByRelId(Oid relid);
-extern void StorePartitionBound(Relation rel, Node *bound);
+extern void StorePartitionBound(Relation rel, Relation parent, Node *bound);
#endif /* HEAP_H */
--
2.11.0
>From 21d38e05d1107a88178e9954b1f4a3804cd9e154 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Thu, 15 Dec 2016 16:27:04 +0900
Subject: [PATCH 2/7] Change how RelationGetPartitionQual() and related code
works
Since we always want to recurse, ie, include the parent's partition
constraint (if any), get rid of the argument recurse.
Refactor out the code doing the mapping of attnos of Vars in partition
constraint expressions (parent attnos -> child attnos). Move it to a
separate function map_partition_varattnos() and call it from appropriate
places. It previously used to be done in get_qual_from_partbound(),
which would lead to wrong results in certain multi-level partitioning
cases, as the mapping would be done for immediate parent-partition pairs.
Now in generate_partition_qual() which is the workhorse of
RelationGetPartitionQual(), we first generate the full expression
(considering all levels of partitioning) and then do the mapping from the
root parent to a leaf partition.
It is also possible to generate partition constraint up to certain non-leaf
level and then apply the same to leaf partitions of that sub-tree after
suitable substitution of varattnos using the new map_partition_varattnos()
directly. This is what happens some tuple-routing cases, e.g., if the main
target table is some non-root partitioned table in a partition tree. In
such cases, we need to apply the partition constraint of the target table
to the leaf partition into which a given tuple is routed.
Bug fix: ATExecAttachPartition() failed to do the mapping when attaching
a partitioned table as partition. Since it is possible for the partitions
of such table to have different attributes from the table being attached
and/or the target partitioned table, the result of applying the partition
constraint during the validation scan could be incorrect.
Reported by: n/a
Patch by: Amit Langote
Reports: n/a
---
src/backend/catalog/partition.c | 97 ++++++++++++++++++++----------------
src/backend/commands/tablecmds.c | 9 ++--
src/backend/executor/execMain.c | 3 +-
src/backend/optimizer/util/plancat.c | 2 +-
src/include/catalog/partition.h | 3 +-
5 files changed, 63 insertions(+), 51 deletions(-)
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 9980582b77..694cb469e0 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,48 @@ 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;
+
+ 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 +920,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);
}
/* Turn an array of OIDs with N elements into a list */
@@ -1445,7 +1451,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;
@@ -1459,6 +1465,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);
@@ -1466,13 +1476,14 @@ 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;
}
@@ -1492,18 +1503,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* the partition's 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 1c219b03dd..d2e4cfc365 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);
@@ -13323,6 +13323,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))
@@ -13345,8 +13346,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 d43a204808..520fe4e0ce 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1256,8 +1256,7 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
resultRelInfo->ri_projectReturning = NULL;
if (load_partition_check)
resultRelInfo->ri_PartitionCheck =
- RelationGetPartitionQual(resultRelationDesc,
- true);
+ RelationGetPartitionQual(resultRelationDesc);
}
/*
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 21effbf87b..6ff821e6cf 100644
--- a/src/include/catalog/partition.h
+++ b/src/include/catalog/partition.h
@@ -70,7 +70,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,
--
2.11.0
>From fd44997465c1f19d4213126355d9638442cbb38b Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Thu, 15 Dec 2016 15:56:27 +0900
Subject: [PATCH 3/7] Refactor tuple-routing setup code
It's unnecessarily repeated in copy.c and nodeModifyTable.c, which makes
it a burden to maintain. Should've been like this to begin with.
I moved the common code to ExecSetupPartitionTupleRouting() in execMain.c
that also houses ExecFindParttion() currently. Hmm, should there be a
new src/backend/executor/execPartition.c?
Reported by: n/a
Patch by: Amit Langote
Reports: n/a
---
src/backend/commands/copy.c | 72 ++++++----------------------
src/backend/executor/execMain.c | 85 ++++++++++++++++++++++++++++++++++
src/backend/executor/nodeModifyTable.c | 76 ++++++------------------------
src/include/executor/executor.h | 5 ++
4 files changed, 120 insertions(+), 118 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 7a8da338f0..d5901651db 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -1406,64 +1406,22 @@ BeginCopy(ParseState *pstate,
/* Initialize state for CopyFrom tuple routing. */
if (is_from && rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
{
- List *leaf_parts;
- ListCell *cell;
- int i,
- num_parted;
- ResultRelInfo *leaf_part_rri;
-
- /* Get the tuple-routing information and lock partitions */
- cstate->partition_dispatch_info =
- RelationGetPartitionDispatchInfo(rel, RowExclusiveLock,
- &num_parted,
- &leaf_parts);
+ PartitionDispatch *partition_dispatch_info;
+ ResultRelInfo *partitions;
+ TupleConversionMap **partition_tupconv_maps;
+ int num_parted,
+ num_partitions;
+
+ ExecSetupPartitionTupleRouting(rel,
+ &partition_dispatch_info,
+ &partitions,
+ &partition_tupconv_maps,
+ &num_parted, &num_partitions);
+ cstate->partition_dispatch_info = partition_dispatch_info;
cstate->num_dispatch = num_parted;
- cstate->num_partitions = list_length(leaf_parts);
- cstate->partitions = (ResultRelInfo *)
- palloc(cstate->num_partitions *
- sizeof(ResultRelInfo));
- cstate->partition_tupconv_maps = (TupleConversionMap **)
- palloc0(cstate->num_partitions *
- sizeof(TupleConversionMap *));
-
- leaf_part_rri = cstate->partitions;
- i = 0;
- foreach(cell, leaf_parts)
- {
- Relation partrel;
-
- /*
- * We locked all the partitions above including the leaf
- * partitions. Note that each of the relations in
- * cstate->partitions will be closed by CopyFrom() after it's
- * finished with its processing.
- */
- partrel = heap_open(lfirst_oid(cell), NoLock);
-
- /*
- * Verify result relation is a valid target for the current
- * operation.
- */
- CheckValidResultRel(partrel, CMD_INSERT);
-
- InitResultRelInfo(leaf_part_rri,
- partrel,
- 1, /* dummy */
- false, /* no partition constraint
- * check */
- 0);
-
- /* Open partition indices */
- ExecOpenIndices(leaf_part_rri, false);
-
- if (!equalTupleDescs(tupDesc, RelationGetDescr(partrel)))
- cstate->partition_tupconv_maps[i] =
- convert_tuples_by_name(tupDesc,
- RelationGetDescr(partrel),
- gettext_noop("could not convert row type"));
- leaf_part_rri++;
- i++;
- }
+ cstate->partitions = partitions;
+ cstate->num_partitions = num_partitions;
+ cstate->partition_tupconv_maps = partition_tupconv_maps;
}
}
else
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 520fe4e0ce..b3cedadce4 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -51,6 +51,7 @@
#include "miscadmin.h"
#include "optimizer/clauses.h"
#include "parser/parsetree.h"
+#include "rewrite/rewriteManip.h"
#include "storage/bufmgr.h"
#include "storage/lmgr.h"
#include "tcop/utility.h"
@@ -2998,6 +2999,90 @@ EvalPlanQualEnd(EPQState *epqstate)
}
/*
+ * ExecSetupPartitionTupleRouting - set up information needed during
+ * tuple routing for partitioned tables
+ *
+ * Output arguments:
+ * 'pd' receives an array of PartitionDispatch objects with one entry for
+ * every partitioned table in the partition tree
+ * 'partitions' receives an array of ResultRelInfo objects with one entry for
+ * every leaf partition in the partition tree
+ * 'tup_conv_maps' receives an array of TupleConversionMap objects with one
+ * 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
+ * '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
+ * tree (= the number of entries in the 'partitions' and 'tup_conv_maps'
+ * output arrays
+ *
+ * Note that all the relations in the partition tree are locked using the
+ * RowExclusiveLock mode upon return from this function.
+ */
+void
+ExecSetupPartitionTupleRouting(Relation rel,
+ PartitionDispatch **pd,
+ ResultRelInfo **partitions,
+ TupleConversionMap ***tup_conv_maps,
+ int *num_parted, int *num_partitions)
+{
+ TupleDesc tupDesc = RelationGetDescr(rel);
+ List *leaf_parts;
+ ListCell *cell;
+ int i;
+ ResultRelInfo *leaf_part_rri;
+
+ /* Get the tuple-routing information and lock partitions */
+ *pd = RelationGetPartitionDispatchInfo(rel, RowExclusiveLock, num_parted,
+ &leaf_parts);
+ *num_partitions = list_length(leaf_parts);
+ *partitions = (ResultRelInfo *) palloc(*num_partitions *
+ sizeof(ResultRelInfo));
+ *tup_conv_maps = (TupleConversionMap **) palloc0(*num_partitions *
+ sizeof(TupleConversionMap *));
+
+ leaf_part_rri = *partitions;
+ i = 0;
+ foreach(cell, leaf_parts)
+ {
+ Relation partrel;
+ TupleDesc part_tupdesc;
+
+ /*
+ * We locked all the partitions above including the leaf partitions.
+ * Note that each of the relations in *partitions are eventually
+ * closed by the caller.
+ */
+ partrel = heap_open(lfirst_oid(cell), NoLock);
+ part_tupdesc = RelationGetDescr(partrel);
+
+ /*
+ * Verify result relation is a valid target for the current operation.
+ */
+ CheckValidResultRel(partrel, CMD_INSERT);
+
+ /*
+ * Save a tuple conversion map to convert a tuple routed to this
+ * partition from the parent's type to the partition's.
+ */
+ (*tup_conv_maps)[i] = convert_tuples_by_name(tupDesc, part_tupdesc,
+ gettext_noop("could not convert row type"));
+
+ InitResultRelInfo(leaf_part_rri,
+ partrel,
+ 1, /* dummy */
+ false,
+ 0);
+
+ /* Open partition indices */
+ ExecOpenIndices(leaf_part_rri, false);
+ leaf_part_rri++;
+ i++;
+ }
+}
+
+/*
* ExecFindPartition -- Find a leaf partition in the partition tree rooted
* at parent, for the heap tuple contained in *slot
*
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index ec440b353d..a9546106ce 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1718,68 +1718,22 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
if (operation == CMD_INSERT &&
rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
{
- int i,
- j,
- num_parted;
- List *leaf_parts;
- ListCell *cell;
- ResultRelInfo *leaf_part_rri;
-
- /* Get the tuple-routing information and lock partitions */
- mtstate->mt_partition_dispatch_info =
- RelationGetPartitionDispatchInfo(rel, RowExclusiveLock,
- &num_parted,
- &leaf_parts);
+ PartitionDispatch *partition_dispatch_info;
+ ResultRelInfo *partitions;
+ TupleConversionMap **partition_tupconv_maps;
+ int num_parted,
+ num_partitions;
+
+ ExecSetupPartitionTupleRouting(rel,
+ &partition_dispatch_info,
+ &partitions,
+ &partition_tupconv_maps,
+ &num_parted, &num_partitions);
+ mtstate->mt_partition_dispatch_info = partition_dispatch_info;
mtstate->mt_num_dispatch = num_parted;
- mtstate->mt_num_partitions = list_length(leaf_parts);
- mtstate->mt_partitions = (ResultRelInfo *)
- palloc0(mtstate->mt_num_partitions *
- sizeof(ResultRelInfo));
- mtstate->mt_partition_tupconv_maps = (TupleConversionMap **)
- palloc0(mtstate->mt_num_partitions *
- sizeof(TupleConversionMap *));
-
- leaf_part_rri = mtstate->mt_partitions;
- i = j = 0;
- foreach(cell, leaf_parts)
- {
- Oid partrelid = lfirst_oid(cell);
- Relation partrel;
-
- /*
- * We locked all the partitions above including the leaf
- * partitions. Note that each of the relations in
- * mtstate->mt_partitions will be closed by ExecEndModifyTable().
- */
- partrel = heap_open(partrelid, NoLock);
-
- /*
- * Verify result relation is a valid target for the current
- * operation
- */
- CheckValidResultRel(partrel, CMD_INSERT);
-
- InitResultRelInfo(leaf_part_rri,
- partrel,
- 1, /* dummy */
- false, /* no partition constraint checks */
- eflags);
-
- /* Open partition indices (note: ON CONFLICT unsupported)*/
- if (partrel->rd_rel->relhasindex && operation != CMD_DELETE &&
- leaf_part_rri->ri_IndexRelationDescs == NULL)
- ExecOpenIndices(leaf_part_rri, false);
-
- if (!equalTupleDescs(RelationGetDescr(rel),
- RelationGetDescr(partrel)))
- mtstate->mt_partition_tupconv_maps[i] =
- convert_tuples_by_name(RelationGetDescr(rel),
- RelationGetDescr(partrel),
- gettext_noop("could not convert row type"));
-
- leaf_part_rri++;
- i++;
- }
+ mtstate->mt_partitions = partitions;
+ mtstate->mt_num_partitions = num_partitions;
+ mtstate->mt_partition_tupconv_maps = partition_tupconv_maps;
}
/*
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 3f649faf2f..b74fa5eb5d 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -213,6 +213,11 @@ extern void EvalPlanQualSetPlan(EPQState *epqstate,
extern void EvalPlanQualSetTuple(EPQState *epqstate, Index rti,
HeapTuple tuple);
extern HeapTuple EvalPlanQualGetTuple(EPQState *epqstate, Index rti);
+extern void ExecSetupPartitionTupleRouting(Relation rel,
+ PartitionDispatch **pd,
+ ResultRelInfo **partitions,
+ TupleConversionMap ***tup_conv_maps,
+ int *num_parted, int *num_partitions);
extern int ExecFindPartition(ResultRelInfo *resultRelInfo,
PartitionDispatch *pd,
TupleTableSlot *slot,
--
2.11.0
>From 8b88ecbb0805d54aa4f69dd0b7a3ab6cdc31c8ed 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.
InitResultRelInfo()'s API changes with this. Instead of passing
a boolean telling whether or not to load the partition constraint,
callers now need to pass the exact constraint expression to use
as ri_PartitionCheck or NIL.
Reported by: n/a
Patch by: Amit Langote
Reports: n/a
---
src/backend/commands/copy.c | 7 +++++-
src/backend/commands/tablecmds.c | 2 +-
src/backend/executor/execMain.c | 42 ++++++++++++++++++++++++++++++------
src/include/executor/executor.h | 3 +--
src/test/regress/expected/insert.out | 4 ++++
src/test/regress/sql/insert.sql | 3 +++
6 files changed, 50 insertions(+), 11 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index d5901651db..a0eb4241e2 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2294,6 +2294,7 @@ CopyFrom(CopyState cstate)
uint64 processed = 0;
bool useHeapMultiInsert;
int nBufferedTuples = 0;
+ List *partcheck = NIL;
#define MAX_BUFFERED_TUPLES 1000
HeapTuple *bufferedTuples = NULL; /* initialize to silence warning */
@@ -2410,6 +2411,10 @@ CopyFrom(CopyState cstate)
hi_options |= HEAP_INSERT_FROZEN;
}
+ /* Don't forget the partition constraints */
+ if (cstate->rel->rd_rel->relispartition)
+ partcheck = RelationGetPartitionQual(cstate->rel);
+
/*
* We need a ResultRelInfo so we can use the regular executor's
* index-entry-making machinery. (There used to be a huge amount of code
@@ -2419,7 +2424,7 @@ CopyFrom(CopyState cstate)
InitResultRelInfo(resultRelInfo,
cstate->rel,
1, /* dummy rangetable index */
- true, /* do load partition check expression */
+ partcheck,
0);
ExecOpenIndices(resultRelInfo, false);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d2e4cfc365..4bd4ec4e8c 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1323,7 +1323,7 @@ ExecuteTruncate(TruncateStmt *stmt)
InitResultRelInfo(resultRelInfo,
rel,
0, /* dummy rangetable index */
- false,
+ NIL,
0);
resultRelInfo++;
}
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index b3cedadce4..1b19bc38ef 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -821,13 +821,19 @@ InitPlan(QueryDesc *queryDesc, int eflags)
Index resultRelationIndex = lfirst_int(l);
Oid resultRelationOid;
Relation resultRelation;
+ List *partcheck = NIL;
resultRelationOid = getrelid(resultRelationIndex, rangeTable);
resultRelation = heap_open(resultRelationOid, RowExclusiveLock);
+
+ /* Don't forget the partition constraint */
+ if (resultRelation->rd_rel->relispartition)
+ partcheck = RelationGetPartitionQual(resultRelation);
+
InitResultRelInfo(resultRelInfo,
resultRelation,
resultRelationIndex,
- true,
+ partcheck,
estate->es_instrument);
resultRelInfo++;
}
@@ -1217,7 +1223,7 @@ void
InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
- bool load_partition_check,
+ List *partition_check,
int instrument_options)
{
MemSet(resultRelInfo, 0, sizeof(ResultRelInfo));
@@ -1255,9 +1261,7 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
resultRelInfo->ri_ConstraintExprs = NULL;
resultRelInfo->ri_junkFilter = NULL;
resultRelInfo->ri_projectReturning = NULL;
- if (load_partition_check)
- resultRelInfo->ri_PartitionCheck =
- RelationGetPartitionQual(resultRelationDesc);
+ resultRelInfo->ri_PartitionCheck = partition_check;
}
/*
@@ -1284,6 +1288,7 @@ ExecGetTriggerResultRel(EState *estate, Oid relid)
ListCell *l;
Relation rel;
MemoryContext oldcontext;
+ List *partcheck = NIL;
/* First, search through the query result relations */
rInfo = estate->es_result_relations;
@@ -1312,6 +1317,10 @@ ExecGetTriggerResultRel(EState *estate, Oid relid)
*/
rel = heap_open(relid, NoLock);
+ /* Don't forget the partition constraint */
+ if (rel->rd_rel->relispartition)
+ partcheck = RelationGetPartitionQual(rel);
+
/*
* Make the new entry in the right context.
*/
@@ -1320,7 +1329,7 @@ ExecGetTriggerResultRel(EState *estate, Oid relid)
InitResultRelInfo(rInfo,
rel,
0, /* dummy rangetable index */
- true,
+ partcheck,
estate->es_instrument);
estate->es_trig_target_relations =
lappend(estate->es_trig_target_relations, rInfo);
@@ -3032,6 +3041,7 @@ ExecSetupPartitionTupleRouting(Relation rel,
ListCell *cell;
int i;
ResultRelInfo *leaf_part_rri;
+ List *partcheck = NIL;
/* Get the tuple-routing information and lock partitions */
*pd = RelationGetPartitionDispatchInfo(rel, RowExclusiveLock, num_parted,
@@ -3042,12 +3052,22 @@ ExecSetupPartitionTupleRouting(Relation rel,
*tup_conv_maps = (TupleConversionMap **) palloc0(*num_partitions *
sizeof(TupleConversionMap *));
+ /*
+ * If the main target rel is a partition, ExecConstraints() as applied to
+ * each leaf partition must consider its partition constraint, because
+ * unlike explicit constraints, an implicit partition constraint is not
+ * inherited.
+ */
+ if (rel->rd_rel->relispartition)
+ partcheck = RelationGetPartitionQual(rel);
+
leaf_part_rri = *partitions;
i = 0;
foreach(cell, leaf_parts)
{
Relation partrel;
TupleDesc part_tupdesc;
+ List *my_check = NIL;
/*
* We locked all the partitions above including the leaf partitions.
@@ -3069,10 +3089,18 @@ ExecSetupPartitionTupleRouting(Relation rel,
(*tup_conv_maps)[i] = convert_tuples_by_name(tupDesc, part_tupdesc,
gettext_noop("could not convert row type"));
+ /*
+ * This is the parent's partition constraint, so any Vars in
+ * it bear the its attribute numbers. We must switch them to
+ * the leaf partition's.
+ */
+ if (partcheck)
+ my_check = map_partition_varattnos(partcheck, partrel, rel);
+
InitResultRelInfo(leaf_part_rri,
partrel,
1, /* dummy */
- false,
+ my_check,
0);
/* Open partition indices */
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index b74fa5eb5d..c8e42ae2eb 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -73,7 +73,6 @@
#define ExecEvalExpr(expr, econtext, isNull, isDone) \
((*(expr)->evalfunc) (expr, econtext, isNull, isDone))
-
/* Hook for plugins to get control in ExecutorStart() */
typedef void (*ExecutorStart_hook_type) (QueryDesc *queryDesc, int eflags);
extern PGDLLIMPORT ExecutorStart_hook_type ExecutorStart_hook;
@@ -189,7 +188,7 @@ extern void CheckValidResultRel(Relation resultRel, CmdType operation);
extern void InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
- bool load_partition_check,
+ List *partition_check,
int instrument_options);
extern ResultRelInfo *ExecGetTriggerResultRel(EState *estate, Oid relid);
extern bool ExecContextForcesOids(PlanState *planstate, bool *hasoids);
diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out
index 561cefa3c4..95a7c4da7a 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -285,6 +285,10 @@ select tableoid::regclass, * from list_parted;
part_ee_ff2 | EE | 10
(8 rows)
+-- fail due to partition constraint failure
+insert into part_ee_ff values ('gg', 1);
+ERROR: new row for relation "part_ee_ff1" violates partition constraint
+DETAIL: Failing row contains (gg, 1).
-- cleanup
drop table range_parted cascade;
NOTICE: drop cascades to 4 other objects
diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql
index 846bb5897a..77577682ac 100644
--- a/src/test/regress/sql/insert.sql
+++ b/src/test/regress/sql/insert.sql
@@ -167,6 +167,9 @@ insert into list_parted values ('EE', 1);
insert into part_ee_ff values ('EE', 10);
select tableoid::regclass, * from list_parted;
+-- fail due to partition constraint failure
+insert into part_ee_ff values ('gg', 1);
+
-- cleanup
drop table range_parted cascade;
drop table list_parted cascade;
--
2.11.0
>From f012daa2a2380582b5d56dfcb7810b363665846c Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Thu, 15 Dec 2016 17:39:19 +0900
Subject: [PATCH 5/7] Fix oddities of tuple-routing and TupleTableSlots
We must use the partition's tuple descriptor *after* a tuple is routed,
not the root table's. Partition's attributes, for example, may be
ordered diferently from the root table's.
We must then switch back to the root table's for the next tuple, because
computing partition key of a tuple to be routed must be looking at the
root table's tuple descriptor. A dedicated TupleTableSlot is allocated
within 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/commands/copy.c | 28 +++++++++++++++++++++++++++-
src/backend/executor/nodeModifyTable.c | 25 +++++++++++++++++++++++++
src/include/nodes/execnodes.h | 3 +++
3 files changed, 55 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index a0eb4241e2..bec8c73903 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2441,6 +2441,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
@@ -2489,7 +2498,8 @@ CopyFrom(CopyState cstate)
for (;;)
{
- TupleTableSlot *slot;
+ TupleTableSlot *slot,
+ *oldslot = NULL;
bool skip_tuple;
Oid loaded_oid = InvalidOid;
@@ -2576,7 +2586,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);
}
@@ -2672,6 +2694,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;
}
}
}
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index a9546106ce..da4c96a863 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.
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 703604ab9d..f49702b122 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 */
--
2.11.0
>From ef83ef4cc90a43acf22c097480f71b2b3c1c30f1 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Thu, 15 Dec 2016 18:00:47 +0900
Subject: [PATCH 6/7] Make ExecConstraints() emit 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 rowtype and the partition's.
To convert back to the correct row format, keep root table relation
descriptor and a reverse tuple conversion map in the ResultRelInfo's
of leaf partitions.
Reported by: n/a
Patch by: Amit Langote
Reports: n/a
---
src/backend/commands/copy.c | 2 +-
src/backend/commands/tablecmds.c | 2 +-
src/backend/executor/execMain.c | 83 ++++++++++++++++++++++++++++++++----
src/include/executor/executor.h | 2 +
src/include/nodes/execnodes.h | 2 +
src/test/regress/expected/insert.out | 15 ++++++-
src/test/regress/sql/insert.sql | 11 +++++
7 files changed, 106 insertions(+), 11 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index bec8c73903..9cd84e80c7 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2424,7 +2424,7 @@ CopyFrom(CopyState cstate)
InitResultRelInfo(resultRelInfo,
cstate->rel,
1, /* dummy rangetable index */
- partcheck,
+ partcheck, NULL, NULL,
0);
ExecOpenIndices(resultRelInfo, false);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 4bd4ec4e8c..67ff1715ea 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1323,7 +1323,7 @@ ExecuteTruncate(TruncateStmt *stmt)
InitResultRelInfo(resultRelInfo,
rel,
0, /* dummy rangetable index */
- NIL,
+ NIL, NULL, NULL,
0);
resultRelInfo++;
}
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 1b19bc38ef..b56fd8ca6f 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -833,7 +833,7 @@ InitPlan(QueryDesc *queryDesc, int eflags)
InitResultRelInfo(resultRelInfo,
resultRelation,
resultRelationIndex,
- partcheck,
+ partcheck, NULL, NULL,
estate->es_instrument);
resultRelInfo++;
}
@@ -1224,6 +1224,8 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
List *partition_check,
+ Relation partition_root,
+ TupleConversionMap *partition_reverse_map,
int instrument_options)
{
MemSet(resultRelInfo, 0, sizeof(ResultRelInfo));
@@ -1262,6 +1264,12 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
resultRelInfo->ri_junkFilter = NULL;
resultRelInfo->ri_projectReturning = NULL;
resultRelInfo->ri_PartitionCheck = partition_check;
+ /*
+ * Following fields are only looked at in some tuple-routing cases.
+ * In other case, they are set to NULL.
+ */
+ resultRelInfo->ri_PartitionRoot = partition_root;
+ resultRelInfo->ri_PartitionReverseMap = partition_reverse_map;
}
/*
@@ -1329,7 +1337,7 @@ ExecGetTriggerResultRel(EState *estate, Oid relid)
InitResultRelInfo(rInfo,
rel,
0, /* dummy rangetable index */
- partcheck,
+ partcheck, NULL, NULL,
estate->es_instrument);
estate->es_trig_target_relations =
lappend(estate->es_trig_target_relations, rInfo);
@@ -1775,6 +1783,26 @@ 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);
+
+ rel = resultRelInfo->ri_PartitionRoot;
+ tupdesc = RelationGetDescr(rel);
+ Assert(resultRelInfo->ri_PartitionReverseMap != NULL);
+ tuple = do_convert_tuple(tuple,
+ resultRelInfo->ri_PartitionReverseMap);
+ ExecStoreTuple(tuple, slot, InvalidBuffer, false);
+ }
insertedCols = GetInsertedColumns(resultRelInfo, estate);
updatedCols = GetUpdatedColumns(resultRelInfo, estate);
@@ -1788,9 +1816,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)));
}
}
}
@@ -1802,6 +1830,20 @@ 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)
+ {
+ HeapTuple tuple = ExecFetchSlotTuple(slot);
+
+ rel = resultRelInfo->ri_PartitionRoot;
+ tupdesc = RelationGetDescr(rel);
+ Assert(resultRelInfo->ri_PartitionReverseMap != NULL);
+ tuple = do_convert_tuple(tuple,
+ resultRelInfo->ri_PartitionReverseMap);
+ ExecStoreTuple(tuple, slot, InvalidBuffer, false);
+ }
insertedCols = GetInsertedColumns(resultRelInfo, estate);
updatedCols = GetUpdatedColumns(resultRelInfo, estate);
@@ -1814,9 +1856,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)));
}
}
@@ -1824,6 +1866,20 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
!ExecPartitionCheck(resultRelInfo, slot, estate))
{
char *val_desc;
+ Relation orig_rel = rel;
+
+ /* See the comment above. */
+ if (resultRelInfo->ri_PartitionRoot)
+ {
+ HeapTuple tuple = ExecFetchSlotTuple(slot);
+
+ rel = resultRelInfo->ri_PartitionRoot;
+ tupdesc = RelationGetDescr(rel);
+ Assert(resultRelInfo->ri_PartitionReverseMap != NULL);
+ tuple = do_convert_tuple(tuple,
+ resultRelInfo->ri_PartitionReverseMap);
+ ExecStoreTuple(tuple, slot, InvalidBuffer, false);
+ }
insertedCols = GetInsertedColumns(resultRelInfo, estate);
updatedCols = GetUpdatedColumns(resultRelInfo, estate);
@@ -1836,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));
}
}
@@ -3068,6 +3124,7 @@ ExecSetupPartitionTupleRouting(Relation rel,
Relation partrel;
TupleDesc part_tupdesc;
List *my_check = NIL;
+ TupleConversionMap *reverse_map;
/*
* We locked all the partitions above including the leaf partitions.
@@ -3097,10 +3154,20 @@ ExecSetupPartitionTupleRouting(Relation rel,
if (partcheck)
my_check = map_partition_varattnos(partcheck, partrel, rel);
+ /*
+ * We must save a reverse tuple conversion map as well, to show the
+ * correct input tuple in the error message shown by ExecConstraints()
+ * in case of routed tuples. Remember that at the point of failure,
+ * the tuple has been converted to the partition's type which might
+ * not match the input tuple.
+ */
+ reverse_map = convert_tuples_by_name(part_tupdesc, tupDesc,
+ gettext_noop("could not convert row type"));
+
InitResultRelInfo(leaf_part_rri,
partrel,
1, /* dummy */
- my_check,
+ my_check, rel, reverse_map,
0);
/* Open partition indices */
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index c8e42ae2eb..3f91d5734e 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -189,6 +189,8 @@ extern void InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
List *partition_check,
+ Relation partition_root,
+ TupleConversionMap *partition_reverse_map,
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 f49702b122..6d06af949a 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -349,6 +349,8 @@ typedef struct ResultRelInfo
List *ri_onConflictSetWhere;
List *ri_PartitionCheck;
List *ri_PartitionCheckExpr;
+ Relation ri_PartitionRoot;
+ TupleConversionMap *ri_PartitionReverseMap;
} ResultRelInfo;
/* ----------------
diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out
index 95a7c4da7a..328f776dd7 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -289,6 +289,18 @@ select tableoid::regclass, * from list_parted;
insert into part_ee_ff values ('gg', 1);
ERROR: new row for relation "part_ee_ff1" violates partition constraint
DETAIL: Failing row contains (gg, 1).
+-- check that correct row is shown in the error message if constraint fails
+-- after a tuple is routed to a partition with different rowtype from the
+-- root table
+create table part_ee_ff3 (like part_ee_ff);
+alter table part_ee_ff3 drop a;
+alter table part_ee_ff3 add a text; -- (a's attnum is now 3)
+alter table part_ee_ff attach partition part_ee_ff3 for values from (20) to (30);
+truncate part_ee_ff;
+alter table part_ee_ff add constraint check_b_25 check (b = 25);
+insert into list_parted values ('ee', 20);
+ERROR: new row for relation "part_ee_ff3" violates check constraint "check_b_25"
+DETAIL: Failing row contains (ee, 20).
-- cleanup
drop table range_parted cascade;
NOTICE: drop cascades to 4 other objects
@@ -297,10 +309,11 @@ 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 7 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
diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql
index 77577682ac..ebe371e884 100644
--- a/src/test/regress/sql/insert.sql
+++ b/src/test/regress/sql/insert.sql
@@ -170,6 +170,17 @@ select tableoid::regclass, * from list_parted;
-- fail due to partition constraint failure
insert into part_ee_ff values ('gg', 1);
+-- check that correct row is shown in the error message if constraint fails
+-- after a tuple is routed to a partition with different rowtype from the
+-- root table
+create table part_ee_ff3 (like part_ee_ff);
+alter table part_ee_ff3 drop a;
+alter table part_ee_ff3 add a text; -- (a's attnum is now 3)
+alter table part_ee_ff attach partition part_ee_ff3 for values from (20) to (30);
+truncate part_ee_ff;
+alter table part_ee_ff add constraint check_b_25 check (b = 25);
+insert into list_parted values ('ee', 20);
+
-- cleanup
drop table range_parted cascade;
drop table list_parted cascade;
--
2.11.0
>From 71590d20c1f8bbe8fb1f4996ff343066a21ea256 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Fri, 16 Dec 2016 09:45:57 +0900
Subject: [PATCH 7/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 | 40 +++++++++++++++++++++++++++++++++++-
src/test/regress/sql/insert.sql | 19 +++++++++++++++++
2 files changed, 58 insertions(+), 1 deletion(-)
diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out
index 328f776dd7..aaa74da607 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -301,6 +301,36 @@ alter table part_ee_ff add constraint check_b_25 check (b = 25);
insert into list_parted values ('ee', 20);
ERROR: new row for relation "part_ee_ff3" violates check constraint "check_b_25"
DETAIL: Failing row contains (ee, 20).
+alter table part_ee_ff drop constraint check_b_25;
+-- 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_ff4 partition of part_ee_ff for values from (30) to (40) partition by range (b);
+create table part_ee_ff4_1 partition of part_ee_ff4 for values from (30) to (35);
+create table part_ee_ff4_2 partition of part_ee_ff4 for values from (35) to (40);
+truncate list_parted;
+insert into list_parted values ('aa'), ('cc');
+insert into list_parted select 'Ff', s.a from generate_series(1, 39) 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 | Ff | 20 | 29
+ part_ee_ff4_1 | Ff | 30 | 34
+ part_ee_ff4_2 | Ff | 35 | 39
+ part_gg2_1 | gg | 1 | 4
+ part_gg2_2 | gg | 5 | 9
+ part_null | | 1 | 1
+(10 rows)
+
-- cleanup
drop table range_parted cascade;
NOTICE: drop cascades to 4 other objects
@@ -309,7 +339,7 @@ drop cascades to table part2
drop cascades to table part3
drop cascades to table part4
drop table list_parted cascade;
-NOTICE: drop cascades to 7 other objects
+NOTICE: drop cascades to 15 other objects
DETAIL: drop cascades to table part_aa_bb
drop cascades to table part_cc_dd
drop cascades to table part_null
@@ -317,3 +347,11 @@ 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_ff4
+drop cascades to table part_ee_ff4_1
+drop cascades to table part_ee_ff4_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
diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql
index ebe371e884..0fa9cef4b5 100644
--- a/src/test/regress/sql/insert.sql
+++ b/src/test/regress/sql/insert.sql
@@ -180,6 +180,25 @@ alter table part_ee_ff attach partition part_ee_ff3 for values from (20) to (30)
truncate part_ee_ff;
alter table part_ee_ff add constraint check_b_25 check (b = 25);
insert into list_parted values ('ee', 20);
+alter table part_ee_ff drop constraint check_b_25;
+
+-- 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_ff4 partition of part_ee_ff for values from (30) to (40) partition by range (b);
+create table part_ee_ff4_1 partition of part_ee_ff4 for values from (30) to (35);
+create table part_ee_ff4_2 partition of part_ee_ff4 for values from (35) to (40);
+
+truncate list_parted;
+insert into list_parted values ('aa'), ('cc');
+insert into list_parted select 'Ff', s.a from generate_series(1, 39) 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;
--
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
