On 5 November 2018 at 20:17, Amit Langote <langote_amit...@lab.ntt.co.jp> wrote: > On 2018/11/04 19:07, David Rowley wrote: >> Perhaps a better design would be to instead of having random special >> partitioned-table-only fields in ResultRelInfo, just have an extra >> struct there that contains the extra partition information which would >> include the translation maps and then just return the ResultRelInfo >> and allow callers to lookup any extra details they require. > > IIUC, you're saying that we could introduce a new struct that contains > auxiliary information needed by partition pruning (maps, slot, etc. for > tuple conversion) and add a new ResultRelInfo member of that struct type. > That way, there is no need to return them separately or return an index to > access them from their arrays. I guess we won't even need the arrays we > have now. I think that might be a good idea and simplifies things > significantly.
I've attached a patch which does this. It adds a new struct named PartitionRoutingInfo into ResultRelInfo and pulls 3 of the 4 arrays out of PartitionTupleRouting. There are fields for each of what these arrays used to store inside the PartitionRoutingInfo struct. While doing this I kept glancing back over at ModifyTableState and at the mt_per_subplan_tupconv_maps array. I wondered if it would be better to make the PartitionRoutingInfo a bit more generic, perhaps call it TupleConversionInfo and have fields like ti_ToGeneric and ti_FromGeneric, with the idea that "generic" would be the root partition or the first subplan for inheritance updates. This would allow us to get rid of a good chunk of code inside nodeModifyTable.c. However, I ended up not doing this and left PartitionRoutingInfo to be specifically for Root to Partition conversion. Also, on the topic about what to name the conversion maps from a few days ago; After looking at this a bit more I decided that having them named ParentToChild and ChildToParent is misleading. If the child is the child of some sub-partitioned table then the parent that the map is talking about is not the partition's parent, but the root partitioned table. So really RootToPartition and PartitionToRoot seem like much more accurate names for the maps. I made a few other changes along the way; I thought that ExecFindPartition() would be a good candidate to take on the responsibility of validating the partition is valid for INSERTs when it uses a partition out of the subplan_resultrel_hash. I thought it was better to check this once when we're in the code path of grabbing the ResultRelInfo out that hash table rather than in a code path that must check if it's been done already each time we route a tuple into a partition. It also allowed me to get rid of ri_PartitionReadyForRouting. I also moved the call to ExecInitRoutingInfo() into ExecFindPartition() which allowed me to make that function static, which could result in the generation of slightly more optimal compiled code. Please find attached the v14 patch. Rather nicely git --stat reports a net negative additional code (with the 48 lines of new tests included) 11 files changed, 632 insertions(+), 660 deletions(-) -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
v14-0001-Speed-up-INSERT-and-UPDATE-on-partitioned-tables.patch
Description: Binary data
