|From:||Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>|
|Cc:||rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com, david(dot)rowley(at)2ndquadrant(dot)com, robertmhaas(at)gmail(dot)com, dilipbalaut(at)gmail(dot)com, memissemerson(at)gmail(dot)com, pgsql-hackers(at)postgresql(dot)org|
|Subject:||Re: [HACKERS] path toward faster partition pruning|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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.
get_partitions_from_clauses is written as _using_ in it's
comment. (also get_partitions_from_clauses_recurse is _guts in
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
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
> * Else there are no clauses....
> partindexes = bms_add_range(NULL, 0, partdesc->nparts - 1);
In get_partitions_from_clauses_recurse, the following comment
+ * 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
+ 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
+ /* 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,
+ &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
+ 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,
+ if (partition_cmp_args(partkey, partattoff, ge, gt, ge,
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?
NTT Open Source Software Center
|Next Message||Amit Langote||2017-11-28 11:44:47||Re: default range partition and constraint exclusion|
|Previous Message||Daniel Gustafsson||2017-11-28 11:07:53||Re: [HACKERS] Other formats in pset like markdown, rst, mediawiki|