On 2017/01/20 4:18, Robert Haas wrote:
> On Thu, Jan 19, 2017 at 12:15 AM, Amit Langote wrote:
>> 0002-Set-ecxt_scantuple-correctly-for-tuple-routing.patch
>>
>> In 2ac3ef7a01df859c62d0a02333b646d65eaec5ff, we changed things so that
>> it's possible for a different TupleTableSlot to be used for partitioned
>> tables at successively lower levels. If we do end up changing the slot
>> from the original, we must update ecxt_scantuple to point to the new one
>> for partition key of the tuple to be computed correctly.
>>
>> Last posted here:
>> https://www.postgresql.org/message-id/99fbfad6-b89b-9427-a6ca-197aad98c48e%40lab.ntt.co.jp
>
> Why does FormPartitionKeyDatum need this? Could that be documented in
> a comment in here someplace, perhaps the header comment to
> FormPartitionKeyDatum?
FormPartitionKeyDatum() computes partition key expressions (if any) and
the expression evaluation machinery expects ecxt_scantuple to point to the
tuple to extract attribute values from.
FormPartitionKeyDatum() already has a tiny comment, which it seems is the
only thing we could say here about this there:
* the ecxt_scantuple slot of estate's per-tuple expr context must point to
* the heap tuple passed in.
In get_partition_for_tuple() which calls FormPartitionKeyDatum(), the
patch adds this comment (changed it a little from the last version):
+ /*
+ * Extract partition key from tuple. Expression evaluation machinery
+ * that FormPartitionKeyDatum() invokes expects ecxt_scantuple to
+ * point to the correct tuple slot. The slot might have changed from
+ * what was used for the parent table if the table of the current
+ * partitioning level has different tuple descriptor from the parent.
+ * So update ecxt_scantuple accordingly.
+ */
+ ecxt->ecxt_scantuple = slot;
FormPartitionKeyDatum(parent, slot, estate, values, isnull);
It says why we need to change which slot ecxt_scantuple points to.
>> 0004-Fix-some-issues-with-views-and-partitioned-tables.patch
>>
>> Automatically updatable views failed to handle partitioned tables.
>> Once that's fixed, WITH CHECK OPTIONS wouldn't work correctly without
>> the WCO expressions having been suitably converted for each partition
>> (think applying map_partition_varattnos to Vars in the WCO expressions
>> just like with partition constraint expressions).
>
> The changes to execMain.c contain a hunk which has essentially the
> same code twice. That looks like a mistake. Also, the patch doesn't
> compile because convert_tuples_by_name() takes 3 arguments, not 4.
Actually, I realized that and sent the updated version [1] of this patch
that fixed this issue. In the updated version, I removed that code block
(the 2 copies of it), because we are still discussing what to do about
showing tuples in constraint violation (in this case, WITH CHECK OPTION
violation) messages. Anyway, attached here again.
>> 0006-Avoid-tuple-coversion-in-common-partitioning-cases.patch
>>
>> 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.
>
> + Assert(!consider_typeid && indesc->tdhasoid == outdesc->tdhasoid);
>
> I think you mean Assert(consider_typeid || indesc->tdhasoid ==
> outdesc->tdhasoid);
Ah, you're right.
> But I wonder why we don't instead just change this function to
> consider tdhasoid rather than tdtypeid. I mean, if the only point of
> comparing the type OIDs is to find out whether the table-has-OIDs
> setting matches, we could instead test that directly and avoid needing
> to pass an extra argument. I wonder if there's some other reason this
> code is there which is not documented in the comment...
With the following patch, regression tests run fine:
if (indesc->natts == outdesc->natts &&
- indesc->tdtypeid == outdesc->tdtypeid)
+ indesc->tdhasoid != outdesc->tdhasoid)
{
If checking tdtypeid (instead of tdhasoid directly) has some other
consideration, I'd would have seen at least some tests broken by this
change. So, if we are to go with this, I too prefer it over my previous
proposal to add an argument to convert_tuples_by_name(). Attached 0003
implements the above approach.
> Phew. Thanks for all the patches, sorry I'm having trouble keeping up.
Thanks a lot for taking time to look through them and committing.
Thanks,
Amit
[1]
https://www.postgresql.org/message-id/92fe2a71-5eb7-ee8d-53ef-cfd5a65dfc3d%40lab.ntt.co.jp
>From c7088290221a9fe0818139145b7bdf6731bc419a Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Wed, 28 Dec 2016 10:10:26 +0900
Subject: [PATCH 1/3] Set ecxt_scantuple correctly for tuple-routing
In 2ac3ef7a01df859c62d0a02333b646d65eaec5ff, we changed things so that
it's possible for a different TupleTableSlot to be used for partitioned
tables at successively lower levels. If we do end up changing the slot
from the original, we must update ecxt_scantuple to point to the new one
for partition key of the tuple to be computed correctly.
Also update the regression tests so that the code manipulating
ecxt_scantuple is covered.
Reported by: Rajkumar Raghuwanshi
Patch by: Amit Langote
Reports: https://www.postgresql.org/message-id/CAKcux6%3Dm1qyqB2k6cjniuMMrYXb75O-MB4qGQMu8zg-iGGLjDw%40mail.gmail.com
---
src/backend/catalog/partition.c | 30 +++++++++++++++++++++++-------
src/backend/executor/execMain.c | 2 --
src/test/regress/expected/insert.out | 2 +-
src/test/regress/sql/insert.sql | 2 +-
4 files changed, 25 insertions(+), 11 deletions(-)
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 3f8a950f37..9b2a71d0d5 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1661,7 +1661,10 @@ get_partition_for_tuple(PartitionDispatch *pd,
bool isnull[PARTITION_MAX_KEYS];
int cur_offset,
cur_index;
- int i;
+ int i,
+ result;
+ ExprContext *ecxt = GetPerTupleExprContext(estate);
+ TupleTableSlot *ecxt_scantuple_old = ecxt->ecxt_scantuple;
/* start with the root partitioned table */
parent = pd[0];
@@ -1690,7 +1693,15 @@ get_partition_for_tuple(PartitionDispatch *pd,
slot = myslot;
}
- /* Extract partition key from tuple */
+ /*
+ * Extract partition key from tuple. Expression evaluation machinery
+ * that FormPartitionKeyDatum() invokes expects ecxt_scantuple to
+ * point to the correct tuple slot. The slot might have changed from
+ * what was used for the parent table if the table of the current
+ * partitioning level has different tuple descriptor from the parent.
+ * So update ecxt_scantuple accordingly.
+ */
+ ecxt->ecxt_scantuple = slot;
FormPartitionKeyDatum(parent, slot, estate, values, isnull);
if (key->strategy == PARTITION_STRATEGY_RANGE)
@@ -1745,16 +1756,21 @@ get_partition_for_tuple(PartitionDispatch *pd,
*/
if (cur_index < 0)
{
+ result = -1;
*failed_at = RelationGetRelid(parent->reldesc);
- return -1;
+ break;
}
- else if (parent->indexes[cur_index] < 0)
- parent = pd[-parent->indexes[cur_index]];
- else
+ else if (parent->indexes[cur_index] >= 0)
+ {
+ result = parent->indexes[cur_index];
break;
+ }
+ else
+ parent = pd[-parent->indexes[cur_index]];
}
- return parent->indexes[cur_index];
+ ecxt->ecxt_scantuple = ecxt_scantuple_old;
+ return result;
}
/*
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index e6edcc06c2..8cb9691056 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -3191,9 +3191,7 @@ ExecFindPartition(ResultRelInfo *resultRelInfo, PartitionDispatch *pd,
{
int result;
Oid failed_at;
- ExprContext *econtext = GetPerTupleExprContext(estate);
- econtext->ecxt_scantuple = slot;
result = get_partition_for_tuple(pd, slot, estate, &failed_at);
if (result < 0)
{
diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out
index 538fe5b181..81af3ef497 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -320,7 +320,7 @@ drop table part_ee_ff, part_gg2_2, part_gg2_1, part_gg2, part_gg1, part_gg;
drop table part_aa_bb, part_cc_dd, part_null, list_parted;
-- 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 p1 (b int not null, a int not null) partition by range ((b+0));
create table p11 (like p1);
alter table p11 drop a;
alter table p11 add a int;
diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql
index 3d5138a8e0..454e1ce2e7 100644
--- a/src/test/regress/sql/insert.sql
+++ b/src/test/regress/sql/insert.sql
@@ -193,7 +193,7 @@ drop table part_aa_bb, part_cc_dd, part_null, list_parted;
-- 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 p1 (b int not null, a int not null) partition by range ((b+0));
create table p11 (like p1);
alter table p11 drop a;
alter table p11 add a int;
--
2.11.0
>From 690a4aae05af5e015a9edfe501e957083205a86b Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Fri, 6 Jan 2017 15:33:02 +0900
Subject: [PATCH 2/3] 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
the WCO expressions having been suitably converted for each partition
(think applying map_partition_varattnos to Vars in the WCO expressions
just like with partition constraint expressions).
Reported by: n/a
Patch by: Amit Langote
Reports: n/a
---
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 +++++++++++++
4 files changed, 85 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 2ac7407318..aa17aa0eb3 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1778,6 +1778,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 withCheckOptionList 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 cases 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 2da3c069e1..2ae3613cec 100644
--- a/src/test/regress/expected/updatable_views.out
+++ b/src/test/regress/expected/updatable_views.out
@@ -2367,3 +2367,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 (2, 1).
+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 ffc64d2de9..3c19edc8f7 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
>From 5ac4d27ebf34a29334dc5822e179559098c534f4 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Mon, 26 Dec 2016 17:44:14 +0900
Subject: [PATCH 3/3] 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 | 9 +++++----
src/backend/catalog/partition.c | 3 +--
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/backend/access/common/tupconvert.c b/src/backend/access/common/tupconvert.c
index b17ceafa6e..7c5427e987 100644
--- a/src/backend/access/common/tupconvert.c
+++ b/src/backend/access/common/tupconvert.c
@@ -214,12 +214,13 @@ convert_tuples_by_name(TupleDesc indesc,
attrMap = convert_tuples_by_name_map(indesc, outdesc, msg);
/*
- * 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.)
+ * Check to see if the map is one-to-one, in which case we need not do
+ * tuple conversion. That's not enough though if one of either source or
+ * destination (tuples) contains OIDs; we'd need conversion in that case
+ * to inject the right OID into the tuple datum.
*/
if (indesc->natts == outdesc->natts &&
- indesc->tdtypeid == outdesc->tdtypeid)
+ 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 9b2a71d0d5..f1d888a336 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1682,12 +1682,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;
--
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
