Re: [HACKERS] UPDATE of partition key

On 14 November 2017 at 01:55, David Rowley <david.rowley@2ndquadrant.com> wrote:
> I'll try to continue with the review tomorrow, but I think some other
> reviews are also looming too.

I started looking at this again today. Here's the remainder of my review.

29. ExecSetupChildParentMap gets called here for non-partitioned relations.
Maybe that's not the best function name? The function only seems to do
that when perleaf is True.

Is a leaf a partition of a partitioned table? It's not that clear the
meaning here.

/*
* If we found that we need to collect transition tuples then we may also
* need tuple conversion maps for any children that have TupleDescs that
* aren't compatible with the tuplestores.  (We can share these maps
* between the regular and ON CONFLICT cases.)
*/
if (mtstate->mt_transition_capture != NULL ||
mtstate->mt_oc_transition_capture != NULL)
{
int numResultRelInfos;

numResultRelInfos = (mtstate->mt_partition_tuple_slot != NULL ?
mtstate->mt_num_partitions :
mtstate->mt_nplans);

ExecSetupChildParentMap(mtstate, targetRelInfo, numResultRelInfos,
(mtstate->mt_partition_dispatch_info != NULL));


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.

The signature of the function is a bit scary!

extern void ExecSetupPartitionTupleRouting(Relation rel,
   ResultRelInfo *update_rri,
   int num_update_rri,
   Index resultRTindex,
   EState *estate,
   PartitionDispatch **pd,
   ResultRelInfo ***partitions,
   TupleConversionMap ***tup_conv_maps,
   int **subplan_leaf_map,
   TupleTableSlot **partition_tuple_slot,
   int *num_parted, int *num_partitions);

What do you think?

31. The following code seems incorrect:

/*
* If this is an UPDATE and a BEFORE UPDATE trigger is present, we may
* need to do update tuple routing.
*/
if (resultRelInfo->ri_TrigDesc &&
resultRelInfo->ri_TrigDesc->trig_update_before_row &&
operation == CMD_UPDATE)
update_tuple_routing_needed = true;

Shouldn't this be setting update_tuple_routing_needed to false if
there are no before row update triggers? Otherwise, you're setting it
to true regardless of if there are any partition key columns being
UPDATEd. That would make the work you're doing in
inheritance_planner() to set part_cols_updated a waste of time.

Also, this bit of code is a bit confused.

/* Decide whether we need to perform update tuple routing. */
if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
update_tuple_routing_needed = false;

/*
* Build state for tuple routing if it's an INSERT or if it's an UPDATE of
* partition key.
*/
if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE &&
(operation == CMD_INSERT || update_tuple_routing_needed))


The first if test would not be required if you fixed the code where
you set update_tuple_routing_needed = true regardless if its a
partitioned table or not.

So basically, you need to take the node->part_cols_updated from the
planner, if that's true then perform your test for before row update
triggers, set a bool to false if there are none, then proceed to setup
the partition tuple routing for partition table inserts or if your
bool is still true. Right?

32. "WCO" abbreviation is not that common and might need to be expanded.

* Below are required as reference objects for mapping partition
* attno's in expressions such as WCO and RETURNING.

Searching for other comments which mention "WCO" they're all around
places that is easy to understand they mean "With Check Option", e.g.
next to a variable with a more descriptive name. That's not the case
here.

33. "are anyway newly allocated", should "anyway" be "always"?
Otherwise, it does not make sense.

* If this result rel is one of the subplan result rels, let
* ExecEndPlan() close it. For INSERTs, this does not apply because
* all leaf partition result rels are anyway newly allocated.

34. Comment added which mentions a member that does not exist.

 * all_part_cols contains all attribute numbers from the parent that are
 * used as partitioning columns by the parent or some descendent which is
 * itself partitioned.
 *

I've not looked at the test coverage as I see Thomas has been looking
at that in some detail.

I'm going to set this patch as waiting for author now.

Thanks again for working on this.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services