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:
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.
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.
David
