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

Hello,

At Fri, 10 Nov 2017 09:34:57 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in
<5fcb1a9f-b4ad-119d-14c7-282c30c7f8d1@lab.ntt.co.jp>
> Hi Amul.
> 
> On 2017/11/09 20:05, amul sul wrote:
> > On Mon, Nov 6, 2017 at 3:31 PM, Amit Langote
> > <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> >> On 2017/11/06 14:32, David Rowley wrote:
> >>> On 6 November 2017 at 17:30, Amit Langote wrote:
> >>>> On 2017/11/03 13:32, David Rowley wrote:
> >>>>> On 31 October 2017 at 21:43, Amit Langote wrote:
> > [....]
> >>
> >> Attached updated set of patches, including the fix to make the new pruning
> >> code handle Boolean partitioning.
> >>
> > 
> > I am getting following warning on mac os x:
> 
> Thanks for the review.
> 
> > partition.c:1800:24: warning: comparison of constant -1 with
> > expression of type 'NullTestType'
> >       (aka 'enum NullTestType') is always false
> > [-Wtautological-constant-out-of-range-compare]
> >                                 if (keynullness[i] == -1)
> >                                     ~~~~~~~~~~~~~~ ^  ~~
> > partition.c:1932:25: warning: comparison of constant -1 with
> > expression of type 'NullTestType'
> >       (aka 'enum NullTestType') is always false
> > [-Wtautological-constant-out-of-range-compare]
> >                                         if (keynullness[i] == -1)
> >                                             ~~~~~~~~~~~~~~ ^  ~~
> > 2 warnings generated.
> > 
> > 
> > Comment for 0004 patch:
> > 270 +   /* -1 represents an invalid value of NullTestType. */
> >  271 +   memset(keynullness, -1, PARTITION_MAX_KEYS * sizeof(NullTestType));
> > 
> > I think we should not use memset to set a value other than 0 or true/false.
> > This will work for -1 on the system where values are stored in the 2's
> > complement but I am afraid of other architecture.
> 
> OK, I will remove all instances of comparing and setting variables of type
> NullTestType to a value of -1.

In 0002, bms_add_range has a bit naive-looking loop

+    while (wordnum <= uwordnum)
+    {
+        bitmapword mask = (bitmapword) ~0;
+
+        /* If working on the lower word, zero out bits below 'lower'. */
+        if (wordnum == lwordnum)
+        {
+            int lbitnum = BITNUM(lower);
+            mask >>= lbitnum;
+            mask <<= lbitnum;
+        }
+
+        /* Likewise, if working on the upper word, zero bits above 'upper' */
+        if (wordnum == uwordnum)
+        {
+            int ushiftbits = BITS_PER_BITMAPWORD - (BITNUM(upper) + 1);
+            mask <<= ushiftbits;
+            mask >>= ushiftbits;
+        }
+
+        a->words[wordnum++] |= mask;
+    }

Without some aggressive optimization, the loop takes most of the
time to check-and-jump for nothing especially with many
partitions and somewhat unintuitive.

The following uses a bit tricky bitmap operation but
is straightforward as a whole.

=====
/* fill the bits upper from BITNUM(lower) (0-based) of the first word */
a->workds[wordnum++] += ~(bitmapword)((1 << BITNUM(lower)) - 1);

/* fill up intermediate words */
while (wordnum < uwordnum)  a->words[wordnum++] = ~(bitmapword) 0;

/* fill up to BITNUM(upper) bit (0-based) of the last word */
a->workds[wordnum++] |=    (~(bitmapword) 0) >> (BITS_PER_BITMAPWORD - (BITNUM(upper) - 1));
=====


In 0003, 

+match_clauses_to_partkey(RelOptInfo *rel,
...
+      if (rinfo->pseudoconstant &&
+        (IsA(clause, Const) &&
+         ((((Const *) clause)->constisnull) ||
+          !DatumGetBool(((Const *) clause)->constvalue))))
+      {
+        *constfalse = true;
+        continue;

If we call this function in both conjunction and disjunction
context (the latter is only in recursive case). constfalse ==
true means no need of any clauses for the former case.

Since (I think) just a list of RestrictInfo is expected to be
treated as a conjunction (it's quite doubious, though..), we
might be better call this for each subnodes of a disjunction. Or,
like match_clauses_to_index, we might be better provide
match_clause_to_partkey(rel, rinfo, contains_const), which
returns NULL if constfalse. (I'm not self-confident on this..)


+          /*
+           * If no commutator exists, cannot flip the qual's args,
+           * so give up.
+           */
+          if (!OidIsValid(expr_op))
+            continue;

I suppose it's better to leftop and rightop as is rather than
flipping over so that var is placed left-side. Does that make
things so complex?

+     * It the operator happens to be '<>', which is never listed
If?


+        if (!op_in_opfamily(expr_op, partopfamily))
+        {
+          Oid    negator = get_negator(expr_op);
+
+          if (!OidIsValid(negator) ||
+            !op_in_opfamily(negator, partopfamily))
+            continue;

classify_partition_bounding_keys() checks the same thing by
checking whether the negator's strategy is
BTEquealStrategyNumber. (I'm not sure the operator is guaranteed
to be of btreee, though..) Aren't they needed to be in similar
way?

# In the function, "partkey strategy" and "operator strategy" are
# confusing..

+  AttrNumber  attno;

This declaration might be better in a narrower scope.


-- 
Kyotaro Horiguchi
NTT Open Source Software Center




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers