(2018/03/22 18:31), Amit Langote wrote:
On 2018/03/20 20:53, Etsuro Fujita wrote:
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.
It's a good idea. I somehow missed that Alvaro had already mentioned it.
In HEAD, we now have ri_onConflictSetProj and ri_onConflictSetWhere. I
propose we name the field ri_onConflictArbiterIndexes as done in the
updated patch.
I like that naming.
@@ -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.
Hmm, I think we shouldn't be doing ExecInitExtraTupleSlot in
ExecPrepareTupleRouting, because we shouldn't have more than one instance
of mtstate->mt_existing and mtstate->mt_conflproj slots.
Yeah, I think so too. What I was going to say here is
ExecSetSlotDescriptor, not ExecInitExtraTupleSlot, as you said below.
Sorry about the incorrectness. I guess I was too tired when writing
that comments.
As you also said above, I think you meant to say here that we do
ExecInitExtraTupleSlot only once for both mtstate->mt_existing and
mtstate->mt_conflproj in ExecInitModifyTable and only do
ExecSetSlotDescriptor in ExecPrepareTupleRouting.
That's right.
I have changed it so
that ExecInitModifyTable now both creates the slot and sets the descriptor
for non-tuple-routing cases and only creates but doesn't set the
descriptor in the tuple-routing case.
IMHO I don't see much value in modifying code as such, because we do
ExecSetSlotDescriptor for mt_existing and mt_conflproj in
ExecPrepareTupleRouting for every inserted tuple. So, I would leave
that as-is, to keep that simple.
For ExecPrepareTupleRouting to be able to access the tupDesc of the
onConflictSet target list, I've added ri_onConflictSetProjTupDesc which is
set by ExecInitPartitionInfo on first call for a give partition. This is
also suggested by Pavan in his review.
Seems like a good idea.
Here are some comments on the latest version of the patch:
+ /*
+ * Caller must set mtstate->mt_conflproj's tuple
descriptor to
+ * this one before trying to use it for projection.
+ */
+ tupDesc = ExecTypeFromTL(onconflset, partrelDesc->tdhasoid);
+ leaf_part_rri->ri_onConflictSet->proj =
+ ExecBuildProjectionInfo(onconflset, econtext,
+ mtstate->mt_conflproj,
+ &mtstate->ps, partrelDesc);
ExecBuildProjectionInfo is called without setting the tuple descriptor
of mtstate->mt_conflproj to tupDesc. That might work at least for now,
but I think it's a good thing to set it appropriately to make that
future proof.
+ * This corresponds to a dropped attribute in the partition, for
+ * which we enerate a dummy entry with resno matching the
+ * partition's attno.
s/enerate/generate/
+ * OnConflictSetState
+ *
+ * Contains execution time state of a ON CONFLICT DO UPDATE operation,
which
+ * includes the state of projection, tuple descriptor of the
projection, and
+ * WHERE quals if any.
s/a ON/an ON/
+typedef struct OnConflictSetState
+{ /* for computing ON CONFLICT DO UPDATE SET */
This is nitpicking, but this wouldn't follow the project style, so I
think that needs re-formatting.
I'll look at the patch a little bit more early next week.
Thanks for updating the patch!
Best regards,
Etsuro Fujita
