On 2018/03/01 21:56, Robert Haas wrote: > On Tue, Feb 27, 2018 at 4:33 AM, Amit Langote > <langote_amit...@lab.ntt.co.jp> wrote: >> Attached an updated version in which I incorporated some of the revisions >> that David Rowley suggested to OR clauses handling (in partprune.c) that >> he posted as a separate patch on the run-time pruning thread [1]. > > I'm very skeptical about this patch's desire to remove the static > qualifier from evaluate_expr(). Why does this patch need that and > constraint exclusion not need it? Why should this patch not instead > by using eval_const_expressions? partkey_datum_from_expr() is > prepared to give up if evaluate_expr() doesn't return a Const, but > there's nothing in evaluate_expr() to make it give up if, for example, > the input is -- or contains -- a volatile function, e.g. random().
Thinking on this a bit, I have removed the evaluate_expr() business from partkey_datum_from_expr() and thus switched evaluate_expr() back to static. Let me explain why I'd added there in the first place -- if the constant expression received in partkey_datum_from_expr() was not of the same type as that of the partition key, it'd try to coerce_to_target_type() the input expression to the partition key type which may result in a non-Const expression. We'd turn it back into a Const by calling evaluate_expr(). I thought the coercion was needed because we'd be comparing the resulting datum with the partition bound datums using a partition comparison function that would require its arguments to be of given types. But I realized we don't need the coercion. Earlier steps would have determined that the clause from which the expression originated contains an operator that is compatible with the partitioning operator family. If so, the type of the expression in question, even though different from the partition key type, would be binary coercible with it. So, it'd be okay to pass the datum extracted from such expression to the partition comparison function to compare it with datums in PartitionBoundInfo, without performing any coercion. > + if (OidIsValid(get_default_oid_from_partdesc(partdesc))) > + rel->has_default_part = true; > + else > + rel->has_default_part = false; > > This can be written a lot more compactly as rel->has_default_part = > OidIsValid(get_default_oid_from_partdesc(partdesc)); Indeed, will fix. > PartitionPruneContext has no comment explaining its general purpose; I > think it should. Will fix. Thanks, Amit
