Re: [HACKERS] path toward faster partition pruning - Mailing list pgsql-hackers

Hello, let me make some comments.

At Tue, 30 Jan 2018 09:57:44 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in
<4a7dda08-b883-6e5e-b0bf-f5ce95584e9e@lab.ntt.co.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


diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index ab17524..a2488ab 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -2111,7 +2111,6 @@ extract_partition_key_clauses(PartitionKey partkey, List *clauses,
             PartClause *pc;
             Oid            partopfamily = partkey->partopfamily[i];
             Oid            partcoll = partkey->partcollation[i];
-            Oid            commutator = InvalidOid;
             AttrNumber    partattno = partkey->partattrs[i];
             Expr       *partexpr = NULL;
 
@@ -2144,6 +2143,7 @@ extract_partition_key_clauses(PartitionKey partkey, List *clauses,
             if (IsA(clause, OpExpr))
             {
                 OpExpr       *opclause = (OpExpr *) clause;
+                Oid            comparator = opclause->opno;
                 Expr       *leftop,
                            *rightop,
                            *valueexpr;
@@ -2161,13 +2161,14 @@ extract_partition_key_clauses(PartitionKey partkey, List *clauses,
                     valueexpr = rightop;
                 else if (EXPR_MATCHES_PARTKEY(rightop, partattno, partexpr))
                 {
-                    valueexpr = leftop;
-
-                    commutator = get_commutator(opclause->opno);
+                    Oid commutator = get_commutator(opclause->opno);
 
                     /* nothing we can do unless we can swap the operands */
                     if (!OidIsValid(commutator))
                         continue;
+
+                    valueexpr = leftop;
+                    comparator = commutator;
                 }
                 else
                     /* Clause does not match this partition key. */
@@ -2212,7 +2213,7 @@ extract_partition_key_clauses(PartitionKey partkey, List *clauses,
                      * equality operator *and* this is a list partitioned
                      * table, we can use it prune partitions.
                      */
-                    negator = get_negator(opclause->opno);
+                    negator = get_negator(comparator);
                     if (OidIsValid(negator) &&
                         op_in_opfamily(negator, partopfamily))
                     {
@@ -2236,7 +2237,7 @@ extract_partition_key_clauses(PartitionKey partkey, List *clauses,
                 }
 
                 pc = (PartClause *) palloc0(sizeof(PartClause));
-                pc->opno = OidIsValid(commutator) ? commutator : opclause->opno;
+                pc->opno = comparator;
                 pc->inputcollid = opclause->inputcollid;
                 pc->value = valueexpr;