Re: Rework manipulation and structure of attribute mappings

On Fri, Dec 06, 2019 at 06:03:12PM +0900, Amit Langote wrote:
> On Wed, Dec 4, 2019 at 5:03 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>> I see. That saved me some time, thanks. It is not really intuitive
>> to name routines about tuple conversion from tupconvert.c to
>> attrmap.c, so I'd think about renaming those routines as well, like
>> build_attrmap_by_name/position instead. That's more consistent with
>> the rest I added.
>
> Sorry I don't understand this. Do you mean we should rename the
> routines left behind in tupconvert.c? For example,
> convert_tuples_by_name() doesn't really "convert" tuples, only builds
> a map needed to do so. Maybe build_tuple_conversion_map_by_name()
> would be a more suitable name.

I had no plans to touch this area nor to rename this layer because
that was a bit out of the original scope of this patch which is to
remove the confusion and random bets with map lengths. I see your
point though and actually a name like what you are suggesting reflects
better what the routine does in reality. :p

> Maybe we don't need to repeat here what AttrMap is used for (it's
> already written in attmap.c), only write what it is and why it's
> needed in the first place. Maybe like this:
>
> /*
> * Attribute mapping structure
> *
> * This maps attribute numbers between a pair of relations, designated 'input'
> * and 'output' (most typically inheritance parent and child relations), whose
> * common columns have different attribute numbers. Such difference may arise
> * due to the columns being ordered differently in the two relations or the
> * two relations having dropped columns at different positions.
> *
> * 'maplen' is set to the number of attributes of the 'output' relation,
> * taking into account any of its dropped attributes, with the corresponding
> * elements of the 'attnums' array set to zero.
> */

That sounds better, thanks.

While on it, I have also spent some time checking after the FK-related
points that I suspected as fishy at the beginning of the thread but I
have not been able to break it. We also have coverage for problems
related to dropped columns in foreign_key.sql (grep for fdrop1), which
is more than enough. There was actually one extra issue in the patch
as of CloneFkReferencing() when filling in mapped_conkey based on the
number of keys in the constraint.

So, a couple of hours after looking at the code I am finishing with
the updated and indented version attached. What do you think?
--
Michael