On Wed, Apr 7, 2021 at 4:43 PM David Rowley <dgrowleyml@gmail.com> wrote:
> On Thu, 4 Mar 2021 at 19:03, Amit Langote <amitlangote09@gmail.com> wrote:
> > On Tue, Oct 20, 2020 at 9:46 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > > I had updated the patch last week to address Horiguchi-san's comments
> > > but didn't manage to post a polished-enough version. I will try again
> > > this week.
> >
> > Sorry, this seems to have totally slipped my mind.
> >
> > Attached is the patch I had promised.
>
> I've been looking at this patch today and spent quite a bit of time
> staring at the following fragment:
Thanks a lot for looking at this.
> case PARTCLAUSE_MATCH_STEPS:
> - Assert(clause_steps != NIL);
> - result = list_concat(result, clause_steps);
> + Assert(clause_step != NULL);
> + steps = lappend(steps, clause_step);
> break;
>
> So here, we used to use list_concat to add the steps that
> match_clause_to_partition_key() output, but now we lappend() the
> single step that match_clause_to_partition_key set in its output arg.
>
> This appears to be ok as we only return PARTCLAUSE_MATCH_STEPS from
> match_clause_to_partition_key() when we process a ScalarArrayOpExpr.
> There we just transform the IN(<list of consts>) into a Boolean OR
> clause with a set of OpExprs which are equivalent to the
> ScalarArrayOpExpr. e.g. "a IN (1,2)" becomes "a = 1 OR a = 2". The
> code path which processes the list of OR clauses in
> gen_partprune_steps_internal() will always just output a single
> PARTPRUNE_COMBINE_UNION combine step. So it does not appear that there
> are any behavioural changes there. The list_concat would always have
> been just adding a single item to the list before anyway.
Right, that was my observation as well.
> However, it does change the meaning of what PARTCLAUSE_MATCH_STEPS
> does. If we ever needed to expand what PARTCLAUSE_MATCH_STEPS does,
> then we'll have less flexibility with the newly updated code. For
> example if we needed to return multiple steps and only combine them at
> the top level then we now can't. I feel there's a good possibility
> that we'll never need to do that, but I'm not certain of that.
>
> I'm keen to hear your opinion on this.
That's a good point. So maybe gen_partprune_steps_internal() should
continue to return a list of steps, the last of which would be an
intersect step to combine the results of the earlier multiple steps.
We should still fix the originally reported issue that
gen_prune_steps_from_opexps() seems to needlessly add an intersect
step.
--
Amit Langote
EDB: http://www.enterprisedb.com
