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