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 <mich...@paquier.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
signature.asc
Description: PGP signature
