Re: [HACKERS] path toward faster partition pruning

Horiguchi-san,

Thanks a lot for the review and sorry it took me a while to reply.

On 2017/11/28 20:39, Kyotaro HORIGUCHI wrote:
> At Wed, 22 Nov 2017 17:59:48 +0900, Amit Langote wrote:
> 0001 and 0002 are under discussion with Robert in another thread.

And now committed [1].

> 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.)

Fixed both.

> 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.

Less indentation sounds good to me to, so fixed.

> 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);
> > }

Done that way.

> 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.

Updated the comment like this:

/*
* classify_partition_bounding_keys() may have found clauses marked
* pseudo-constant that are false that the planner didn't or it may have
* itself found contradictions among clauses.
*/

> + 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).

Hmm, if all AND clauses contained volatile functions, then planner
wouldn't have called here at all. Also, any clauses it passes to
get_partitions_from_clauses() are known to be OK to use for pruning.

> + /* 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?

Sure, done.

>
> | 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)))

That's better, fixed.

> + 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.

In another thread that I recently started [2], Tom seemed to point out
that ScalarArrayOpExpr cannot really be used for comparing an array on LHS
with, say, an array of arrays on RHS. The LHS expression must yield a
scalar value which is compared with individual scalar values in the array
on RHS. The syntax allows arbitrarily multi-dimensional array to be
specified on RHS, but ultimately only looks at the scalar therein. For
example:

select 1 = any (array[array[array[1]]]);
?column?
|---------
t
(1 row)

select 1 = any ('{{{1}}}');
?column?
|---------
t
(1 row)

select 1 = any ('{{{2}}}');
?column?
|---------
f
(1 row)

But for the code in question on which you seemed to have commented, it
only works if the array is presented as a Const that can be deconstructed
to a list of scalar values using deconstruct_array(). If the array was
presented as an ArrayExpr, and its multidims is true, the elements list
cannot simply be assumed to contain Const nodes holding scalar values.
So, we should prohibit that case from proceeding. Although, I haven't
been able to frame a test query that results in such a case.

> + 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?)

Added a free_parsestate() call.

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

Fixed.

>
> + 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.

Fixed.

> + * 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?

Hmm, I guess the original partitioning patch forgot to accept TRUE_P,
FALSE_P as valid partition bound datums along with Sconst, NumericOnly,
and NULL_P. Sent a patch for that in a separate thread [3].

Attached updated patches. As Robert commented [4], I tried to re-arrange
the patches after breaking down the planner patch ("0005 looks like it
might need to be split into smaller patches"). Brief description of each
follows:

[PATCH 1/5] Some interface changes for partition_bound_{cmp/bsearch}

As the name says, it's a preparatory patch to enable the next patch to use
the partition bound binary search function using partial keys. Until now,
callers needed to specify the whole key (specify values for all partition
columns), but a query may not have clauses on all partition columns, so we
should be able to compare such incomplete keys against partition bounds.

[PATCH 2/5] Introduce a get_partitions_from_clauses()

This used to be a last patch in the previous versions. But I moved it
ahead in the list as basic functionality that needs to be in place before
starting to modify the planner to use the same for faster pruning.

To summarize, just like the existing get_partition_for_tuple() that
receives a tuple from ExecFindPartition() and returns the index of the
partition that should contain that tuple, get_partitions_from_clauses()
will receive a set of clauses that are all known to match some partition
key and derive from it and return the set of indexes of partitions that
satisfy all those clauses.

[PATCH 3/5] Move some code of set_append_rel_size to separate
function

First in the series that modifies the planner. Just a preparatory patch
that moves some code.

[PATCH 4/5] More refactoring around partitioned table AppendPath
creation

Another refactoring patch that changes how we manipulate the partitions
when generating AppendPath for partitioned tables.

Actually, there is one behavior change here - partitioned_rels in Append
today carry the RT indexes of even the partitioned child tables that have
been pruned. Patch modifies things so that that doesn't happen anymore.

[PATCH 5/5] Teach planner to use get_partitions_from_clauses()

With this patch, we finally hit the new faster pruning functionality.

Partitions will be pruned even before looking at the individual partition
RelOptInfos. In fact, set_append_rel_size() now only ever looks at
non-pruned partitions.

Since, we can call get_partitions_from_clauses() from only the SELECT
planning code path, UPDATE/DELETE cases still rely on constraint
exclusion. So, we retrieve the partition constraint only in those cases.

Thanks,
Amit

[1]
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=7b88d63a9122646ead60c1afffc248a31d4e457d

[2] https://www.postgresql.org/message-id/7677.1512743642%40sss.pgh.pa.us

[3]
https://www.postgresql.org/message-id/e05c5162-1103-7e37-d1ab-6de3e0afaf70%40lab.ntt.co.jp

[4]
https://www.postgresql.org/message-id/CA%2BTgmoYYrPA21e0y5w2NW2-sbANFR4n2nbrSWEWjzvaa_GNi0g%40mail.gmail.com