Re: [HACKERS] UPDATE of partition key

On Fri, Jan 5, 2018 at 3:25 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Jan 5, 2018 at 7:12 AM, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)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.

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

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.

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

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company