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
