On 21 August 2018 at 08:12, Amit Langote <langote_amit...@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
tup_convert_v3.patch
Description: Binary data