> On Wed, Jul 15, 2020 at 04:35:52PM +0900, Etsuro Fujita wrote:
> On Sun, Jul 12, 2020 at 9:55 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> > Attached is an updated version of the patch.
>
> Here is a new version of the patch.
>
> Changes:
> * Fix a typo in a comment in get_steps_using_prefix_recurse()
> * Remove redundant regression test cases
> * Add a regression test comment
> * Add the commit message
>
> If there are no objections, I will commit this version.
Thanks for the patch! Sorry, forgot to follow up in time. To the best of
my knowledge the patch looks good, I have just a couple of questions:
+ List *step_exprs1,
+ *step_cmpfns1;
pc = lfirst(lc);
if (pc->keyno == cur_keyno)
{
- /* clean up before starting a new recursion cycle. */
- if (cur_keyno == 0)
- {
- list_free(step_exprs);
- list_free(step_cmpfns);
- step_exprs = list_make1(pc->expr);
- step_cmpfns = list_make1_oid(pc->cmpfn);
- }
- else
- {
- step_exprs = lappend(step_exprs, pc->expr);
- step_cmpfns = lappend_oid(step_cmpfns, pc->cmpfn);
- }
+ /* Leave the original step_exprs unmodified. */
+ step_exprs1 = list_copy(step_exprs);
+ step_exprs1 = lappend(step_exprs1, pc->expr);
+
+ /* Leave the original step_cmpfns unmodified. */
+ step_cmpfns1 = list_copy(step_cmpfns);
+ step_cmpfns1 = lappend_oid(step_cmpfns1, pc->cmpfn);
Does this also belong to some of mentioned fixes? I'm curious if there
could be memory concumption concerns, since it's done within a recursive
function.
- Assert(list_length(step_exprs) == cur_keyno);
+ Assert(list_length(step_exprs) == cur_keyno ||
+ (context->rel->part_scheme->strategy ==
+ PARTITION_STRATEGY_HASH &&
+ step_opstrategy == HTEqualStrategyNumber &&
+ !bms_is_empty(step_nullkeys) &&
+ bms_num_members(step_nullkeys) + list_length(step_exprs) + 2 ==
+ context->rel->part_scheme->partnatts));
I see the explanation in the commit message, but this assert condition
is quite complex, maybe worth couple of commentary lines right there?