Re: partitioning - changing a slot's descriptor is expensive

On 2018-06-27 14:46:26 +0900, Amit Langote wrote:
> Hi Andres,
> 
> On 2018/06/27 14:09, Andres Freund wrote:
> > Hi,
> > 
> > (sorry if I CCed the wrong folks, the code has changed a fair bit)
> > 
> > I noticed that several places in the partitioning code look like:
> > 
> >     /*
> >      * If the tuple has been routed, it's been converted to the
> >      * partition's rowtype, which might differ from the root
> >      * table's.  We must convert it back to the root table's
> >      * rowtype so that val_desc shown error message matches the
> >      * input tuple.
> >      */
> >     if (resultRelInfo->ri_PartitionRoot)
> >     {
> >         TableTuple tuple = ExecFetchSlotTuple(slot);
> >         TupleConversionMap *map;
> > 
> >         rel = resultRelInfo->ri_PartitionRoot;
> >         tupdesc = RelationGetDescr(rel);
> >         /* a reverse map */
> >         map = convert_tuples_by_name(orig_tupdesc, tupdesc,
> >                                      gettext_noop("could not convert row type"));
> >         if (map != NULL)
> >         {
> >             tuple = do_convert_tuple(tuple, map);
> >             ExecSetSlotDescriptor(slot, tupdesc);
> >             ExecStoreTuple(tuple, slot, InvalidBuffer, false);
> >         }
> >     }
> 
> This particular code block (and a couple of others like it) are executed
> only if a tuple fails partition constraint check, so maybe not a very
> frequently executed piece of code.
> 
> There is however similar code that runs in non-error paths, such as in
> ExecPrepareTupleRouting(), that is executed *if* the leaf partition and
> the root parent have differing attribute numbers.  So, one might say that
> that code's there to handle the special cases which may not arise much in
> practice, but it may still be a good idea to make improvements if we can
> do so.

Yea, I was referring to all do_convert_tuple() callers, and some of them
are more hot-path than the specific one above. E.g. the
ConvertPartitionTupleSlot() call in CopyFrom().


> > Unfortunately calling ExecSetSlotDescriptor() is far from cheap, it has
> > to reallocate the values/isnull arrays (and potentially do desc pinning
> > etc). Nor is it always cheap to call ExecFetchSlotTuple(slot) - it might
> > be a virtual slot. Calling heap_deform_tuple(), as done in
> > do_convert_tuple(), might be superfluous work, because the input tuple
> > might already be entirely deformed in the input slot.
> > 
> > I think it'd be good to rewrite the code so there's an input and an
> > output slot that each will keep their slot descriptors set.
> > 
> > Besides performance as the code stands, this'll also be important for
> > pluggable storage (as we'd otherwise get a *lot* of back/forth
> > conversions between tuple formats).
> > 
> > Anybody interesting in writing a patch that redoes this?
> 
> Thanks for the pointers above, I will see if I can produce a patch and
> will also be interested in hearing what others may have to say.

Cool.

Greetings,

Andres Freund