On 2017/06/12 11:51, Tom Lane wrote:
> Amit Langote <langote_amit...@lab.ntt.co.jp> writes:
>> On 2017/06/12 11:09, Joe Conway wrote:
>>> On 06/11/2017 05:35 PM, Tom Lane wrote:
>>>> First guess is that map_partition_varattnos has forgotten to handle
>>>> WithCheckOption.qual.
>
>>> Drat. I'll take a look, but it would probably be good if someone
>>> generally familiar with the partitioned tables patches have a look as well.
>
>> I am looking too. Commit 587cda35ca3 which added that code did add a test
>> in updatable_views.sql, but tests added by this commit have perhaps
>> exposed something not previously covered.
>
> My first guess above is wrong -- the undefined reference is actually
> "mtstate->mt_plans[i]". This is because mtstate->mt_num_partitions is
> more than mtstate->mt_nplans in the failing case, so that the blithe
> assumption that we can use the partition number to index into
> mtstate->mt_plans is certainly wrong.
Yes, the code should never access mtstate->mt_plans[i] for i > 0, because
in this case (the INSERT case), there is only one plan. Unlike
UPDATE/DELETE cases, we don't build plans for every partition in the
INSERT case.
> Don't know how this should
> be corrected, offhand, but do we really have more than one plan
> for a partitioned query?
There can't be, as mentioned, in the INSERT case.
How about something like the attached? The patch makes
mtstate->mt_plans[0] to be passed for all invocations of ExecInitQual() on
WithCheckOption.qual that is being initialized for each partition. Note
that we are passing a PlanState that's based on the root parent table's
properties to initialize WITH CHECK OPTION quals to be checked against
rows that will be inserted into partitions (that is, after tuple-routing).
I tried to come up with a test that would cause the existing (unpatched)
code to crash, but couldn't. How would one define a policy or WITH CHECK
OPTIONS view or something else, such that ExecInitQual() called on
WithCheckOptions.qual would try to access parent planstate and would crash
once parent becomes an invalid pointer (mtstate->mt_plans[i] for i > 0)?
After looking at the code downstream to ExecInitQual(), it seems that the
following cases would cases would cause parent planstate to be accessed:
* Qual references a whole row var
* Qual references an Aggref or a GroupingFunc or a WindowFunc
* Qual references a (Alternative) SubPlan
Thanks,
Amit
From 90fdad27c04db70f78c24e4999cd3aef7c06d1f7 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Mon, 12 Jun 2017 14:35:05 +0900
Subject: [PATCH] Don't pass invalid PlanState pointer to ExecInitQual
Current code in ExecInitModifyTable that initializes
WithCheckOption.qual failed to consider that there *isn't* one
plan per partition in the ModifyTableState.mt_plans[] array.
---
src/backend/executor/nodeModifyTable.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/backend/executor/nodeModifyTable.c
b/src/backend/executor/nodeModifyTable.c
index bf26488c51..89991e34ae 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1841,8 +1841,17 @@ ExecInitModifyTable(ModifyTable *node, EState *estate,
int eflags)
if (node->withCheckOptionLists != NIL && mtstate->mt_num_partitions > 0)
{
List *wcoList;
+ PlanState *plan;
- Assert(operation == CMD_INSERT);
+ /*
+ * In case of INSERT on partitioned tables, there is only plan;
in the
+ * planner we don't build one for each partition. Likewise,
there is
+ * only one WITH CHECK OPTIONS list, not one per partition.
+ */
+ Assert(operation == CMD_INSERT &&
+ list_length(node->withCheckOptionLists) == 1 &&
+ mtstate->mt_nplans == 1);
+ plan = mtstate->mt_plans[0]; /* The only plan */
resultRelInfo = mtstate->mt_partitions;
wcoList = linitial(node->withCheckOptionLists);
for (i = 0; i < mtstate->mt_num_partitions; i++)
@@ -1858,10 +1867,9 @@ ExecInitModifyTable(ModifyTable *node, EState *estate,
int eflags)
partrel, rel);
foreach(ll, mapped_wcoList)
{
- WithCheckOption *wco = (WithCheckOption *)
lfirst(ll);
- ExprState *wcoExpr = ExecInitQual((List *)
wco->qual,
-
mtstate->mt_plans[i]);
+ WithCheckOption *wco =
castNode(WithCheckOption, lfirst(ll));
+ ExprState *wcoExpr = ExecInitQual((List *)
wco->qual, plan);
wcoExprs = lappend(wcoExprs, wcoExpr);
}
--
2.11.0
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
