Re: Rework manipulation and structure of attribute mappings - Mailing list pgsql-hackers

Thanks for the updated patch.

On Wed, Dec 4, 2019 at 5:03 PM Michael Paquier <michael@paquier.xyz> wrote:
> On Mon, Nov 25, 2019 at 05:55:50PM +0900, Amit Langote wrote:
> > Actually, I was just suggesting that we create a new function
> > convert_tuples_by_position_map() and put the logic that compares the
> > two TupleDescs to create the AttrMap in it, just like
> > convert_tuples_by_name_map().  Now you could say that there would be
> > no point in having such a function, because no caller currently wants
> > to use such a map (that is, without the accompanying
> > TupleConversionMap), but maybe there will be in the future.  Though
> > irrespective of that consideration, I guess you'd agree to group
> > similar code in a single source file.
>
> Hmm.  I would rather keep the attribute map generation and the tuple
> conversion part, the latter depending on the former, into two
> different files.  That's what I did in the v2 attached.

Check.

> > As it's mainly just moving around code, I gave it a shot; patch
> > attached (applies on top of yours).  I haven't invented any new names
> > yet, but let's keep discussing that as you say.
>
> 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.

> Another thing is that we have duplicated code at the end of
> build_attrmap_by_name_if_req and build_attrmap_by_position, which I
> think would be better refactored as a static function part of
> attmap.c.  This way the if_req() flavor gets much simpler.

Neat.

> I have also fixed the issue with the FK mapping in
> addFkRecurseReferencing() you reported previously.

Check.

> What do you think about that?  I would like to think that we are
> getting at something rather committable here, though I feel that I
> need to review more the comments around the new files and if we could
> document better AttrMap and its properties.

Regarding that, comment on a comment added by the patch:

+ * Attribute mapping structure
+ *
+ * An attribute mapping tracks the relationship of a child relation and
+ * its parent for inheritance and partitions.  This is used mainly for
+ * cloned object creations (indexes, foreign keys, etc.) when creating
+ * an inherited child relation, and for runtime-execution attribute
+ * mapping.
+ *
+ * Dropped attributes are marked with 0 and the length of the map is set
+ * to be the number of attributes of the parent, which takes into account
+ * its dropped attributes.

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.
 */

Thanks,
Amit