Thanks for having a look at this, Amit. On Fri, 24 Jan 2020 at 21:57, Amit Langote <amitlangot...@gmail.com> wrote: > > On Thu, Jan 23, 2020 at 4:31 PM Amit Langote <amitlangot...@gmail.com> wrote: > Part of the optimizer patch that looks a bit complex is the changes to > inheritance_planner() which is to be expected, because that function > is a complex beast itself. I have suggestions to modify some comments > around the code added/modified by the patch for clarity; attaching a > delta patch for that.
I've made another pass over the patch and made various changes. The biggest of which was the required modifications to nodeModifyTable.c so that it can now prune all partitions. Append and MergeAppend were modified to allow this in 5935917ce59 (Thanks for pushing that Tom). I've also slightly simplified the code in ExecModifyTable() and added slightly more code to ExecInitModifyTable(). We now only set mt_whichplan to WHICHPLAN_CHOOSE_SUBPLAN when run-time pruning is enabled and do_exec_prune is true. I also made it so when all partitions are pruned that we set mt_whichplan to WHICHPLAN_CHOOSE_SUBPLAN as this saves an additional run-time check during execution. Over in inheritance_planner(), I noticed that the RT index of the SELECT query and the UPDATE/DELETE query can differ. There was some code that performed translations. I changed that code slightly so that it's a bit more optimal. It was building two lists, one for the old RT index and one for the new. It added elements to this list regardless of if the RT indexes were the same or not. I've now changed that to only add to the list if they differ, which I feel should never be slower and most likely always faster. I'm also now building a translation map between the old and new RT indexes, however, I only found one test in the regression tests which require any sort of translation of these RT indexes. This was with an inheritance table, so I need to do a bit more work to find a case where this happens with a partitioned table to ensure all this works. > The executor patch looks pretty benign too. Diffs that looked a bit > suspicious at first are due to replacing > ModifyTableState.resultRelInfo that is a pointer into > EState.es_result_relations array by an array of ResultRelInfo > pointers, but doing that seems to make the relevant code easier to > follow, especially if you consider the changes that the patch makes to > that code. Yeah, that's because the ModifyTableState's resultRelInfo field was just a pointer to the estate->es_result_relations array offset by the ModifyTable's resultRelIndex. This was fine previously because we always initialised the plans for each ResultRelInfo. However, now that we might be pruning some of those that array can't be used as it'll still contain ResultRelInfos for relations we're not going to touch. Changing this to an array of pointers allows us to point to the elements in estate->es_result_relations that we're going to use. I also renamed the field just to ensure nothing can compile (thinking of extensions here) that's not got updated code. Tom, I'm wondering if you wouldn't mind looking over my changes to inheritance_planner()? David
modifytable_runtime_prune_2020-03-10.patch
Description: Binary data