Re: [HACKERS] path toward faster partition pruning

Hello,

At Wed, 22 Nov 2017 17:59:48 +0900, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote in <df609168-b7fd-4c0b-e9b2-6e398d411e27(at)lab(dot)ntt(dot)co(dot)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