Re: [HACKERS] path toward faster partition pruning

From: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
To: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
Cc: jesper(dot)pedersen(at)redhat(dot)com, david(dot)rowley(at)2ndquadrant(dot)com, amitlangote09(at)gmail(dot)com, rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com, robertmhaas(at)gmail(dot)com, dilipbalaut(at)gmail(dot)com, memissemerson(at)gmail(dot)com, pgsql-hackers(at)postgresql(dot)org
Subject: Re: [HACKERS] path toward faster partition pruning
Date: 2018-01-31 08:03:06
Message-ID: 64241fee-09af-fe4b-a0ab-7cd739965041@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
v23-0001-Some-interface-changes-for-partition_bound_-cmp-.patch text/plain 11.7 KB
v23-0002-Introduce-a-get_partitions_from_clauses.patch text/plain 69.8 KB
v23-0003-Move-some-code-of-set_append_rel_size-to-separat.patch text/plain 8.6 KB
v23-0004-More-refactoring-around-partitioned-table-Append.patch text/plain 13.4 KB
v23-0005-Teach-planner-to-use-get_partitions_from_clauses.patch text/plain 34.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Konstantin Knizhnik 2018-01-31 08:03:44 Re: JIT compiling with LLVM v9.0
Previous Message Masahiko Sawada 2018-01-31 06:53:06 Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions