Re: [HACKERS] UPDATE of partition key

On 21 November 2017 at 17:24, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
> On 13 November 2017 at 18:25, David Rowley <david.rowley@2ndquadrant.com> wrote:
>>
>> 30. The following chunk of code is giving me a headache trying to
>> verify which arrays are which size:
>>
>> ExecSetupPartitionTupleRouting(rel,
>>    mtstate->resultRelInfo,
>>    (operation == CMD_UPDATE ? nplans : 0),
>>    node->nominalRelation,
>>    estate,
>>    &partition_dispatch_info,
>>    &partitions,
>>    &partition_tupconv_maps,
>>    &subplan_leaf_map,
>>    &partition_tuple_slot,
>>    &num_parted, &num_partitions);
>> mtstate->mt_partition_dispatch_info = partition_dispatch_info;
>> mtstate->mt_num_dispatch = num_parted;
>> mtstate->mt_partitions = partitions;
>> mtstate->mt_num_partitions = num_partitions;
>> mtstate->mt_parentchild_tupconv_maps = partition_tupconv_maps;
>> mtstate->mt_subplan_partition_offsets = subplan_leaf_map;
>> mtstate->mt_partition_tuple_slot = partition_tuple_slot;
>> mtstate->mt_root_tuple_slot = MakeTupleTableSlot();
>>
>> I know this patch is not completely responsible for it, but you're not
>> making things any better.
>>
>> Would it not be better to invent some PartitionTupleRouting struct and
>> make that struct a member of ModifyTableState and CopyState, then just
>> pass the pointer to that struct to ExecSetupPartitionTupleRouting()
>> and have it fill in the required details? I think the complexity of
>> this is already on the high end, I think you really need to do the
>> refactor before this gets any worse.
>>
>
>Ok. I am currently working on doing this change. So not yet included in the attached patch. Will send yet another revised patch for this change.

Attached incremental patch encapsulate_partinfo.patch (to be applied
over the latest v25 patch) contains changes to move all the
partition-related information into new structure
PartitionTupleRouting. Further to that, I also moved
PartitionDispatchInfo into this structure. So it looks like this :

typedef struct PartitionTupleRouting
{
PartitionDispatch *partition_dispatch_info;
int num_dispatch;
ResultRelInfo **partitions;
int num_partitions;
TupleConversionMap **parentchild_tupconv_maps;
int    *subplan_partition_offsets;
TupleTableSlot *partition_tuple_slot;
TupleTableSlot *root_tuple_slot;
} PartitionTupleRouting;

So this structure now encapsulates *all* the
partition-tuple-routing-related information. So ModifyTableState now
has only one tuple-routing-related field 'mt_partition_tuple_routing'.
It is changed like this :

@@ -976,24 +976,14 @@ typedef struct ModifyTableState
        TupleTableSlot *mt_existing;    /* slot to store existing
target tuple in */
        List       *mt_excludedtlist;   /* the excluded pseudo
relation's tlist  */
        TupleTableSlot *mt_conflproj;   /* CONFLICT ... SET ...
projection target */
-       struct PartitionDispatchData **mt_partition_dispatch_info;
-       /* Tuple-routing support info */
-       int                     mt_num_dispatch;        /* Number of
entries in the above array */
-       int                     mt_num_partitions;      /* Number of
members in the following
-
  * arrays */
-       ResultRelInfo **mt_partitions;  /* Per partition result
relation pointers */
-       TupleTableSlot *mt_partition_tuple_slot;
-       TupleTableSlot *mt_root_tuple_slot;
+       struct PartitionTupleRouting *mt_partition_tuple_routing; /*
Tuple-routing support info */
        struct TransitionCaptureState *mt_transition_capture;
        /* controls transition table population for specified operation */
        struct TransitionCaptureState *mt_oc_transition_capture;
        /* controls transition table population for INSERT...ON
CONFLICT UPDATE */
-       TupleConversionMap **mt_parentchild_tupconv_maps;
-       /* Per partition map for tuple conversion from root to leaf */
        TupleConversionMap **mt_childparent_tupconv_maps;
        /* Per plan/partition map for tuple conversion from child to root */
        bool            mt_is_tupconv_perpart;  /* Is the above map
per-partition ? */
-       int             *mt_subplan_partition_offsets;
        /* Stores position of update result rels in leaf partitions */
 } ModifyTableState;

So the code in nodeModifyTable.c (and similar code in copy.c) is
accordingly adjusted to use mtstate->mt_partition_tuple_routing.

The places where we use (mtstate->mt_partition_dispatch_info != NULL)
condition to run tuple-routing code, I have replaced it with
mtstate->mt_partition_tuple_routing != NULL.

If you are ok with the incremental patch, I can extract this change
into a separate preparatory patch to be applied on PG master.

Thanks
-Amit Khandekar