Re: [HACKERS] UPDATE of partition key

On 21 November 2017 at 17:24, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com> wrote:
> On 13 November 2017 at 18:25, David Rowley <david(dot)rowley(at)2ndquadrant(dot)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