On 2017/08/30 12:03, Robert Haas wrote: > On Tue, Aug 29, 2017 at 10:36 PM, Amit Langote > <langote_amit...@lab.ntt.co.jp> wrote: >>> I keep having the feeling that this is a big patch with a small patch >>> struggling to get out. Is it really necessary to change >>> RelationGetPartitionDispatchInfo so much or could you just do a really >>> minimal surgery to remove the code that sets the stuff we don't need? >>> Like this: >> >> Sure, done in the attached updated patch. > > On first glance, that looks pretty good. I'll have a deeper look > tomorrow.
Thanks. > It strikes me that if PartitionTupleRoutingInfo is an > executor structure, we should probably (as a separate patch) relocate > FormPartitionKeyDatum and get_partition_for_tuple to someplace in > src/backend/executor and rename the accordingly; maybe it's time for > an execPartition.c? catalog/partition.c is getting really bug, so I agree. It would be a good idea to introduce an execPartition.c so that future patches in this area (such as executor partition-pruning patch on the horizon) have a convenient place to park their code. Will see if I can make a patch for that. > it's not a bad thing if some of that stuff gets moved elsewhere. A > lingering question in my mind, though, is whether it's really correct > to think of PartitionTupleRoutingInfo as executor-specific. Maybe > we're going to need that for other things too? Hmm. Maybe, a subset of PartitionTupleRoutingInfo's fields are usable outside the executor (only PartitionDispatch, which is exported by partition.h anyway?), but not all of it. For example, fields keystate, tupslot seem pretty executor-specific. In fact, they seem to be required only for tuple routing. One idea is to not introduce PartitionTupleRoutingInfo at all and add its fields directly as ModifyTableState/CopyState fields. We currently have, for example, a mt_partition_tupconv_maps array with one slot for every leaf partition. Maybe, there could be following fields in ModifyTableState (and similarly in CopyState): int mt_num_parted /* this one exists today */ struct PartitionDispatchData **mt_partition_dispatch_info; /* and this */ List **mt_parted_keystate; TupleConversionMap **mt_parted_tupconv_maps; TupleTableSlot **mt_parted_tupslots; Each of those arrays will have mt_num_parted slots. Thanks, Amit -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers