Re: Run-time pruning for ModifyTable

Thanks for having a look at this, Amit.

On Fri, 24 Jan 2020 at 21:57, Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>
> On Thu, Jan 23, 2020 at 4:31 PM Amit Langote <amitlangote09(at)gmail(dot)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