Re: [HACKERS] path toward faster partition pruning

Thanks Tomas for the review.

On 2018/03/30 1:55, Tomas Vondra wrote:
> Hi,
> 
> I think there's a bug in generate_pruning_steps_from_opexprs, which does
> this for PARTITION_STRATEGY_HASH:
> 
> 
>     for_each_cell(lc1, lc)
>     {
>         pc = lfirst(lc1);
> 
>         /*
>          * Note that we pass nullkeys for step_nullkeys,
>          * because we need to tell hash partition bound search
>          * function which of the keys are NULL.
>          */
>         Assert(pc->op_strategy == HTEqualStrategyNumber);
>         pc_steps =
>                 get_steps_using_prefix(context,
>                                        HTEqualStrategyNumber,
>                                        pc->expr,
>                                        pc->cmpfn,
>                                        pc->keyno,
>                                        nullkeys,
>                                        prefix);
>     }
> 
>     opsteps = list_concat(opsteps, list_copy(pc_steps));
> 
> 
> Notice that the list_concat() is outside the for_each_cell loop. Doesn't
> that mean we fail to consider some of the clauses (all except the very
> last clause) for pruning? I haven't managed to come up with an example,
> but I haven't tried very hard.
> 
> FWIW I've noticed this because gcc complains that pg_steps might be used
> uninitialized:
> 
> partprune.c: In function ‘generate_partition_pruning_steps_internal’:
> partprune.c:992:16: warning: ‘pc_steps’ may be used uninitialized in
> this function [-Wmaybe-uninitialized]
>       opsteps = list_concat(opsteps, list_copy(pc_steps));
>                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> partprune.c:936:14: note: ‘pc_steps’ was declared here
>       List   *pc_steps;
>               ^~~~~~~~
> All of PostgreSQL successfully made. Ready to install.
> 
> 
> So even if it's not a bug, we probably need to fix the code somehow.

Yeah, the code needs to be fixed.  Although, it seems to me that in the
hash partitioning case, the loop would iterate at most once, at least if
the query didn't contain any Params.  That's because, at that point, there
cannot be multiple mutually AND'd equality clauses referring to the same
key.  For example, if there were in the original query and they contained
different values, we wouldn't get this far anyway as they would be reduced
to constant-false at an earlier planning stage.  If they all contained the
same value (e.g. key = 1 and key = 1::smallint and a = 1::int and a =
1::bigint), then only one of them will be left in rel->baserestrictinfo
anyway.  But we still need to have the loop because all of what I said
wouldn't happen if the clauses contained Params.  In that case, the result
would be determined at execution time.

I have fixed the code as you suggested and will post the fixed version
shortly after fixing the issues David reported.

Thanks,
Amit