Re: [HACKERS] UPDATE of partition key

On 10 January 2018 at 02:30, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Jan 5, 2018 at 3:25 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Fri, Jan 5, 2018 at 7:12 AM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
>>> The above patch is to be applied over the last remaining preparatory
>>> patch, now named (and attached) :
>>> 0001-Refactor-CheckConstraint-related-code.patch
>>
>> Committed that one, too.
>
> Some more comments on the main patch:
>
> I don't really like the fact that ExecCleanupTupleRouting() now takes
> a ModifyTableState as an argument, particularly because of the way
> that is using that argument.  To figure out whether a ResultRelInfo
> was pre-existing or one it created, it checks whether the pointer
> address of the ResultRelInfo is >= mtstate->resultRelInfo and <
> mtstate->resultRelInfo + mtstate->mt_nplans.  However, that means that
> ExecCleanupTupleRouting() ends up knowing about the memory allocation
> pattern used by ExecInitModifyTable(), which seems like a slightly
> dangerous amount of action at a distance.  I think it would be better
> for the PartitionTupleRouting structure to explicitly indicate which
> ResultRelInfos should be closed, for example by storing a Bitmapset
> *input_partitions.  (Here, by "input", I mean "provided from the
> mtstate rather than created by the PartitionTupleRouting structure;
> other naming suggestions welcome.)  When
> ExecSetupPartitionTupleRouting latches onto a partition, it can do
> proute->input_partitions = bms_add_member(proute->input_partitons, i).
> In ExecCleanupTupleRouting, it can do if
> (bms_is_member(proute->input_partitions, i)) continue.

Did the changes. But, instead of a new bitmapet, I used the offset
array for the purpose. As per our parallel discussion on
tup-conversion maps, it is almost finalized that the subplan-partition
offset map is good to have. So I have used that offset array to
determine whether a partition is present in the subplan. I used the
assumption that subplan and partition array have their partitions in
the same order.

>
> We have a test, in the regression test suite for file_fdw, which
> generates the message "cannot route inserted tuples to a foreign
> table".  I think we should have a similar test for the case where an
> UPDATE tries to move a tuple from a regular partition to a foreign
> table partition.

Added an UPDATE scenario in contrib/file_fdw/input/file_fdw.source.

> I'm not sure if it should fail with the same error
> or a different one, but I think we should have a test that it fails
> cleanly and with a nice error message of some sort.

The update-tuple-routing goes through the same ExecInsert() code, so
it fails at the same place with the same error message.

>
> The comment for get_partitioned_child_rels() claims that it sets
> is_partition_key_update, but it really sets *is_partition_key_update.
> And I think instead of "is a partition key" it should say "is used in
> the partition key either of the relation whose RTI is specified or of
> any child relation."  I propose "used in" instead of "is" because
> there can be partition expressions, and the rest is to clarify that
> child partition keys matter.

Fixed.

>
> create_modifytable_path uses partColsUpdated rather than
> partKeyUpdated, which actually seems like better terminology.  I
> propose partKeyUpdated -> partColsUpdated everywhere.  Also, why use
> is_partition_key_update for basically the same thing in some other
> places?  I propose changing that to partColsUpdated as well.

Done.

>
> The capitalization of the first comment hunk in execPartition.h is strange.

I think you are referring to :
 * subplan_partition_offsets int Array ordered by UPDATE subplans. Each
Changed Array to array. Didn't change UPDATE.

Attached v36 patch.