Re: ON CONFLICT DO UPDATE for partitioned tables - Mailing list pgsql-hackers

(2018/03/20 13:30), Amit Langote wrote:
> On 2018/03/19 21:59, Etsuro Fujita wrote:
>> (2018/03/18 13:17), Alvaro Herrera wrote:
>>> Alvaro Herrera wrote:
>>> The only thing that I remain unhappy about this patch is the whole
>>> adjust_and_expand_partition_tlist() thing.  I fear we may be doing
>>> redundant and/or misplaced work.  I'll look into it next week.
>>
>> I'm still reviewing the patches, but I really agree on that point.  As
>> Pavan mentioned upthread, the onConflictSet tlist for the root parent,
>> from which we create a translated onConflictSet tlist for a partition,
>> would have already been processed by expand_targetlist() to contain all
>> missing columns as well, so I think we could create the tlist for the
>> partition by simply re-ordering the expression-converted tlist (ie,
>> conv_setproj) based on the conversion map for the partition.  The Attached
>> defines a function for that, which could be called, instead of calling
>> adjust_and_expand_partition_tlist().  This would allow us to get rid of
>> planner changes from the patches.  Maybe I'm missing something, though.
>
> Thanks for the patch.  I can confirm your proposed
> adjust_onconflictset_tlist() is enough to replace adjust_inherited_tlist()
> + expand_targetlist() combination (aka
> adjust_and_expand_partition_tlist()), thus rendering the planner changes
> in this patch unnecessary.  I tested it with a partition tree involving
> partitions of varying attribute numbers (dropped columns included) and it
> seems to work as expected (as also exercised in regression tests) as shown
> below.

Thanks for testing!

> I have incorporated your patch in the main patch after updating the
> comments a bit.  Also, now that 6666ee49f49 is in [1], the transition
> table related tests I proposed yesterday pass nicely.  Instead of posting
> as a separate patch, I have merged it with the main patch.  So now that
> planner refactoring is unnecessary, attached is just one patch.

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.

  ExecOnConflictUpdate(ModifyTableState *mtstate,
                       ResultRelInfo *resultRelInfo,
+                     TupleDesc onConflictSetTupdesc,
                       ItemPointer conflictTid,
                       TupleTableSlot *planSlot,
                       TupleTableSlot *excludedSlot,
@@ -1419,6 +1459,7 @@ ExecOnConflictUpdate(ModifyTableState *mtstate,
      ExecCheckHeapTupleVisible(estate, &tuple, buffer);

      /* Store target's existing tuple in the state's dedicated slot */
+    ExecSetSlotDescriptor(mtstate->mt_existing, RelationGetDescr(relation));
      ExecStoreTuple(&tuple, mtstate->mt_existing, buffer, false);

      /*
@@ -1462,6 +1503,7 @@ ExecOnConflictUpdate(ModifyTableState *mtstate,
      }

      /* Project the new tuple version */
+    ExecSetSlotDescriptor(mtstate->mt_conflproj, onConflictSetTupdesc);
      ExecProject(resultRelInfo->ri_onConflictSetProj);

Can we do ExecSetSlotDescriptor for mtstate->mt_existing and 
mtstate->mt_conflproj in ExecPrepareTupleRouting in the case of tuple 
routing?  That would make the API changes to ExecOnConflictUpdate 
unnecessary.

@@ -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.

I'll look at other parts of the patch next.

Best regards,
Etsuro Fujita