Re: [HACKERS] path toward faster partition pruning

Hello, let me make some comments.

At Tue, 30 Jan 2018 09:57:44 +0900, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote in <4a7dda08-b883-6e5e-b0bf-f5ce95584e9e(at)lab(dot)ntt(dot)co(dot)jp>
> Hi Jesper.
>
> On 2018/01/29 22:13, Jesper Pedersen wrote:
> > Hi Amit,
> >
> > On 01/26/2018 04:17 AM, Amit Langote wrote:
> >> I made a few of those myself in the updated patches.
> >>
> >> Thanks a lot again for your work on this.
> >>
> >
> > This needs a rebase.
>
> AFAICS, v22 cleanly applies to HEAD (c12693d8f3 [1]), compiles, and make
> check passes.

Yes, it cleanly applies to HEAD and seems working.

0001 seems fine.

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.)

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)

-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);

-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?

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

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center