Re: non-bulk inserts and tuple routing

From: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
To: Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: non-bulk inserts and tuple routing
Date: 2018-02-16 01:49:12
Message-ID: c0c02a3f-5eba-77c5-5476-95e1f003623e@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
v8-0001-During-tuple-routing-initialize-per-partition-obj.patch text/plain 22.3 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Langote 2018-02-16 02:06:12 Re: FOR EACH ROW triggers on partitioned tables
Previous Message Peter Geoghegan 2018-02-16 01:07:08 Re: [HACKERS] MERGE SQL Statement for PG11