Hi,
I looked at the patch. Some comments.
On 2018/10/02 16:35, Andres Freund wrote:
> I wasn't quite happy yet with that patch.
>
> - ConvertTupleSlot seems like a too generic name, it's very unclear it's
> related to tuple mapping, rather than something internal to slots. I
> went for execute_attr_map_slot (and renamed do_convert_tuple to
> execute_attr_map_tuple, to match).
>
> I'd welcome a better name.
do_convert_tuple -> convert_tuple_using_map
ConvertTuplSlot -> convert_tuple_using_map_slot
?
> - I disliked inheritence_tupconv_map, it doesn't seem very clear why
> this is named inheritence_* (typo aside). I went for
> convert_tuples_by_name_map_if_req() - while I think this sounds
> too much like it converts tuples itself it should be renamed with the
> rest of the convert_tuples_by_* routines.
>
> I'd welcome a better name.
Agree about doing something about the convert_* names. A comment near the
beginning of tupconvert.c says all of these convert_* routines are meant
as conversion "setup" routines:
/*
* The conversion setup routines have the following common API:
So, maybe:
convert_tuples_by_position -> get_conversion_map_by_position
convert_tuples_by_name -> get_conversion_map_by_name
convert_tuples_by_name_map -> get_attr_map_for_conversion
convert_tuples_by_name_map_if_req -> get_attr_map_for_conversion_if_req
> - Combined the two patches, they seemed to largely affect related code
Hmm yeah, the code and data structures that my patch changed are only
related to the cases which involve tuple conversion.
I noticed the comment at the top of tupconvert.c requires updating:
* of dropped columns. There is some overlap of functionality with the
* executor's "junkfilter" routines, but these functions work on bare
* HeapTuples rather than TupleTableSlots.
Now we have functions that manipulate TupleTableSlots.
Thanks,
Amit
