Re: Slotification of partition tuple conversion - Mailing list pgsql-hackers
From | Amit Khandekar |
---|---|
Subject | Re: Slotification of partition tuple conversion |
Date | |
Msg-id | CAJ3gD9cbVxq4c0MP+my+QfPaSqbGWySU6+P2q1jJODs-dH2MuA@mail.gmail.com Whole thread Raw |
In response to | Re: Slotification of partition tuple conversion (Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>) |
Responses |
Re: Slotification of partition tuple conversion
|
List | pgsql-hackers |
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. > > + /* > + * Get the tuple in the per-tuple context. Also, we > cannot call > + * ExecMaterializeSlot(), otherwise the tuple will get freed > + * while storing the next tuple. > + */ > + oldcontext = > MemoryContextSwitchTo(GetPerTupleMemoryContext(estate)); > + tuple = ExecCopySlotTuple(slot); > + MemoryContextSwitchTo(oldcontext); > > The comment about why we cannot call ExecMaterializeSlot is a bit unclear. > Maybe, the comment doesn't need to mention that? Instead, expand a bit > more on why the context switch here or how it interacts with recently > tuple buffering (0d5f05cde01)? Done : - * Get the tuple in the per-tuple context. Also, we cannot call - * ExecMaterializeSlot(), otherwise the tuple will get freed - * while storing the next tuple. + * Get the tuple in the per-tuple context, so that it will be + * freed after each batch insert. Specifically, we could not call ExecMaterializeSlot() because it sets tts_shouldFree to true which we don't want for batch inserts. > > Seeing that all the sites in the partitioning code that previously called > do_convert_tuple() now call ConvertTupleSlot, I wonder if we don't need a > full TupleConversionMap, just the attribute map in it. We don't need the > input/output Datum and bool arrays in it, because we'd be using the ones > from input and output slots of ConvertTupleSlot. So, can we replace all > instances of TupleConversionMap in the partitioning code and data > structures by AttributeNumber arrays. Yeah, I earlier thought I could get rid of do_convert_tuple() altogether. But there are places where we currently deal with tuples rather than slots. And here, we could not replace do_convert_tuple() unless we slotify the surrounding code similar to how you did in the Allocate-dedicated-slots patch. E.g. ExecEvalConvertRowtype() and AfterTriggerSaveEvent() deals with tuples so the do_convert_tuple() calls there couldn't be replaced. So I think we can work towards what you have in mind in form of incremental patches. > > Also, how about putting ConvertTupleSlot in execPartition.c and exporting > it in execPartition.h, instead of tupconvert.c and tupconvert.h, resp.? I think the goal of ConvertTupleSlot() is to eventually replace do_convert_tuple() as well, so it would look more of a general conversion rather than specific for partitions. Hence I think it's better to have it in tupconvert.c . -- Thanks, -Amit Khandekar EnterpriseDB Corporation The Postgres Database Company
Attachment
pgsql-hackers by date: