From: | Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | Andres Freund <andres(at)anarazel(dot)de>, pgsql-hackers(at)postgresql(dot)org |
Cc: | amitdkhan(dot)pg(at)gmail(dot)com |
Subject: | Re: Tuple conversion naming |
Date: | 2018-10-02 07:18:19 |
Message-ID: | cff8ef40-fe21-81e2-d9a0-607e9491bd48@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | 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:
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
From | Date | Subject | |
---|---|---|---|
Next Message | Andres Freund | 2018-10-02 07:35:55 | Re: Slotification of partition tuple conversion |
Previous Message | Amit Langote | 2018-10-02 07:11:54 | Re: Expression errors with "FOR UPDATE" and postgres_fdw with partition wise join enabled. |