Re: Rework manipulation and structure of attribute mappings

On Fri, Nov 22, 2019 at 02:21:41PM +0900, Amit Langote wrote:
> Thanks for working on this. I guess this is a follow up to:
> https://www.postgresql.org/message-id/20191102052001.GB1614%40paquier.xyz

Exactly. I got that in my mind for a couple of days, so I gave it a
shot and the result was kind of nice. And here we are.

> On Thu, Nov 21, 2019 at 1:26 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> The refactoring to use AttrMap looks good, though attmap.c as a
> separate module contains too little functionality (just palloc and
> pfree) to be a separate file, IMHO. If we are to build a separate
> module, why not move a bit more functionality into it from
> tupconvert.c. How about move all the code that actually creates the
> map to attmap.c? The entry points would be all the
> convert_tuples_by_name_map() and convert_tuples_by_name_map_if_req()
> functions that return AttrMap, rather than simply make_attrmap(int
> len) which can be a static routine.

Yeah, the current part is a little bit shy about that. Moving
convert_tuples_by_name_map() and the second one to attmap.c makes
sense.

> Actually, we should also refactor
> convert_tuples_by_position() to carve out the code that builds the
> AttrMap into a separate function and move it to attmap.c.

Not sure how to name that. One logic uses a match based on the
attribute name, and the other uses the type. So the one based on the
attribute name should be something like build_attrmap_by_name() and
the second attrmap_build_by_position()? We could use a better
convention like AttrMapBuildByPosition for example. Any suggestions
of names are welcome. Please note that I still have a commit fest to
run and finish, so I'll unlikely come back to that before December.
Let's continue with the tuning of the function names though.

> To be honest, "convert_tuples_" part in those names might start
> sounding a bit outdated in the future, so we should really consider
> inventing a new interface map_attributes(TupleDesc indesc, TupleDesc
> outdesc), because most call sites that fetch the AttrMap directly
> don't really use it for "converting tuples", but to convert
> expressions or to map key arrays.
>
> After all the movement, tupconvert.c will only retain the
> functionality to build a TupleConversionMap (wrapping the AttrMap) and
> to convert HeapTuples, that is, execute_attr_map_tuple() and
> execute_attr_map_slot(), which makes sense.

Agreed. Let's design that carefully.

> Actually, the patch can make addFkRecurseReferencing() crash, because
> the fkattnum[] array doesn't really contain attmap->maplen elements:
>
> - for (int j = 0; j < numfks; j++)
> - mapped_fkattnum[j] = attmap[fkattnum[j] - 1];
> + for (int j = 0; j < attmap->maplen; j++)
> + mapped_fkattnum[j] = attmap->attnums[fkattnum[j] - 1];
>
> You failed to notice that j is really used as index into fkattnum[],
> not the map array returned by convert_tuples_by_name(). So, I think
> the original coding is fine here.

Ouch, yes. The regression tests did not complain on this one. It
means that we could improve the coverage. The second, though... I
need to check it more closely.
--
Michael