Re: [HACKERS] path toward faster partition pruning

Hello,

At Wed, 22 Nov 2017 17:59:48 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in <df609168-b7fd-4c0b-e9b2-6e398d411e27@lab.ntt.co.jp>
> Thanks Rajkumar for the test.
> 
> On 2017/11/21 19:06, Rajkumar Raghuwanshi wrote:
> > explain select * from hp_tbl where a = 2;
> > server closed the connection unexpectedly
> >     This probably means the server terminated abnormally
> >     before or while processing the request.
> > The connection to the server was lost. Attempting reset: Failed.
> > !>
> 
> It seems I wrote an Assert in the code to support hash partitioning that
> wasn't based on a valid assumption.  I was wrongly assuming that all hash
> partitions for a given modulus (largest modulus) must exist at any given
> time, but that isn't the case.
> 
> Fixed in the attached.  No other changes beside that.

0001 and 0002 are under discussion with Robert in another thread.

I don't have a comment on 0003, 0004.

0005:

 get_partitions_from_clauses is written as _using_ in it's
 comment. (also get_partitions_from_clauses_recurse is _guts in
 its comment.)

 get_append_rel_partitions just returns NIL if constfalse. I
 suppose we'd better reducing indentation level
 here. get_partitions_from_clauses_recurse in 0006 does the same
 thing.

 In the same function, there's a else clause separated from then
 clause by a multiline comment. It seems better that the else
 clause has braces and the comment is in the braces like the
 following.

 >  else
 >  {
 >    /*
 >     * Else there are no clauses....
 >     */
 >    partindexes = bms_add_range(NULL, 0, partdesc->nparts - 1);
 >  }



0006:

 In get_partitions_from_clauses_recurse, the following comment
 seems confusing.

+	/*
+	 * The analysis of the matched clauses done by
+	 * classify_partition_bounding_keys may have found mutually contradictory
+	 * clauses.
+	 */

 constfalse = true also when the cluase was just one false pseudo
 constant restrictinfo.

+	if (!constfalse)
+	{
+		/*
+		 * If all clauses in the list were OR clauses,
+		 * classify_partition_bounding_keys() wouldn't have formed keys
+		 * yet.  They will be handled below by recursively calling this
+		 * function for each of OR clauses' arguments and combining the
+		 * resulting partition sets appropriately.
+		 */
+		if (nkeys > 0)

 classify_p_b_keys() to return zero also when is not only all-OR
 clauses(all AND clauses with volatile function also returns
 zero).

+			/* Set partexpr if needed. */
+			if (partattno == 0)

 Could you add a description about the meaning of 0 to the
 comment of PartitionKeyData something like this?

  | AttrNumber *partattrs;    /* attribute numbers of columns in the
  |                            * partition key. 0 means partexpression */

+ #define EXPR_MATCHES_PARTKEY(expr, partattno, partexpr) \
+ 		((IsA((expr), Var) &&\
+ 		 ((Var *) (expr))->varattno == (partattno)) ||\
+ 		 equal((expr), (partexpr)))
...
+        if (EXPR_MATCHES_PARTKEY(leftop, partattno, partexpr))

 partattno = 0 has a different meaning than ordinary attnos.	 
 I belive that the leftop cannot be a whole row var, but I
 suppose we should make it clear. Likewise, even it doesn't
 actually happen but it formally has a chance to make a false
 match since partexpr is not cleared when partattno > 0.
 EXPR_MATCHES_PARTKEY might be better be something like follows.

| #define EXPR_MATCHES_PARTKEY(expr, partattno, partexpr) \
| 	((partattno) != 0 ? \
| 	 (IsA((expr), Var) && ((Var *) (expr))->varattno == (partattno)) :\
| 	 equal((expr), (partexpr)))


+        if (!op_in_opfamily(opclause->opno, partopfamily))
+        {
...
+          negator = get_negator(opclause->opno);
+          if (OidIsValid(negator) &&
+            op_in_opfamily(negator, partopfamily))
+          {
+            get_op_opfamily_properties(negator, partopfamily,
+                           false,
+                           &strategy,
+                           &lefttype, &righttype);
+            if (strategy == BTEqualStrategyNumber)

 This seems to me to be a bit too much relying on the specific
 relationship of the access methods' property. Isn't it
 reasonable that add checking that partkey->strategy != 'h'
 before getting negator?

+          commuted->opno = get_commutator(opclause->opno);

 Im afraid that get_commutator can return InvalidOid for
 user-defined types or by user-defined operator class or perhaps
 other reasons uncertain to me. match_clauses_to_partkey is
 checking that.

+      else if (IsA(clause, ScalarArrayOpExpr))

 I'm not sure what to do with a multidimentional ArrayExpr but
 ->multidims is checked some places.

+		ParseState *pstate = make_parsestate(NULL);

 make_parsestate mandates for the caller to free it by
 free_parsestate(). It doesn't seem to leak anything in the
 context and I saw the same thing at other places but it would be
 better to follow it if possible, or add some apology as a
 comment.. (or update the comment of make_parsestate?)

+  * If the leftarg_const and rightarg_consr are both of the type expected

 rightarg_consr -> const



+		if (partition_cmp_args(partkey, partattoff,
+							   le, lt, le,
+							   &test_result))

+		if (partition_cmp_args(partkey, partattoff, ge, gt, ge,
+							   &test_result))

 Please unify the style.


+       * Boolean conditions have a special shape, which would've been
+       * accepted if the partitioning opfamily accepts Boolean
+       * conditions.

 I noticed that bare true and false are not accepted by the
 values list of create table syntax. This is not a comment on
 this patch but is that intentional?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center