On 14 January 2018 at 17:27, Amit Khandekar <amitdkhan...@gmail.com> wrote: > On 13 January 2018 at 02:56, Robert Haas <robertmh...@gmail.com> wrote: > > I guess I'm inclined to keep mt_per_sub_plan_maps for the case where > > there are no partitions, but not use it when partitions are present. > > What do you think about that? > > Even where partitions are present, in the usual case where there are no > transition tables we won't require per-leaf map at all [1]. So I think we > should keep mt_per_sub_plan_maps only for the case where per-leaf map is not > allocated. And we will not allocate mt_per_sub_plan_maps when > mt_per_leaf_maps is needed. In other words, exactly one of the two maps will > be allocated. > > This is turning out to be close to what's already there in the last patch > versions: use a single map array, and an offsets array. The difference is : > in the patch I am using the *same* variable for the two maps. Where as, now > we are talking about two different array variables for maps, but only > allocating one of them. > > Are you ok with this ? I think the thing you were against was to have a > common *variable* for two purposes. But above, I am saying we have two > variables but assign a map array to only *one* of them and leave the other > unused. > > --------- > > Regarding the on-demand map allocation .... > Where mt_per_sub_plan_maps is allocated, we won't have the on-demand > allocation: all the maps will be allocated initially. The reason is becaues > the map_is_required array is only per-leaf. Or else, again, we need to keep > another map_is_required array for per-subplan. May be we can support the > on-demand stuff for subplan maps also, but only as a separate change after we > are done with update-partition-key. > > > --------- > > Regarding mt_per_leaf_tupconv_required, I am thinking we can make it a bool, > and name it : mt_per_leaf_map_not_required. When it is true for a given > index, it means, we have already called convert_tuples_by_name() and it > returned NULL; i.e. it means we are sure that map is not required. A false > value means we need to call convert_tuples_by_name() if it is NULL, and then > set mt_per_leaf_map_not_required to (map == NULL). > > Instead of a bool array, , we can instead make it a Bitmapset. But I think > access would become slower as compared to array, particularly because it is > going to be a heavily used function.
I went ahead and did the above changes. I haven't yet merged these changes in the main patch. Instead, I have attached it as an incremental patch to be applied on the main v36 patch. The incremental patch is not yet quite polished, and quite a bit of cosmetic changes might be required, plus testing. But am posting it in case I have some early feedback. Details : The per-subplan map array variable is kept in ModifyTableState : - TupleConversionMap **mt_childparent_tupconv_maps; - /* Per plan/partition map for tuple conversion from child to root */ - bool mt_is_tupconv_perpart; /* Is the above map per-partition ? */ + TupleConversionMap **mt_per_subplan_tupconv_maps; + /* Per plan map for tuple conversion from child to root */ } ModifyTableState; The per-leaf array variable and the not_required array is kept in PartitionTupleRouting : - TupleConversionMap **partition_tupconv_maps; + TupleConversionMap **parent_child_tupconv_maps; + TupleConversionMap **child_parent_tupconv_maps; + bool *child_parent_tupconv_map_not_reqd; As you can see above, all the arrays are per-partition. So removed the per-leaf tag in these arrays. Instead, renamed the existing partition_tupconv_maps to parent_child_tupconv_maps, and the new per-leaf array to child_parent_tupconv_maps Have two separate functions ExecSetupChildParentMapForLeaf() and ExecSetupChildParentMapForSubplan() since most of their code is different. And now because of this, we can re-use ExecSetupChildParentMapForLeaf() in both copy.c and nodeModifyTable.c. Even inserts/copy will benefit from the on-demand map allocation. This is because now there is a function TupConvMapForLeaf() that is called in both copy.c and ExecInsert(). This is the function that does on-demand allocation. Attached the incremental patch conversion_map_changes.patch that has the above changes. It is to be applied over the latest main patch (update-partition-key_v36.patch).
conversion_map_changes.patch
Description: Binary data
