agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Andres Freund <andres@anarazel.de>
Cc: Pavan Deolasee <pavan.deolasee@gmail.com>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>
Cc: Peter Geoghegan <pg@bowt.ie>
Subject: Re: ON CONFLICT DO UPDATE for partitioned tables
Date: Fri, 23 Mar 2018 20:02:14 +0900
Message-ID: <5AB4DEB6.2020901@lab.ntt.co.jp> (raw)
In-Reply-To: <d50382eb-79ba-66ba-4382-28858426fc99@lab.ntt.co.jp>
References: <20180318041715.xvn6lwiv23pcdbny@alvherre.pgsql>
<5AAFB415.8070305@lab.ntt.co.jp>
<f484764f-fe76-eb56-6e4d-889041d62ad4@lab.ntt.co.jp>
<5AB0F633.2030007@lab.ntt.co.jp>
<d50382eb-79ba-66ba-4382-28858426fc99@lab.ntt.co.jp>
(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
view thread (53+ messages) latest in thread
Message-ID: <5AB4DEB6.2020901@lab.ntt.co.jp>
Permalink: ../5AB4DEB6.2020901@lab.ntt.co.jp/
Also on: postgresql.org/message-id/5AB4DEB6.2020901@lab.ntt.co.jp
reply
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, alvherre@alvh.no-ip.org, andres@anarazel.de, pavan.deolasee@gmail.com, pg@bowt.ie
Subject: Re: ON CONFLICT DO UPDATE for partitioned tables
In-Reply-To: <5AB4DEB6.2020901@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 agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox