On 2017/01/05 5:50, Robert Haas wrote:
> On Tue, Dec 27, 2016 at 3:59 AM, Amit Langote
> <langote_amit...@lab.ntt.co.jp> wrote:
>> Patches 0001 to 0006 unchanged.
>
> Committed 0001 earlier, as mentioned in a separate email. Committed
> 0002 and part of 0003.
Thanks! I realized however that the approach I used in 0002 of passing
the original slot to ExecConstraints() fails in certain situations. For
example, if a BR trigger changes the tuple, the original slot would not
receive those changes, so it will be wrong to use such a tuple anymore.
In attached 0001, I switched back to the approach of converting the
partition-tupdesc-based tuple back to the root partitioned table's format.
The converted tuple contains the changes by BR triggers, if any. Sorry
about some unnecessary work.
> But I'm skeptical that the as-patched-by-0003
> logic in generate_partition_qual() makes sense. You do this:
>
> result = list_concat(generate_partition_qual(parent),
> copyObject(rel->rd_partcheck));
>
> /* Mark Vars with correct attnos */
> result = map_partition_varattnos(result, rel, parent);
>
> But that has the effect of applying map_partition_varattnos to
> everything in rel->rd_partcheck in addition to applying it to
> everything returned by generate_partition_qual() on the parent, which
> doesn't seem right.
I've replaced this portion of the code with (as also mentioned below):
/* Quick copy */
if (rel->rd_partcheck != NIL)
return copyObject(rel->rd_partcheck);
Down below (for the case when the partition qual is not cached, we now do
this:
my_qual = get_qual_from_partbound(rel, parent, bound);
/* 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;
/*
* Change Vars to have partition's attnos instead of the parent's.
* We do this after we concatenate the parent's quals, because
* we want every Var in it to bear this relation's attnos.
*/
result = map_partition_varattnos(result, rel, parent);
Which is then cached wholly in rd_partcheck.
As for your concern whether it's correct to do so, consider that doing
generate_partition_qual() on the parent returns qual with Vars that bear
the parent's attnos (which is OK as far parent as partition is concerned).
To apply the qual to the current relation as partition, we must change
the Vars to have this relation's attnos.
> Also, don't we want to do map_partition_varattnos() just ONCE, rather
> than on every call to this function? I think maybe your concern is
> that the parent might be changed without a relcache flush on the
> child, but I don't quite see how that could happen. If the parent's
> tuple descriptor changes, surely the child's tuple descriptor would
> have to be altered at the same time...
Makes sense. I fixed so that we return copyObject(rel->rd_partcheck), if
it's non-NIL, instead of generating parent's qual and doing the mapping
again. For some reason, I thought we couldn't save the mapped version in
the relcache.
By the way, in addition to the previously mentioned bug of RETURNING, I
found that WITH CHECK OPTION didn't work correctly as well. In fact
automatically updatable views failed to consider partitioned tables at
all. Patch 0007 is addressed towards fixing that.
Thanks,
Amit
>From e408234633c01817d6a2313fdbdccdb4f0057c1e Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Fri, 6 Jan 2017 15:53:10 +0900
Subject: [PATCH 1/7] Fix reporting of violation in ExecConstraints, again
We decided in f1b4c771ea74f42447dccaed42ffcdcccf3aa694 that passing
the original slot (one containing the tuple formatted per root
partitioned table's tupdesc) to ExecConstraints(), but that breaks
certain cases. Imagine what would happen if a BR trigger changed the
tuple - the original slot would not contain those changes.
So, it seems better to convert (if necessary) the tuple formatted
per partition tupdesc after tuple-routing back to the root table's
format and use the converted tuple to make val_desc shown in the
message if an error occurs.
---
src/backend/commands/copy.c | 6 ++--
src/backend/executor/execMain.c | 53 +++++++++++++++++++++++++++++-----
src/backend/executor/nodeModifyTable.c | 5 ++--
src/include/executor/executor.h | 3 +-
src/test/regress/expected/insert.out | 18 ++++++++++--
src/test/regress/sql/insert.sql | 17 ++++++++++-
6 files changed, 82 insertions(+), 20 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index f56b2ac49b..65eb167087 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2491,8 +2491,7 @@ CopyFrom(CopyState cstate)
for (;;)
{
- TupleTableSlot *slot,
- *oldslot;
+ TupleTableSlot *slot;
bool skip_tuple;
Oid loaded_oid = InvalidOid;
@@ -2534,7 +2533,6 @@ CopyFrom(CopyState cstate)
ExecStoreTuple(tuple, slot, InvalidBuffer, false);
/* Determine the partition to heap_insert the tuple into */
- oldslot = slot;
if (cstate->partition_dispatch_info)
{
int leaf_part_index;
@@ -2625,7 +2623,7 @@ CopyFrom(CopyState cstate)
/* Check the constraints of the tuple */
if (cstate->rel->rd_att->constr ||
resultRelInfo->ri_PartitionCheck)
- ExecConstraints(resultRelInfo, slot, oldslot, estate);
+ ExecConstraints(resultRelInfo, slot, estate);
if (useHeapMultiInsert)
{
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index ff277d300a..332c54c819 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1763,8 +1763,7 @@ ExecPartitionCheck(ResultRelInfo *resultRelInfo, TupleTableSlot *slot,
*/
void
ExecConstraints(ResultRelInfo *resultRelInfo,
- TupleTableSlot *slot, TupleTableSlot *orig_slot,
- EState *estate)
+ TupleTableSlot *slot, EState *estate)
{
Relation rel = resultRelInfo->ri_RelationDesc;
TupleDesc tupdesc = RelationGetDescr(rel);
@@ -1787,23 +1786,37 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
{
char *val_desc;
Relation orig_rel = rel;
- TupleDesc orig_tupdesc = tupdesc;
+ TupleDesc orig_tupdesc = RelationGetDescr(rel);
/*
- * choose the correct relation to build val_desc from the
- * tuple contained in orig_slot
+ * 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 val_desc shown error message matches the
+ * input tuple.
*/
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"));
+ if (map != NULL)
+ {
+ tuple = do_convert_tuple(tuple, map);
+ ExecStoreTuple(tuple, slot, InvalidBuffer, false);
+ }
}
insertedCols = GetInsertedColumns(resultRelInfo, estate);
updatedCols = GetUpdatedColumns(resultRelInfo, estate);
modifiedCols = bms_union(insertedCols, updatedCols);
val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel),
- orig_slot,
+ slot,
tupdesc,
modifiedCols,
64);
@@ -1830,15 +1843,27 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
/* See the comment above. */
if (resultRelInfo->ri_PartitionRoot)
{
+ HeapTuple tuple = ExecFetchSlotTuple(slot);
+ TupleDesc old_tupdesc = RelationGetDescr(rel);
+ TupleConversionMap *map;
+
rel = resultRelInfo->ri_PartitionRoot;
tupdesc = RelationGetDescr(rel);
+ /* a reverse map */
+ map = convert_tuples_by_name(old_tupdesc, tupdesc,
+ gettext_noop("could not convert row type"));
+ if (map != NULL)
+ {
+ tuple = do_convert_tuple(tuple, map);
+ ExecStoreTuple(tuple, slot, InvalidBuffer, false);
+ }
}
insertedCols = GetInsertedColumns(resultRelInfo, estate);
updatedCols = GetUpdatedColumns(resultRelInfo, estate);
modifiedCols = bms_union(insertedCols, updatedCols);
val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel),
- orig_slot,
+ slot,
tupdesc,
modifiedCols,
64);
@@ -1860,15 +1885,27 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
/* See the comment above. */
if (resultRelInfo->ri_PartitionRoot)
{
+ HeapTuple tuple = ExecFetchSlotTuple(slot);
+ TupleDesc old_tupdesc = RelationGetDescr(rel);
+ TupleConversionMap *map;
+
rel = resultRelInfo->ri_PartitionRoot;
tupdesc = RelationGetDescr(rel);
+ /* a reverse map */
+ map = convert_tuples_by_name(old_tupdesc, tupdesc,
+ gettext_noop("could not convert row type"));
+ if (map != NULL)
+ {
+ tuple = do_convert_tuple(tuple, map);
+ ExecStoreTuple(tuple, slot, InvalidBuffer, false);
+ }
}
insertedCols = GetInsertedColumns(resultRelInfo, estate);
updatedCols = GetUpdatedColumns(resultRelInfo, estate);
modifiedCols = bms_union(insertedCols, updatedCols);
val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel),
- orig_slot,
+ slot,
tupdesc,
modifiedCols,
64);
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 4692427e60..23e04893b8 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -262,7 +262,6 @@ ExecInsert(ModifyTableState *mtstate,
Relation resultRelationDesc;
Oid newId;
List *recheckIndexes = NIL;
- TupleTableSlot *oldslot = slot;
/*
* get the heap tuple out of the tuple table slot, making sure we have a
@@ -433,7 +432,7 @@ ExecInsert(ModifyTableState *mtstate,
* Check the constraints of the tuple
*/
if (resultRelationDesc->rd_att->constr || resultRelInfo->ri_PartitionCheck)
- ExecConstraints(resultRelInfo, slot, oldslot, estate);
+ ExecConstraints(resultRelInfo, slot, estate);
if (onconflict != ONCONFLICT_NONE && resultRelInfo->ri_NumIndices > 0)
{
@@ -994,7 +993,7 @@ lreplace:;
* tuple-routing is performed here, hence the slot remains unchanged.
*/
if (resultRelationDesc->rd_att->constr || resultRelInfo->ri_PartitionCheck)
- ExecConstraints(resultRelInfo, slot, slot, estate);
+ ExecConstraints(resultRelInfo, slot, estate);
/*
* replace the heap tuple
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index b9c7f72903..3e8d64686e 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -195,8 +195,7 @@ extern void InitResultRelInfo(ResultRelInfo *resultRelInfo,
extern ResultRelInfo *ExecGetTriggerResultRel(EState *estate, Oid relid);
extern bool ExecContextForcesOids(PlanState *planstate, bool *hasoids);
extern void ExecConstraints(ResultRelInfo *resultRelInfo,
- TupleTableSlot *slot, TupleTableSlot *orig_slot,
- EState *estate);
+ TupleTableSlot *slot, EState *estate);
extern void ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo,
TupleTableSlot *slot, EState *estate);
extern LockTupleMode ExecUpdateLockMode(EState *estate, ResultRelInfo *relinfo);
diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out
index ca3134c34c..501d50eeac 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -335,10 +335,24 @@ select tableoid::regclass, * from p;
truncate p;
alter table p add constraint check_b check (b = 3);
+create function p11_trig_fn()
+returns trigger AS
+$$
+begin
+ NEW.b := 4;
+ return NEW;
+end;
+$$
+language plpgsql;
+create trigger p11_trig before insert ON p11
+ for each row execute procedure p11_trig_fn();
-- check that correct input row is shown when constraint check_b fails on p11
--- after "(1, 2)" is routed to it
+-- after "(1, 2)" is routed to it (actually "(1, 4)" would be shown due to the
+-- BR trigger p11_trig_fn)
insert into p values (1, 2);
ERROR: new row for relation "p11" violates check constraint "check_b"
-DETAIL: Failing row contains (1, 2).
+DETAIL: Failing row contains (1, 4).
+drop trigger p11_trig on p11;
+drop function p11_trig_fn();
-- cleanup
drop table p, p1, p11;
diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql
index 09c9879da1..22aa94e181 100644
--- a/src/test/regress/sql/insert.sql
+++ b/src/test/regress/sql/insert.sql
@@ -197,9 +197,24 @@ select tableoid::regclass, * from p;
truncate p;
alter table p add constraint check_b check (b = 3);
+create function p11_trig_fn()
+returns trigger AS
+$$
+begin
+ NEW.b := 4;
+ return NEW;
+end;
+$$
+language plpgsql;
+create trigger p11_trig before insert ON p11
+ for each row execute procedure p11_trig_fn();
+
-- check that correct input row is shown when constraint check_b fails on p11
--- after "(1, 2)" is routed to it
+-- after "(1, 2)" is routed to it (actually "(1, 4)" would be shown due to the
+-- BR trigger p11_trig_fn)
insert into p values (1, 2);
+drop trigger p11_trig on p11;
+drop function p11_trig_fn();
-- cleanup
drop table p, p1, p11;
--
2.11.0
>From d8f1dd0f87f2c99c951cae15e0b90fbc96a9783b 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] Fix a bug in how we generate partition constraints
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 various levels causing the same attribute to be
numbered differently in different instances of the Var corresponding
to a given attribute.
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, so that Vars in the final expression are
numbered according the leaf relation (to which it is supposed to apply).
Reported by: n/a
Patch by: Amit Langote
Reports: n/a
---
src/backend/catalog/partition.c | 99 +++++++++++++++----------------
src/backend/commands/tablecmds.c | 7 ++-
src/include/catalog/partition.h | 1 +
src/test/regress/expected/alter_table.out | 30 ++++++++++
src/test/regress/sql/alter_table.sql | 25 ++++++++
5 files changed, 110 insertions(+), 52 deletions(-)
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index f54e1bdf3f..874e69d8d6 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -852,10 +852,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);
@@ -876,38 +872,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;
}
/*
@@ -1496,27 +1505,15 @@ generate_partition_qual(Relation rel)
/* Guard against stack overflow due to overly deep partition tree */
check_stack_depth();
+ /* Quick copy */
+ if (rel->rd_partcheck != NIL)
+ return copyObject(rel->rd_partcheck);
+
/* Grab at least an AccessShareLock on the parent table */
parent = heap_open(get_partition_parent(RelationGetRelid(rel)),
AccessShareLock);
- /* Quick copy */
- if (rel->rd_partcheck)
- {
- if (parent->rd_rel->relispartition)
- result = list_concat(generate_partition_qual(parent),
- copyObject(rel->rd_partcheck));
- else
- result = copyObject(rel->rd_partcheck);
-
- heap_close(parent, AccessShareLock);
- 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));
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for relation %u",
@@ -1533,20 +1530,22 @@ generate_partition_qual(Relation rel)
my_qual = get_qual_from_partbound(rel, parent, bound);
- /* If requested, add parent's quals to the list (if any) */
+ /* Add the parent's quals to the list (if any) */
if (parent->rd_rel->relispartition)
- {
- List *parent_check;
-
- parent_check = generate_partition_qual(parent);
- result = list_concat(parent_check, my_qual);
- }
+ result = list_concat(generate_partition_qual(parent), my_qual);
else
result = my_qual;
- /* Save a copy of my_qual in the relcache */
+ /*
+ * Change Vars to have partition's attnos instead of the parent's.
+ * We do this after we concatenate the parent's quals, because
+ * we want every Var in it to bear this relation's attnos.
+ */
+ result = map_partition_varattnos(result, rel, parent);
+
+ /* Save a copy in the relcache */
oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
- rel->rd_partcheck = copyObject(my_qual);
+ rel->rd_partcheck = copyObject(result);
MemoryContextSwitchTo(oldcxt);
/* Keep the parent locked until commit */
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 42558ec42e..f913e87bc8 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13429,6 +13429,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))
@@ -13451,8 +13452,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/include/catalog/partition.h b/src/include/catalog/partition.h
index 1c3c4d6ac3..537f0aad67 100644
--- a/src/include/catalog/partition.h
+++ b/src/include/catalog/partition.h
@@ -77,6 +77,7 @@ extern bool partition_bounds_equal(PartitionKey key,
extern void check_new_partition_bound(char *relname, Relation parent, Node *bound);
extern Oid get_partition_parent(Oid relid);
extern List *get_qual_from_partbound(Relation rel, Relation parent, Node *bound);
+extern List *map_partition_varattnos(List *expr, Relation partrel, Relation parent);
extern List *RelationGetPartitionQual(Relation rel);
/* For tuple routing */
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index d8028c6747..b290bc810e 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3339,3 +3339,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 2265266517..5d07e2ede4 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2191,3 +2191,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 9c636bea31c2faf0715a12a2dd4cdea73e4e7f64 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Tue, 13 Dec 2016 15:07:41 +0900
Subject: [PATCH 3/7] Fix a bug of insertion into an internal partition.
Since implicit partition constraints are not inherited, an internal
partition's constraint was not being enforced when targeted directly.
So, include such constraint when setting up leaf partition result
relations for tuple-routing.
Reported by: n/a
Patch by: Amit Langote
Reports: n/a
---
src/backend/commands/copy.c | 1 -
src/backend/commands/tablecmds.c | 1 -
src/backend/executor/execMain.c | 42 ++++++++++++++++++++++++++++--------
src/include/executor/executor.h | 1 -
src/test/regress/expected/insert.out | 6 ++++++
src/test/regress/sql/insert.sql | 5 +++++
6 files changed, 44 insertions(+), 12 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 65eb167087..baab1d0eeb 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2425,7 +2425,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 f913e87bc8..37940f4469 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1324,7 +1324,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 332c54c819..7a073c6df3 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,13 +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;
}
@@ -1327,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 =
@@ -3169,7 +3194,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 3e8d64686e..502807e53f 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 501d50eeac..b3cfd44ffd 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -354,5 +354,11 @@ ERROR: new row for relation "p11" violates check constraint "check_b"
DETAIL: Failing row contains (1, 4).
drop trigger p11_trig on p11;
drop function p11_trig_fn();
+-- 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, p1, p11;
diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql
index 22aa94e181..c5a75a505f 100644
--- a/src/test/regress/sql/insert.sql
+++ b/src/test/regress/sql/insert.sql
@@ -216,5 +216,10 @@ insert into p values (1, 2);
drop trigger p11_trig on p11;
drop function p11_trig_fn();
+-- 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, p1, p11;
--
2.11.0
>From 3821a771a264a9bbd101f434f2bf76c883ece91b Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Wed, 21 Dec 2016 10:51:32 +0900
Subject: [PATCH 4/7] Add some more tests for tuple-routing
We fixed some issues with how PartitionDispatch related code handled
multi-level partitioned tables in commit a25665088d, but didn't add
any tests.
Reported by: Dmitry Ivanov, Robert Haas
Patch by: Amit Langote
Reports: https://www.postgresql.org/message-id/0d5b64c9-fa05-4dab-93e7-56576d1193ca%40postgrespro.ru
https://www.postgresql.org/message-id/CA%2BTgmoZ86v1G%2Bzx9etMiSQaBBvYMKfU-iitqZArSh5z0n8Q4cA%40mail.gmail.com
---
src/test/regress/expected/insert.out | 38 +++++++++++++++++++++++++++++++++++-
src/test/regress/sql/insert.sql | 18 +++++++++++++++++
2 files changed, 55 insertions(+), 1 deletion(-)
diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out
index b3cfd44ffd..f4b97bf228 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 c5a75a505f..1916c9849a 100644
--- a/src/test/regress/sql/insert.sql
+++ b/src/test/regress/sql/insert.sql
@@ -167,6 +167,24 @@ insert into list_parted values ('EE', 1);
insert into part_ee_ff values ('EE', 10);
select tableoid::regclass, * from list_parted;
+-- some more tests to exercise tuple-routing with multi-level partitioning
+create table part_gg partition of list_parted for values in ('gg') partition by range (b);
+create table part_gg1 partition of part_gg for values from (unbounded) to (1);
+create table part_gg2 partition of part_gg for values from (1) to (10) partition by range (b);
+create table part_gg2_1 partition of part_gg2 for values from (1) to (5);
+create table part_gg2_2 partition of part_gg2 for values from (5) to (10);
+
+create table part_ee_ff3 partition of part_ee_ff for values from (20) to (30) partition by range (b);
+create table part_ee_ff3_1 partition of part_ee_ff3 for values from (20) to (25);
+create table part_ee_ff3_2 partition of part_ee_ff3 for values from (25) to (30);
+
+truncate list_parted;
+insert into list_parted values ('aa'), ('cc');
+insert into list_parted select 'Ff', s.a from generate_series(1, 29) s(a);
+insert into list_parted select 'gg', s.a from generate_series(1, 9) s(a);
+insert into list_parted (b) values (1);
+select tableoid::regclass::text, a, min(b) as min_b, max(b) as max_b from list_parted group by 1, 2 order by 1;
+
-- cleanup
drop table range_parted cascade;
drop table list_parted cascade;
--
2.11.0
>From 82a8593dfdb49bf84833dddd7c725a5545e16d04 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Mon, 26 Dec 2016 17:44:14 +0900
Subject: [PATCH 5/7] Avoid tuple coversion in common partitioning cases
Currently, the tuple conversion is performed after a tuple is routed,
even if the attributes of a target leaf partition map one-to-one with
those of the root table, which is wasteful. Avoid that by making
convert_tuples_by_name() return a NULL map for such cases.
Reported by: n/a
Patch by: Amit Langote
Reports: n/a
---
src/backend/access/common/tupconvert.c | 8 ++++++--
src/backend/catalog/partition.c | 5 ++---
src/backend/commands/analyze.c | 1 +
src/backend/executor/execMain.c | 7 ++++---
src/backend/executor/execQual.c | 2 +-
src/include/access/tupconvert.h | 1 +
6 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/src/backend/access/common/tupconvert.c b/src/backend/access/common/tupconvert.c
index b17ceafa6e..bbbd271e1f 100644
--- a/src/backend/access/common/tupconvert.c
+++ b/src/backend/access/common/tupconvert.c
@@ -202,6 +202,7 @@ convert_tuples_by_position(TupleDesc indesc,
TupleConversionMap *
convert_tuples_by_name(TupleDesc indesc,
TupleDesc outdesc,
+ bool consider_typeid,
const char *msg)
{
TupleConversionMap *map;
@@ -216,11 +217,14 @@ convert_tuples_by_name(TupleDesc indesc,
/*
* Check to see if the map is one-to-one and the tuple types are the same.
* (We check the latter because if they're not, we want to do conversion
- * to inject the right OID into the tuple datum.)
+ * to inject the right OID into the tuple datum. In the partitioning
+ * case (!consider_typeid), tdhasoids must always match between indesc
+ * and outdesc, so we need not require tdtypeid's to be the same.)
*/
if (indesc->natts == outdesc->natts &&
- indesc->tdtypeid == outdesc->tdtypeid)
+ (!consider_typeid || indesc->tdtypeid == outdesc->tdtypeid))
{
+ Assert(!consider_typeid && indesc->tdhasoid == outdesc->tdhasoid);
same = true;
for (i = 0; i < n; i++)
{
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 874e69d8d6..61ad945062 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1054,7 +1054,7 @@ RelationGetPartitionDispatchInfo(Relation rel, int lockmode,
*/
pd[i]->tupslot = MakeSingleTupleTableSlot(tupdesc);
pd[i]->tupmap = convert_tuples_by_name(RelationGetDescr(parent),
- tupdesc,
+ tupdesc, false,
gettext_noop("could not convert row type"));
}
else
@@ -1660,12 +1660,11 @@ get_partition_for_tuple(PartitionDispatch *pd,
return -1;
}
- if (myslot != NULL)
+ if (myslot != NULL && map != NULL)
{
HeapTuple tuple = ExecFetchSlotTuple(slot);
ExecClearTuple(myslot);
- Assert(map != NULL);
tuple = do_convert_tuple(tuple, map);
ExecStoreTuple(tuple, myslot, InvalidBuffer, true);
slot = myslot;
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index e3e1a53072..31bec9f961 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -1419,6 +1419,7 @@ acquire_inherited_sample_rows(Relation onerel, int elevel,
map = convert_tuples_by_name(RelationGetDescr(childrel),
RelationGetDescr(onerel),
+ true,
gettext_noop("could not convert row type"));
if (map != NULL)
{
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 7a073c6df3..d3ebf7e146 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1828,7 +1828,7 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
rel = resultRelInfo->ri_PartitionRoot;
tupdesc = RelationGetDescr(rel);
/* a reverse map */
- map = convert_tuples_by_name(orig_tupdesc, tupdesc,
+ map = convert_tuples_by_name(orig_tupdesc, tupdesc, false,
gettext_noop("could not convert row type"));
if (map != NULL)
{
@@ -1875,7 +1875,7 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
rel = resultRelInfo->ri_PartitionRoot;
tupdesc = RelationGetDescr(rel);
/* a reverse map */
- map = convert_tuples_by_name(old_tupdesc, tupdesc,
+ map = convert_tuples_by_name(old_tupdesc, tupdesc, false,
gettext_noop("could not convert row type"));
if (map != NULL)
{
@@ -1917,7 +1917,7 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
rel = resultRelInfo->ri_PartitionRoot;
tupdesc = RelationGetDescr(rel);
/* a reverse map */
- map = convert_tuples_by_name(old_tupdesc, tupdesc,
+ map = convert_tuples_by_name(old_tupdesc, tupdesc, false,
gettext_noop("could not convert row type"));
if (map != NULL)
{
@@ -3189,6 +3189,7 @@ ExecSetupPartitionTupleRouting(Relation rel,
* partition from the parent's type to the partition's.
*/
(*tup_conv_maps)[i] = convert_tuples_by_name(tupDesc, part_tupdesc,
+ false,
gettext_noop("could not convert row type"));
InitResultRelInfo(leaf_part_rri,
diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index bf007b7efd..346b2ff17e 100644
--- a/src/backend/executor/execQual.c
+++ b/src/backend/executor/execQual.c
@@ -2928,7 +2928,7 @@ ExecEvalConvertRowtype(ConvertRowtypeExprState *cstate,
/* prepare map from old to new attribute numbers */
cstate->map = convert_tuples_by_name(cstate->indesc,
- cstate->outdesc,
+ cstate->outdesc, true,
gettext_noop("could not convert row type"));
cstate->initialized = true;
diff --git a/src/include/access/tupconvert.h b/src/include/access/tupconvert.h
index e86cfd56c8..231fe872d7 100644
--- a/src/include/access/tupconvert.h
+++ b/src/include/access/tupconvert.h
@@ -36,6 +36,7 @@ extern TupleConversionMap *convert_tuples_by_position(TupleDesc indesc,
extern TupleConversionMap *convert_tuples_by_name(TupleDesc indesc,
TupleDesc outdesc,
+ bool consider_typeid,
const char *msg);
extern AttrNumber *convert_tuples_by_name_map(TupleDesc indesc,
--
2.11.0
>From 53f4532635d4a4faaec90c42d8bfa54fbbffc9c5 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Tue, 27 Dec 2016 16:56:58 +0900
Subject: [PATCH 6/7] Fix RETURNING to work correctly after tuple-routing
In ExecInsert(), do not switch back to the original resultRelInfo until
after we finish ExecProcessReturning(), so that RETURNING projection
is done considering the partition the tuple was routed to. For the
projection to work correctly, we must initialize the same for each
leaf partition during ModifyTableState initialization.
With this commit, map_partition_varattnos() now accepts one more argument
viz. target_varno. Previously, it assumed varno = 1 for its input
expressions, which limited its applicability. It was enought so far
since its usage was limited to partition constraints. To use it with
expressions such as an INSERT's returning list, we must be prepared for
varnos != 1 as in the change above.
Reported by: n/a
Patch by: Amit Langote
Reports: n/a
---
src/backend/catalog/partition.c | 8 ++++---
src/backend/commands/tablecmds.c | 1 +
src/backend/executor/execMain.c | 4 ++--
src/backend/executor/nodeModifyTable.c | 44 +++++++++++++++++++++++++++-------
src/include/catalog/partition.h | 3 ++-
src/test/regress/expected/insert.out | 24 ++++++++++++++++++-
src/test/regress/sql/insert.sql | 16 ++++++++++++-
7 files changed, 84 insertions(+), 16 deletions(-)
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 61ad945062..d08e057a2e 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -883,7 +883,8 @@ get_qual_from_partbound(Relation rel, Relation parent, Node *bound)
* different from the parent's.
*/
List *
-map_partition_varattnos(List *expr, Relation partrel, Relation parent)
+map_partition_varattnos(List *expr, int target_varno,
+ Relation partrel, Relation parent)
{
TupleDesc tupdesc = RelationGetDescr(parent);
AttrNumber attno;
@@ -908,7 +909,7 @@ map_partition_varattnos(List *expr, Relation partrel, Relation parent)
}
expr = (List *) map_variable_attnos((Node *) expr,
- 1, 0,
+ target_varno, 0,
part_attnos,
tupdesc->natts,
&found_whole_row);
@@ -1540,8 +1541,9 @@ generate_partition_qual(Relation rel)
* Change Vars to have partition's attnos instead of the parent's.
* We do this after we concatenate the parent's quals, because
* we want every Var in it to bear this relation's attnos.
+ * It's safe to assume varno = 1 here.
*/
- result = map_partition_varattnos(result, rel, parent);
+ result = map_partition_varattnos(result, 1, rel, parent);
/* Save a copy in the relcache */
oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 37940f4469..97042321c7 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13453,6 +13453,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
constr = linitial(partConstraint);
my_constr = make_ands_implicit((Expr *) constr);
tab->partition_constraint = map_partition_varattnos(my_constr,
+ 1,
part_rel,
rel);
/* keep our lock until commit */
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index d3ebf7e146..060b036ddf 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1279,10 +1279,10 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
/*
* This is not our own partition constraint, but rather an ancestor's.
* So any Vars in it bear the ancestor's attribute numbers. We must
- * switch them to our own.
+ * switch them to our own. (dummy varno = 1)
*/
if (partition_check != NIL)
- partition_check = map_partition_varattnos(partition_check,
+ partition_check = map_partition_varattnos(partition_check, 1,
resultRelationDesc,
partition_root);
}
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 23e04893b8..f46070fde7 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 *result = NULL;
/*
* get the heap tuple out of the tuple table slot, making sure we have a
@@ -573,12 +574,6 @@ ExecInsert(ModifyTableState *mtstate,
list_free(recheckIndexes);
- if (saved_resultRelInfo)
- {
- resultRelInfo = saved_resultRelInfo;
- estate->es_result_relation_info = resultRelInfo;
- }
-
/*
* Check any WITH CHECK OPTION constraints from parent views. We are
* required to do this after testing all constraints and uniqueness
@@ -596,9 +591,15 @@ ExecInsert(ModifyTableState *mtstate,
/* Process RETURNING if present */
if (resultRelInfo->ri_projectReturning)
- return ExecProcessReturning(resultRelInfo, slot, planSlot);
+ result = ExecProcessReturning(resultRelInfo, slot, planSlot);
- return NULL;
+ if (saved_resultRelInfo)
+ {
+ resultRelInfo = saved_resultRelInfo;
+ estate->es_result_relation_info = resultRelInfo;
+ }
+
+ return result;
}
/* ----------------------------------------------------------------
@@ -1785,6 +1786,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
{
TupleTableSlot *slot;
ExprContext *econtext;
+ List *returningList;
/*
* Initialize result tuple slot and assign its rowtype using the first
@@ -1817,6 +1819,32 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
resultRelInfo->ri_RelationDesc->rd_att);
resultRelInfo++;
}
+
+ /*
+ * Build a projection for each leaf partition rel. Note that we
+ * didn't build the returningList for each partition within the
+ * planner, but simple translation of the varattnos for each
+ * partition will suffice. This only occurs for the INSERT case;
+ * UPDATE/DELETE are handled above.
+ */
+ resultRelInfo = mtstate->mt_partitions;
+ returningList = linitial(node->returningLists);
+ for (i = 0; i < mtstate->mt_num_partitions; i++)
+ {
+ Relation partrel = resultRelInfo->ri_RelationDesc;
+ List *rlist,
+ *rliststate;
+
+ /* varno = node->nominalRelation */
+ rlist = map_partition_varattnos(returningList,
+ node->nominalRelation,
+ partrel, rel);
+ rliststate = (List *) ExecInitExpr((Expr *) rlist, &mtstate->ps);
+ resultRelInfo->ri_projectReturning =
+ ExecBuildProjectionInfo(rliststate, econtext, slot,
+ resultRelInfo->ri_RelationDesc->rd_att);
+ resultRelInfo++;
+ }
}
else
{
diff --git a/src/include/catalog/partition.h b/src/include/catalog/partition.h
index 537f0aad67..df7dcce331 100644
--- a/src/include/catalog/partition.h
+++ b/src/include/catalog/partition.h
@@ -77,7 +77,8 @@ extern bool partition_bounds_equal(PartitionKey key,
extern void check_new_partition_bound(char *relname, Relation parent, Node *bound);
extern Oid get_partition_parent(Oid relid);
extern List *get_qual_from_partbound(Relation rel, Relation parent, Node *bound);
-extern List *map_partition_varattnos(List *expr, Relation partrel, Relation parent);
+extern List *map_partition_varattnos(List *expr, int target_varno,
+ Relation partrel, Relation parent);
extern List *RelationGetPartitionQual(Relation rel);
/* For tuple routing */
diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out
index f4b97bf228..2c7e4a8337 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -396,5 +396,27 @@ drop function p11_trig_fn();
insert into p1 (a, b) values (2, 3);
ERROR: new row for relation "p11" violates partition constraint
DETAIL: Failing row contains (3, 2).
+-- check that RETURNING works correctly with tuple-routing
+alter table p drop constraint check_b;
+create table p12 partition of p1 for values from (5) to (10);
+create table p2 (b int not null, a int not null);
+alter table p attach partition p2 for values from (1, 10) to (1, 20);
+create table p3 partition of p for values from (1, 20) to (1, 30);
+create table p4 (like p);
+alter table p4 drop a;
+alter table p4 add a int not null;
+alter table p attach partition p4 for values from (1, 30) to (1, 40);
+with ins (a, b, c) as
+ (insert into p (b, a) select s.a, 1 from generate_series(2, 39) s(a) returning tableoid::regclass, *)
+ select a, b, min(c), max(c) from ins group by a, b order by 1;
+ a | b | min | max
+-----+---+-----+-----
+ p11 | 1 | 2 | 4
+ p12 | 1 | 5 | 9
+ p2 | 1 | 10 | 19
+ p3 | 1 | 20 | 29
+ p4 | 1 | 30 | 39
+(5 rows)
+
-- cleanup
-drop table p, p1, p11;
+drop table p, p1, p11, p12, p2, p3, p4;
diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql
index 1916c9849a..aa0cacb744 100644
--- a/src/test/regress/sql/insert.sql
+++ b/src/test/regress/sql/insert.sql
@@ -239,5 +239,19 @@ drop function p11_trig_fn();
-- selected by tuple-routing
insert into p1 (a, b) values (2, 3);
+-- check that RETURNING works correctly with tuple-routing
+alter table p drop constraint check_b;
+create table p12 partition of p1 for values from (5) to (10);
+create table p2 (b int not null, a int not null);
+alter table p attach partition p2 for values from (1, 10) to (1, 20);
+create table p3 partition of p for values from (1, 20) to (1, 30);
+create table p4 (like p);
+alter table p4 drop a;
+alter table p4 add a int not null;
+alter table p attach partition p4 for values from (1, 30) to (1, 40);
+with ins (a, b, c) as
+ (insert into p (b, a) select s.a, 1 from generate_series(2, 39) s(a) returning tableoid::regclass, *)
+ select a, b, min(c), max(c) from ins group by a, b order by 1;
+
-- cleanup
-drop table p, p1, p11;
+drop table p, p1, p11, p12, p2, p3, p4;
--
2.11.0
>From f6855b58b72128db9cfeb8d648eff4b0879e32fc Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Fri, 6 Jan 2017 15:33:02 +0900
Subject: [PATCH 7/7] Fix some issues with views and partitioned tables
Automatically updatable views failed to handle partitioned tables.
Once that's fixed, WITH CHECK OPTIONS wouldn't work correctly without
them constraint expressions having been suitably converted for each
partition (think map_partition_varattnos on Vars in the expressions
just like with regular check constraints).
Reported by: n/a
Patch by: Amit Langote
Reports: n/a
---
src/backend/executor/execMain.c | 34 +++++++++++++++++++++++
src/backend/executor/nodeModifyTable.c | 40 +++++++++++++++++++++++++++
src/backend/rewrite/rewriteHandler.c | 3 +-
src/test/regress/expected/updatable_views.out | 24 ++++++++++++++++
src/test/regress/sql/updatable_views.sql | 19 +++++++++++++
5 files changed, 119 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 060b036ddf..634a02bd8e 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -2000,6 +2000,40 @@ ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo,
Bitmapset *insertedCols;
Bitmapset *updatedCols;
+ /*
+ * 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);
+ TupleDesc old_tupdesc = RelationGetDescr(rel);
+ TupleConversionMap *map;
+
+ rel = resultRelInfo->ri_PartitionRoot;
+ tupdesc = RelationGetDescr(rel);
+ /* a reverse map */
+ map = convert_tuples_by_name(old_tupdesc, tupdesc, false,
+ gettext_noop("could not convert row type"));
+ if (map != NULL)
+ {
+ tuple = do_convert_tuple(tuple, map);
+ ExecStoreTuple(tuple, slot, InvalidBuffer, false);
+ }
+ }
+
+ /*
+ * choose the correct relation to build val_desc from the
+ * tuple contained in orig_slot
+ */
+ if (resultRelInfo->ri_PartitionRoot)
+ {
+ rel = resultRelInfo->ri_PartitionRoot;
+ tupdesc = RelationGetDescr(rel);
+ }
+
switch (wco->kind)
{
/*
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index f46070fde7..5ccf2321de 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1780,6 +1780,46 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
}
/*
+ * Build WITH CHECK OPTION constraints for each leaf partition rel.
+ * Note that we didn't build the returningList for each partition within
+ * the planner, but simple translation of the varattnos for each partition
+ * will suffice. This only occurs for the INSERT case; UPDATE/DELETE are
+ * handled above.
+ */
+ if (node->withCheckOptionLists != NIL && mtstate->mt_num_partitions > 0)
+ {
+ List *wcoList;
+
+ Assert(operation == CMD_INSERT);
+ resultRelInfo = mtstate->mt_partitions;
+ wcoList = linitial(node->withCheckOptionLists);
+ for (i = 0; i < mtstate->mt_num_partitions; i++)
+ {
+ Relation partrel = resultRelInfo->ri_RelationDesc;
+ List *mapped_wcoList;
+ List *wcoExprs = NIL;
+ ListCell *ll;
+
+ /* varno = node->nominalRelation */
+ mapped_wcoList = map_partition_varattnos(wcoList,
+ node->nominalRelation,
+ partrel, rel);
+ foreach(ll, mapped_wcoList)
+ {
+ WithCheckOption *wco = (WithCheckOption *) lfirst(ll);
+ ExprState *wcoExpr = ExecInitExpr((Expr *) wco->qual,
+ mtstate->mt_plans[i]);
+
+ wcoExprs = lappend(wcoExprs, wcoExpr);
+ }
+
+ resultRelInfo->ri_WithCheckOptions = mapped_wcoList;
+ resultRelInfo->ri_WithCheckOptionExprs = wcoExprs;
+ resultRelInfo++;
+ }
+ }
+
+ /*
* Initialize RETURNING projections if needed.
*/
if (node->returningLists)
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index d1ff3b20b6..d3e44fb135 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -2249,7 +2249,8 @@ view_query_is_auto_updatable(Query *viewquery, bool check_cols)
if (base_rte->rtekind != RTE_RELATION ||
(base_rte->relkind != RELKIND_RELATION &&
base_rte->relkind != RELKIND_FOREIGN_TABLE &&
- base_rte->relkind != RELKIND_VIEW))
+ base_rte->relkind != RELKIND_VIEW &&
+ base_rte->relkind != RELKIND_PARTITIONED_TABLE))
return gettext_noop("Views that do not select from a single table or view are not automatically updatable.");
if (base_rte->tablesample)
diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out
index 6fba613f0f..b6b61422bf 100644
--- a/src/test/regress/expected/updatable_views.out
+++ b/src/test/regress/expected/updatable_views.out
@@ -2465,3 +2465,27 @@ ERROR: new row violates check option for view "v1"
DETAIL: Failing row contains (-1, invalid).
DROP VIEW v1;
DROP TABLE t1;
+-- check that an auto-updatable view on a partitioned table works correctly
+create table p (a int, b int) partition by range (a, b);
+create table p1 (b int not null, 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;
+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);
+create view pv as select * from p;
+insert into pv values (1, 2);
+select tableoid::regclass, * from p;
+ tableoid | a | b
+----------+---+---
+ p11 | 1 | 2
+(1 row)
+
+create view pv_wco as select * from p where a = 0 with check option;
+insert into pv_wco values (1, 2);
+ERROR: new row violates check option for view "pv_wco"
+DETAIL: Failing row contains (1, 2).
+drop view pv, pv_wco;
+drop table p, p1, p11;
diff --git a/src/test/regress/sql/updatable_views.sql b/src/test/regress/sql/updatable_views.sql
index bb9a3a6174..cd4f5a811d 100644
--- a/src/test/regress/sql/updatable_views.sql
+++ b/src/test/regress/sql/updatable_views.sql
@@ -1112,3 +1112,22 @@ INSERT INTO v1 VALUES (-1, 'invalid'); -- should fail
DROP VIEW v1;
DROP TABLE t1;
+
+-- check that an auto-updatable view on a partitioned table works correctly
+create table p (a int, b int) partition by range (a, b);
+create table p1 (b int not null, 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;
+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);
+
+create view pv as select * from p;
+insert into pv values (1, 2);
+select tableoid::regclass, * from p;
+create view pv_wco as select * from p where a = 0 with check option;
+insert into pv_wco values (1, 2);
+drop view pv, pv_wco;
+drop table p, p1, p11;
--
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
