From: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: non-bulk inserts and tuple routing
Date: Fri, 16 Feb 2018 12:41:23 +0900
Message-ID: <5A8652E3.9000009@lab.ntt.co.jp> (raw)
In-Reply-To: <c0c02a3f-5eba-77c5-5476-95e1f003623e@lab.ntt.co.jp>
References: <8975331d-d961-cbdd-f862-fdd3d97dc2d0@lab.ntt.co.jp>
<f3a0d60e-1f60-f2ea-dd6e-89c397e4ecf4@lab.ntt.co.jp>
<CA+TgmoZM7S8cakW4j4b5xOz=p3ZOaMJTK-zd8ca8Gh9gY-17Tw@mail.gmail.com>
<56c9f77b-830c-4652-233e-a1c5fa5a9938@lab.ntt.co.jp>
<8f53a342-e0df-4fc9-7422-c3c3c601117c@lab.ntt.co.jp>
<5A69A3B7.10403@lab.ntt.co.jp>
<37f7cc10-def8-b2d0-615d-dfe035e7b1ee@lab.ntt.co.jp>
<5A70396A.8090400@lab.ntt.co.jp>
<8399fd28-07c2-3cbc-f807-f89cdf2f98f5@lab.ntt.co.jp>
<5A704055.40207@lab.ntt.co.jp>
<5A7443DF.1010408@lab.ntt.co.jp>
<14870d0e-d9d4-2ab9-ddfe-182b3c8abfca@lab.ntt.co.jp>
<5A783549.4020409@lab.ntt.co.jp>
<5A7AD6B5.3020304@lab.ntt.co.jp>
<5A7C2364.9040700@lab.ntt.co.jp>
<481cc9d5-2dd8-b3f4-23b0-ecbccd8a85e0@lab.ntt.co.jp>
<5A7D91F3.9050309@lab.ntt.co.jp>
<4f144297-ba52-9f87-1f7a-d44fc7006d80@lab.ntt.co.jp>
<5A8578C1.20308@lab.ntt.co.jp>
<c0c02a3f-5eba-77c5-5476-95e1f003623e@lab.ntt.co.jp>
(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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: fujita.etsuro@lab.ntt.co.jp, Langote_Amit_f8@lab.ntt.co.jp, pgsql-hackers@lists.postgresql.org
Subject: Re: non-bulk inserts and tuple routing
In-Reply-To: <5A8652E3.9000009@lab.ntt.co.jp>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox