Re: Tuple conversion naming - Mailing list pgsql-hackers

Hi,

On 2018-10-02 16:18:19 +0900, Amit Langote wrote:
> Hi,
> 
> I agree that some clean up might be in order, but want to clarify a few
> points.
> 
> On 2018/10/02 15:11, Andres Freund wrote:
> > Hi,
> > 
> > The naming around partition related tuple conversions is imo worthy of
> > improvement.
> 
> Note that tuple conversion functionality in tupconvert.c has existed even
> before partitioning, although partitioning may have contributed to
> significant expansion of its usage.

Fair enough, doesn't really change my point tho :)


> > For executing them we have:
> > - do_convert_tuple
> > - ConvertPartitionTupleSlot
> >
> > which is two randomly differing spellings of related functionality,
> > without the name indicating that they, for reasons, don't both use
> > TupleConversionMap.
> 
> Actually, ConvertPartitionTupleSlot performs two tasks: 1. performs the
> conversion of the tuple in the input slot using do_convert_tuple, and 2.
> switches the TupleTableSlot to be used from this point on to a
> partition-specific one.

Yea, I had this mostly written with the view of the code after the
"slottification of partition tuple covnersion" thread.


> > I'm also not particularly happy about
> > "do_convert_tuple", which is a pretty generic name.
> > 
> > A lot of this probably made sense at some time, but we got to clean this
> > up. We'll grow more partitioning related code, and this IMO makes
> > reading the code harder - and given the change rate, that's something I
> > frequently have to do.
> 
> On the "Slotification of partition tuple conversion" thread, I suggested
> that we try to decouple partitioning-related tuple conversion from
> tupconvert.c, which may significantly improve things imho.  See the last
> paragraph of my message here:
> 
> https://www.postgresql.org/message-id/ed66156b-54fe-b5d2-10bf-3c73a8052e7e%40lab.ntt.co.jp
> 
> Basically, Amit Khandekar proposed that we add another function with the
> same functionality as do_convert_tuple to tupconvert.c that accepts and
> returns TupleTableSlot instead of HeapTuple.  Per discussion, it turned
> out that we won't need a full TupleConversionMap for partitioning-related
> conversions if we start using this new function, so we could land the new
> conversion function in execPartition.c instead of tupconvert.c.  Perhaps,
> we could just open-code do_convert_tuple()'s functionality in the existing
> ConvertPartitionTupleSlot().
> 
> Of course, that is not to say we shouldn't do anything about the possibly
> unhelpful names of the functions that tupconvert.c exports.

I'm not quite sure I see the point of a full decoupling of
partitioning-related tuple conversion from tupconvert.c. Yes, the
routines should be more clearly named, but why should the code be
separate?   I'm kinda wondering if we shouldn't have the tuple
conversion functions just use the slot based functionality in the back,
and just store those in the TupConversionMap.

Greetings,

Andres Freund