Re: Rework manipulation and structure of attribute mappings

Hi Michael,

Thanks for working on this. I guess this is a follow up to:
https://www.postgresql.org/message-id/20191102052001.GB1614%40paquier.xyz

On Thu, Nov 21, 2019 at 1:26 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> Hi all,
>
> After working on dc816e58, I have noticed that what we are doing with
> attribute mappings is not that good. In a couple of code paths of the
> rewriter, the executor, and more particularly ALTER TABLE, when
> working on the creation of inherited relations or partitions an
> attribute mapping gets used to make sure that the new cloned elements
> (indexes, fk, etc.) have correct definitions linked correctly from the
> parent to the child's attributes.
>
> Sometimes things can go wrong, because the attribute array is just an
> AttrNumber pointer and it is up to the caller building the map to
> guess which length it has. Existing callers do that fine, but this
> can lead to errors as recent history has proved.
>
> Attached is a patch to refactor all that which simply adds the
> attribute mapping length directly with the attribute list. The nice
> effect of the refactoring is that now callers willing to get attribute
> maps don't need to think about which length it should have, and this
> allows to perform checks on the expected number of attributes in the
> map particularly in the executor part. A couple of structures also
> have their logic simplified.

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. 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 attrmap.c.

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.

Thoughts?

> On top of that, I have spotted two fishy attribute mapping calculations
> in addFkRecurseReferencing() when adding a foreign key for partitions
> when there are dropped columns and in CloneFkReferencing(). The
> mapping was using the number of attributes from the foreign key, which
> can be lower than the mapping of the parent if there are dropped
> columns in-between. I am pretty sure that if some attributes of the
> parent are dropped (aka mapping set to 0 in the middle of its array
> then we could finish with an incorrect attribute mapping, and I
> suspect that this could lead to failures similar to what was fixed in
> dc816e58, but I have not spent much time yet into that part.

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.

Thanks,
Amit