Re: Wired if-statement in gen_partprune_steps_internal

On Wed, Oct 14, 2020 at 11:26 AM Andy Fan <zhihui(dot)fan1213(at)gmail(dot)com> wrote:

>
>
> On Mon, Oct 12, 2020 at 4:37 PM Amit Langote <amitlangote09(at)gmail(dot)com>
> wrote:
>
>> 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.
>>
>>
> This changes LGTM, and "make check" PASSED, thanks for the patch!
>
>
I created https://commitfest.postgresql.org/30/2771/ so that this patch
will not
be lost. Thanks!

--
Best Regards
Andy Fan