(2018/03/20 13:30), Amit Langote wrote:
On 2018/03/19 21:59, Etsuro Fujita wrote:
(2018/03/18 13:17), Alvaro Herrera wrote:
Alvaro Herrera wrote:
The only thing that I remain unhappy about this patch is the whole
adjust_and_expand_partition_tlist() thing. I fear we may be doing
redundant and/or misplaced work. I'll look into it next week.
I'm still reviewing the patches, but I really agree on that point. As
Pavan mentioned upthread, the onConflictSet tlist for the root parent,
from which we create a translated onConflictSet tlist for a partition,
would have already been processed by expand_targetlist() to contain all
missing columns as well, so I think we could create the tlist for the
partition by simply re-ordering the expression-converted tlist (ie,
conv_setproj) based on the conversion map for the partition. The Attached
defines a function for that, which could be called, instead of calling
adjust_and_expand_partition_tlist(). This would allow us to get rid of
planner changes from the patches. Maybe I'm missing something, though.
Thanks for the patch. I can confirm your proposed
adjust_onconflictset_tlist() is enough to replace adjust_inherited_tlist()
+ expand_targetlist() combination (aka
adjust_and_expand_partition_tlist()), thus rendering the planner changes
in this patch unnecessary. I tested it with a partition tree involving
partitions of varying attribute numbers (dropped columns included) and it
seems to work as expected (as also exercised in regression tests) as shown
below.
Thanks for testing!
I have incorporated your patch in the main patch after updating the
comments a bit. Also, now that 6666ee49f49 is in [1], the transition
table related tests I proposed yesterday pass nicely. Instead of posting
as a separate patch, I have merged it with the main patch. So now that
planner refactoring is unnecessary, attached is just one patch.
Here are comments on executor changes in (the latest version of) the patch:
@@ -421,8 +424,18 @@ ExecInsert(ModifyTableState *mtstate,
ItemPointerData conflictTid;
bool specConflict;
List *arbiterIndexes;
+ PartitionTupleRouting *proute =
+
mtstate->mt_partition_tuple_routing;
- arbiterIndexes = node->arbiterIndexes;
+ /* Use the appropriate list of arbiter indexes. */
+ if (mtstate->mt_partition_tuple_routing != NULL)
+ {
+ Assert(partition_index >= 0 && proute != NULL);
+ arbiterIndexes =
+
proute->partition_arbiter_indexes[partition_index];
+ }
+ else
+ arbiterIndexes = node->arbiterIndexes;
To handle both cases the same way, I wonder if it would be better to
have the arbiterindexes list in ResultRelInfo as well, as mentioned by
Alvaro upthread, or to re-add mt_arbiterindexes as before and set it to
proute->partition_arbiter_indexes[partition_index] before we get here,
maybe in ExecPrepareTupleRouting, in the case of tuple routing.
ExecOnConflictUpdate(ModifyTableState *mtstate,
ResultRelInfo *resultRelInfo,
+ TupleDesc onConflictSetTupdesc,
ItemPointer conflictTid,
TupleTableSlot *planSlot,
TupleTableSlot *excludedSlot,
@@ -1419,6 +1459,7 @@ ExecOnConflictUpdate(ModifyTableState *mtstate,
ExecCheckHeapTupleVisible(estate, &tuple, buffer);
/* Store target's existing tuple in the state's dedicated slot */
+ ExecSetSlotDescriptor(mtstate->mt_existing, RelationGetDescr(relation));
ExecStoreTuple(&tuple, mtstate->mt_existing, buffer, false);
/*
@@ -1462,6 +1503,7 @@ ExecOnConflictUpdate(ModifyTableState *mtstate,
}
/* Project the new tuple version */
+ ExecSetSlotDescriptor(mtstate->mt_conflproj, onConflictSetTupdesc);
ExecProject(resultRelInfo->ri_onConflictSetProj);
Can we do ExecSetSlotDescriptor for mtstate->mt_existing and
mtstate->mt_conflproj in ExecPrepareTupleRouting in the case of tuple
routing? That would make the API changes to ExecOnConflictUpdate
unnecessary.
@@ -2368,9 +2419,13 @@ ExecInitModifyTable(ModifyTable *node, EState
*estate, int eflags)
econtext = mtstate->ps.ps_ExprContext;
relationDesc = resultRelInfo->ri_RelationDesc->rd_att;
- /* initialize slot for the existing tuple */
- mtstate->mt_existing =
- ExecInitExtraTupleSlot(mtstate->ps.state, relationDesc);
+ /*
+ * Initialize slot for the existing tuple. We determine which
+ * tupleDesc to use for this after we have determined which
relation
+ * the insert/update will be applied to, possibly after
performing
+ * tuple routing.
+ */
+ mtstate->mt_existing =
ExecInitExtraTupleSlot(mtstate->ps.state, NULL);
/* carried forward solely for the benefit of explain */
mtstate->mt_excludedtlist = node->exclRelTlist;
@@ -2378,8 +2433,16 @@ ExecInitModifyTable(ModifyTable *node, EState
*estate, int eflags)
/* create target slot for UPDATE SET projection */
tupDesc = ExecTypeFromTL((List *) node->onConflictSet,
relationDesc->tdhasoid);
+ PinTupleDesc(tupDesc);
+ mtstate->mt_conflproj_tupdesc = tupDesc;
+
+ /*
+ * Just like the "existing tuple" slot, we'll defer deciding
which
+ * tupleDesc to use for this slot to a point where tuple
routing has
+ * been performed.
+ */
mtstate->mt_conflproj =
- ExecInitExtraTupleSlot(mtstate->ps.state, tupDesc);
+ ExecInitExtraTupleSlot(mtstate->ps.state, NULL);
If we do ExecInitExtraTupleSlot for mtstate->mt_existing and
mtstate->mt_conflproj in ExecPrepareTupleRouting in the case of tuple
routing, as said above, we wouldn't need this changes. I think doing
that only in the case of tuple routing and keeping this as-is would be
better because that would save cycles in the normal case.
I'll look at other parts of the patch next.
Best regards,
Etsuro Fujita
