Thanks Dilip for reviewing.
On 2017/10/31 1:50, Dilip Kumar wrote:
> On Mon, Oct 30, 2017 at 12:20 PM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> On 2017/10/30 14:55, Amit Langote wrote:
>>> Fixed in the attached updated patch, along with a new test in 0001 to
>>> cover this case. Also, made a few tweaks to 0003 and 0005 (moved some
>>> code from the former to the latter) around the handling of ScalarArrayOpExprs.
>>
>> Sorry, I'd forgotten to include some changes.
>>
>> In the previous versions, RT index of the table needed to be passed to
>> partition.c, which I realized is no longer needed, so I removed that
>> requirement from the interface. As a result, patches 0002 and 0003 have
>> changed in this version.
>
> Some Minor comments:
>
> + * get_rel_partitions
> + * Return the list of partitions of rel that pass the clauses mentioned
> + * rel->baserestrictinfo
> + *
> + * Returned list contains the AppendRelInfos of chosen partitions.
> + */
> +static List *
> +get_append_rel_partitions(PlannerInfo *root,
>
> Function name in function header is not correct.
Fixed.
> + !DatumGetBool(((Const *) clause)->constvalue))
> + {
> + *constfalse = true;
> + continue;
>
> DatumGetBool will return true if the first byte of constvalue is
> nonzero otherwise
> false. IIUC, this is not the intention here. Or I am missing something?
This coding pattern is in use in quite a few places; see for example in
restriction_is_constant_false() and many others like
relation_excluded_by_constraints(), negate_clause(), etc.
If a RestrictInfo is marked pseudoconstant=true, then the clause therein
must be a Const with constvalue computing to 0 if the clause is false, so
that DatumGetBool(constvalue) returns boolean false and non-zero otherwise.
> + * clauses in this function ourselves, for example, having both a > 1 and
> + * a = 0 the list
>
> a = 0 the list -> a = 0 in the list
Right, fixed.
>
> +static bool
> +partkey_datum_from_expr(const Expr *expr, Datum *value)
> +{
> + /*
> + * Add more expression types here as needed to support higher-level
> + * code.
> + */
> + switch (nodeTag(expr))
> + {
> + case T_Const:
> + *value = ((Const *) expr)->constvalue;
> + return true;
>
> I think for evaluating other expressions (e.g. T_Param) we will have
> to pass ExprContext to this function.
That's right.
> Or we can do something cleaner
> because if we want to access the ExprContext inside
> partkey_datum_from_expr then we may need to pass it to
> "get_partitions_from_clauses" which is a common function for optimizer
> and executor.
Yeah, I've thought about that a little. Since nothing else but the
planner calls it now and the planner doesn't itself have its hands on the
ExprContext that would be necessary for computing something like Params, I
left it out of the interface for now. That said, I *am* actually thinking
about some interface changes that would be necessary for some other
unrelated functionality/optimizations that callers like the run-time
pruning code would expect of get_partitions_from_clauses(). We can design
the interface extension such that the aforementioned ExprContext is passed
together.
Attached updated version of the patches addressing some of your comments
above and fixing a bug that Rajkumar reported [1]. As mentioned there,
I'm including here a patch (the 0005 of the attached) to tweak the default
range partition constraint to be explicit about null values that it might
contain. So, there are 6 patches now and what used to be patch 0005 in
the previous set is patch 0006 in this version of the set.
Thanks,
Amit
[1]
https://www.postgresql.org/message-id/cd5a2d2e-0957-042c-40c2-06033fe0abf2@lab.ntt.co.jp
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers