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@paquier.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