(2018/02/16 10:49), Amit Langote wrote:
On 2018/02/15 21:10, Etsuro Fujita wrote:
here are some minor comments:
o On changes to ExecCleanupTupleRouting:
- ExecCloseIndices(resultRelInfo);
- heap_close(resultRelInfo->ri_RelationDesc, NoLock);
+ if (resultRelInfo)
+ {
+ ExecCloseIndices(resultRelInfo);
+ heap_close(resultRelInfo->ri_RelationDesc, NoLock);
+ }
You might check at the top of the loop whether resultRelInfo is NULL and
if so do continue. I think that would save cycles a bit.
Good point, done.
Thanks.
o On ExecInitPartitionInfo:
+ int firstVarno;
+ Relation firstResultRel;
My old compiler got "variable may be used uninitialized" warnings.
Fixed. Actually, I moved those declarations from out here into the blocks
where they're actually needed.
OK, my compiler gets no warnings now.
+ /*
+ * Build the RETURNING projection if any for the partition. Note that
+ * we didn't build the returningList for each partition within the
+ * planner, but simple translation of the varattnos will suffice.
+ * This only occurs for the INSERT case; in the UPDATE/DELETE cases,
+ * ExecInitModifyTable() would've initialized this.
+ */
I think the last comment should be the same as for WCO lists: "This only
occurs for the INSERT case or in the case of UPDATE for which we didn't
find a result rel above to reuse."
Fixed. The "above" is no longer needed, because there is no code left in
ExecInitPartitionInfo() to find UPDATE result rels to reuse. That code is
now in ExecSetupPartitionTupleRouting().
OK, that makes sense.
+ /*
+ * Initialize result tuple slot and assign its rowtype using the
first
+ * RETURNING list. We assume the rest will look the same.
+ */
+ tupDesc = ExecTypeFromTL(returningList, false);
+
+ /* Set up a slot for the output of the RETURNING projection(s) */
+ ExecInitResultTupleSlot(estate,&mtstate->ps);
+ ExecAssignResultType(&mtstate->ps, tupDesc);
+ slot = mtstate->ps.ps_ResultTupleSlot;
+
+ /* Need an econtext too */
+ if (mtstate->ps.ps_ExprContext == NULL)
+ ExecAssignExprContext(estate,&mtstate->ps);
+ econtext = mtstate->ps.ps_ExprContext;
Do we need this initialization? I think we would already have the slot
and econtext initialized when we get here.
I think you're right. If node->returningLists is non-NULL at all,
ExecInitModifyTable() would've initialized the needed slot and expression
context. I added Assert()s to that affect.
OK, but one thing I'd like to ask is:
+ /*
+ * Use the slot that would have been set up in ExecInitModifyTable()
+ * for the output of the RETURNING projection(s). Just make sure to
+ * assign its rowtype using the RETURNING list.
+ */
+ Assert(mtstate->ps.ps_ResultTupleSlot != NULL);
+ tupDesc = ExecTypeFromTL(returningList, false);
+ ExecAssignResultType(&mtstate->ps, tupDesc);
+ slot = mtstate->ps.ps_ResultTupleSlot;
Do we need that assignment here?
Attached updated patch.
Thanks for updating the patch!
Best regards,
Etsuro Fujita
