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

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'scomment. (also get_partitions_from_clauses_recurse is _guts
initscomment.)
 
get_append_rel_partitions just returns NIL if constfalse. Isuppose we'd better reducing indentation levelhere.
get_partitions_from_clauses_recursein 0006 does the samething.
 
In the same function, there's a else clause separated from thenclause by a multiline comment. It seems better that the
elseclausehas braces and the comment is in the braces like thefollowing.
 
>  else>  {>    /*>     * Else there are no clauses....>     */>    partindexes = bms_add_range(NULL, 0,
partdesc->nparts- 1);>  }
 



0006:
In get_partitions_from_clauses_recurse, the following commentseems 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 pseudoconstant 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-ORclauses(all AND clauses with volatile function also
returnszero).

+            /* Set partexpr if needed. */
+            if (partattno == 0)
Could you add a description about the meaning of 0 to thecomment of PartitionKeyData something like this?
 | AttrNumber *partattrs;    /* attribute numbers of columns in the |                            * partition key. 0
meanspartexpression */
 

+ #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
Isupposewe should make it clear. Likewise, even it doesn'tactually happen but it formally has a chance to make a
falsematchsince 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 specificrelationship of the access methods' property. Isn't
itreasonablethat add checking that partkey->strategy != 'h'before getting negator?
 

+          commuted->opno = get_commutator(opclause->opno);
Im afraid that get_commutator can return InvalidOid foruser-defined types or by user-defined operator class or
perhapsotherreasons uncertain to me. match_clauses_to_partkey ischecking 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 byfree_parsestate(). It doesn't seem to leak anything in thecontext
andI saw the same thing at other places but it would bebetter to follow it if possible, or add some apology as
acomment..(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 thevalues list of create table syntax. This is not a comment
onthispatch but is that intentional?
 

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center