On 2017/04/07 8:36, Tom Lane wrote:
> I wrote:
>> I propose to deal with this by reverting 3838074f8 in toto, and then
>> trying to clarify that comment, and maybe adding a regression test case
>> based on the example I showed earlier so that it will be a little more
>> obvious if someone breaks this again.
>> However, I see that 3838074f8 touches some partitioning code, which
>> makes me wonder if there's anything in the partitioning logic that
>> really depends on this erroneous "optimization".
Definitely misread the comment there, but was mystified why the tests
didn't break. The partitioning tuple-routing code optionally avoids
converting tuples by using this optimization. Since TupleDesc.tdtypeid of
the parent and the partition to which a tuple is routed are never the
same, tuples would always have to be converted before 3838074f8. One of
the earlier versions of that patch introduced a consider_typeid parameter
for which only ExecEvalConvertRowtype() passed true.
> After further poking around, I've concluded that that approach is probably
> an overreaction. Of the dozen or so callers of convert_tuples_by_position
> and convert_tuples_by_name, it seems that ExecEvalConvertRowtype is the
> only one that really needs the correct composite-datum headers in the
> converted tuple; and even for it, forcing use of do_convert_tuple is
> a pretty expensive, brute-force way to get that result. Ashutosh's
> proposal to use heap_copy_tuple_as_datum when no column rearrangement
> is required should be substantially more efficient.
>
> So I now think it's okay to remove consideration of matching the target
> rowtype OID from the tupconvert.c functions, although we have to realize
> that that is effectively an API change for them, one which has a definite
> potential for biting third-party callers.
And I see that just in 3f902354b08 lets the partition tuple-routing code
keep utilizing that optimization.
Thanks,
Amit
