Re: Slotification of partition tuple conversion - Mailing list pgsql-hackers
From | Amit Khandekar |
---|---|
Subject | Re: Slotification of partition tuple conversion |
Date | |
Msg-id | CAJ3gD9cs+ogjCG=mOkq2agmJ0O_t+TMR-3oZ2uP+1JRzbjUZyw@mail.gmail.com Whole thread Raw |
In response to | Slotification of partition tuple conversion (Amit Khandekar <amitdkhan.pg@gmail.com>) |
Responses |
Re: Slotification of partition tuple conversion
|
List | pgsql-hackers |
On 3 September 2018 at 12:14, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > Hi Amit, > > Thanks for the updated patch and sorry I couldn't reply sooner. > > On 2018/08/21 16:18, Amit Khandekar wrote: >> On 21 August 2018 at 08:12, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: >>> Here are some comments on the patch: >> >> Thanks for the review. >> >>> >>> +ConvertTupleSlot >>> >>> Might be a good idea to call this ConvertSlotTuple? >> >> I thought the name 'ConvertTupleSlot' emphasizes the fact that we are >> operating rather on a slot without having to touch the tuple wherever >> possible. Hence I chose the name. But I am open to suggestions if >> there are more votes against this. > > Hmm, okay. > > The verb here is "to convert", which still applies to a tuple, the only > difference is that the new interface accepts and returns TupleTableSlot, > instead of HeapTuple. Yeah, in that sense you are right. Let's see others' suggestions. For now I haven't changed it. > @@ -2691,12 +2861,14 @@ CopyFrom(CopyState cstate) > > /* > * We might need to convert from the parent rowtype to the > - * partition rowtype. > + * partition rowtype. Don't free the already stored tuple as it > + * may still be required for a multi-insert batch. > */ > tuple = > ConvertPartitionTupleSlot(proute->parent_child_tupconv_maps[leaf_part_index], > tuple, > proute->partition_tuple_slot, > - &slot); > + &slot, > + false); > > So the "already stored tuple" means a previous tuple that may have been > stored in 'proute->partition_tuple_slot', which shouldn't be freed in > ConvertPartitionTupleSlot (via ExecStoreTuple), because it might also have > been stored in the batch insert tuple array. > > Now, because with ConvertTupleSlot, there is no worry about freeing an > existing tuple, because we never perform ExecStoreTuple on > proute->partition_tuple_slot (or one of the slots in it if we consider my > patch at [1] which converts it to an array). All I see applied to those > slots is ExecStoreVirtualTuple() in ConvertTupleSlot(), and in some cases, > ExecCopySlotTuple() in the caller of ConvertTupleSlot(). > > So, I think that that sentence is obsolete with your patch. Right. Removed the "Don't free the already stored tuple" part. > I was saying, because we no longer use do_convert_tuple for > "partitioning-related" tuple conversions, we could get rid of > TupleConversionMaps in the partitioning-specific PartitionTupleRouting > structure. We use child_parent_tupconv_maps in AfterTriggerSaveEvent() where we call do_convert_tuple() with the map. We pass parent_child_tupconv_maps to adjust_partition_tlist(), which requires the map->outdesc. So there are these places where still we can't get rid of TupleConversionMaps even for partition-related tuples. Regarding adjust_partition_tlist(), we can perhaps pass the outdesc from somewhere else rather than map->outdesc, making it possible to use AttrNumber map rather than TupleConversionMap. But it needs some investigation. Overall, I think both the above maps should be worked on as a separate item, and not put in this patch that focusses on ConvertTupleSlot(). I have changed the PartitionDispatch->tupmap type to AttrNumber[], though. This was possible because we are using the tupmap->attrMap and no other fields. > > Also, since ConvertTupleSlot itself just uses the attrMap field of > TupleConversionMap, I was wondering if its signature should contain > AttrNumber * instead of TupleConversionMap *? Done. > > Maybe if we get around to getting rid of do_convert_tuple altogether, that > would also mean getting rid of the TupleConversionMap struct, because its > various tuple data related arrays would be redundant, because > ConvertTupleSlot has input and output TupleTableSlots, which have space > for that information. Yeah agree. We will be working towards that. -- Thanks, -Amit Khandekar EnterpriseDB Corporation The Postgres Database Company
Attachment
pgsql-hackers by date: