Re: Wired if-statement in gen_partprune_steps_internal

Hi,

On Thu, Oct 8, 2020 at 6:56 PM Andy Fan <zhihui(dot)fan1213(at)gmail(dot)com> wrote:
>
> Hi:
>
> I found the following code in gen_partprune_steps_internal, which
> looks the if-statement to be always true since list_length(results) > 1;
> I added an Assert(step_ids != NIL) and all the test cases passed.
> if the if-statement is always true, shall we remove it to avoid confusion?
>
>
> gen_partprune_steps_internal(GeneratePruningStepsContext *context,
>
>
> if (list_length(result) > 1)
> {
> List *step_ids = NIL;
>
> foreach(lc, result)
> {
> PartitionPruneStep *step = lfirst(lc);
>
> step_ids = lappend_int(step_ids, step->step_id);
> }
> Assert(step_ids != NIL);
> if (step_ids != NIL) // This should always be true.
> {
> PartitionPruneStep *step;
>
> step = gen_prune_step_combine(context, step_ids,
> PARTPRUNE_COMBINE_INTERSECT);
> result = lappend(result, step);
> }
> }

That seems fine to me.

Looking at this piece of code, I remembered that exactly the same
piece of logic is also present in gen_prune_steps_from_opexps(), which
looks like this:

/* Lastly, add a combine step to mutually AND these op steps, if needed */
if (list_length(opsteps) > 1)
{
List *opstep_ids = NIL;

foreach(lc, opsteps)
{
PartitionPruneStep *step = lfirst(lc);

opstep_ids = lappend_int(opstep_ids, step->step_id);
}

if (opstep_ids != NIL)
return gen_prune_step_combine(context, opstep_ids,
PARTPRUNE_COMBINE_INTERSECT);
return NULL;
}
else if (opsteps != NIL)
return linitial(opsteps);

I think we should remove this duplicative logic and return the
generated steps in a list from this function, which the code in
gen_partprune_steps_internal() then "combines" using an INTERSECT
step. See attached a patch to show what I mean.

--
Amit Langote
EDB: http://www.enterprisedb.com