Fujita-san,
Thanks for the review.
On 2018/02/15 21:10, Etsuro Fujita wrote:
> (2018/02/13 10:12), Amit Langote wrote:
>> Updated patch is attached.
>
> Thanks, 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.
> 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.
> + /*
> + * 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().
> + /*
> + * 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.
> Other than that, the patch looks good to me.
>
> Sorry for the delay.
No problem! Thanks again.
Attached updated patch.
Thanks,
Amit
