Re: [HACKERS] path toward faster partition pruning

Horiguchi-san,

Thanks for the review.

On 2018/01/30 20:43, Kyotaro HORIGUCHI wrote:
> At Tue, 30 Jan 2018 09:57:44 +0900, Amit Langote wrote:
>> AFAICS, v22 cleanly applies to HEAD (c12693d8f3 [1]), compiles, and make
> I have some random comments on 0002.
> 
> -extract_partition_key_clauses implicitly assumes that the
>  commutator of inequality operator is the same to the original
>  operator. (commutation for such operators is an identity
>  function.)

Yeah, it seems so.

>  I believe it is always true for a sane operator definition, but
>  perhaps wouldn't it be better using commutator instead of
>  opclause->opno as the source of negator if any? (Attached diff 1)

ISTM, the same thing happens with or without the patch.

- pc->opno = OidIsValid(commutator) ? commutator : opclause->opno;
+ pc->opno = comparator;

comparator as added by the patch is effectively equal to the RHS
expression in the deleted line.

> -get_partitions_from_or_clause_args abandons arg_partset with all
>  bit set when partition constraint doesn't refute whole the
>  partition. Finally get_partitions_from_clauses does the same
>  thing but it's waste of cycles and looks weird. It seems to have
>  intended to return immediately there.
> 
>   >       /* Couldn't eliminate any of the partitions. */
>   >       partdesc = RelationGetPartitionDesc(context->relation);
>   > -     arg_partset = bms_add_range(NULL, 0, partdesc->nparts - 1);
>   > +     return bms_add_range(NULL, 0, partdesc->nparts - 1);
>   >     }
>   >  
>   >     subcontext.rt_index = context->rt_index;
>   >     subcontext.relation = context->relation;
>   >     subcontext.clauseinfo = &partclauseinfo;
>  !>     arg_partset = get_partitions_from_clauses(&subcontext);

You're right, fixed.

> -get_partitions_from_or_clause_args converts IN (null) into
>  nulltest and the nulltest doesn't exclude a child that the
>  partition key column can be null.
> 
>  drop table if exists p;
>  create table p (a int, b int) partition by list (a);
>  create table c1 partition of p for values in (1, 5, 7);
>  create table c2 partition of p for values in (4, 6, null);
>  insert into p values (1, 0), (null, 0);
>  
>  explain select tableoid::regclass, * from p where a in (1, null);
>  >                            QUERY PLAN                            
>  > -----------------------------------------------------------------
>  >  Result  (cost=0.00..76.72 rows=22 width=12)
>  >    ->  Append  (cost=0.00..76.50 rows=22 width=12)
>  >          ->  Seq Scan on c1  (cost=0.00..38.25 rows=11 width=12)
>  >                Filter: (a = ANY ('{1,NULL}'::integer[]))
>  >          ->  Seq Scan on c2  (cost=0.00..38.25 rows=11 width=12)
>  >                Filter: (a = ANY ('{1,NULL}'::integer[]))
> 
>  Although the clause "a in (null)" doesn't match the (null, 0)
>  row so it donesn't harm finally, I don't think this is a right
>  behavior. null in an SAOP rightop should be just ignored on
>  partition pruning. Or is there any purpose of this behavior?

Yeah, it seems that we're better off ignoring null values appearing the
IN-list.  Framing a IS NULL or IS NOT NULL expression corresponding to a
null value in the SAOP rightop array doesn't seem to be semantically
correct, as you also pointed out.  In ExecEvalScalarArrayOpExpr(), I see
that a null in the rightop array (IN-list) doesn't lead to selecting rows
containing null in the corresponding column.

> - In extract_bounding_datums, clauseinfo is set twice to the same
>   value.

Oops, my bad when merging in David's patch.

Update patch set attached.  Thanks again.

Regards,
Amit