On Thu, 4 Mar 2021 at 19:03, Amit Langote <amitlangot...@gmail.com> wrote: > > On Tue, Oct 20, 2020 at 9:46 PM Amit Langote <amitlangot...@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
