Re: Tuple conversion naming - Mailing list pgsql-hackers

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.

> For building tuple conversion maps we have:
> - convert_tuples_by_name
> - convert_tuples_by_name_map
> - convert_tuples_by_position
> - ExecSetupChildParentMapForLeaf
> - TupConvMapForLeaf
> - free_conversion_map

Of these, ExecSetupChildParentMapForLeaf and TupConvMapForLeaf go away
with the patch posted in the "Speeding up INSERTs and UPDATEs to
partitioned tables" thread:

https://www.postgresql.org/message-id/CAKJS1f-SnNXUS0_2wOn-7ZsuvrmQd_6_t-uG8pyUKfdnE9_y-A%40mail.gmail.com

To summarize of what the above patch does and why it removed those
functions:  those functions allocate the memory needed to hold
TupleConversionMap pointers for *all* partitions in a given partition
tree, but the patch refactored the tuple routing initialization code such
that such allocations (the aforementioned and quite a few others) and the
functions are redundant.

> I've a bunch of problems with this:
> 1) convert_tuples_by_name etc sound like they actually perform tuple
>    conversion, rather than just prepare for it
> 2) convert_tuples_by_name_map returns not TupleConversionMap, but an
>    array. But convert_tuples_by_name does return TupleConversionMap.
> 3) The naming is pretty inconsistent. free_conversion_map
>    vs. convert_tuples_by_name, but both deal with TupleConversionMap?

Yeah, I had similar thoughts in past.

> 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.

> 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.

Thanks,
Amit