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 | Resend email |
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 |
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 |