Re: [HACKERS] UPDATE of partition key - Mailing list pgsql-hackers
From | Amit Khandekar |
---|---|
Subject | Re: [HACKERS] UPDATE of partition key |
Date | |
Msg-id | CAJ3gD9dJu8hJB9wSde149nX436fW+k=t0_enzV5=OQxxNcxVUA@mail.gmail.com Whole thread Raw |
In response to | Re: [HACKERS] UPDATE of partition key (Robert Haas <robertmhaas@gmail.com>) |
List | pgsql-hackers |
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.
Attachment
pgsql-hackers by date: