On 28 December 2017 at 15:07, David Rowley <david.rowley@2ndquadrant.com> wrote:
> 43. In partition_prune.sql you have a mix of /* */ and -- comments.
> Please just use --
Just a few extras that I found:
44. In match_clauses_to_partkey you're making use of
estimate_expression_value(), I don't think this is safe.
if (IsA(estimate_expression_value(root, rightop), Const))
*contains_const = true;
The only other places I see using this in the planner are for costing
purposes. Also, the header comment for that function says it's not
safe. Particularly "This effectively means that we plan using the
first supplied value of the Param.". If that's the case, then if we're
planning a generic plan, then wouldn't it be possible that the planner
chooses the current supplied parameter value and prune away partitions
based on that value. That would make the plan invalid for any other
parameter, but it's meant to be a generic plan, so we can't do that.
45. Why use a list_copy() here?
/*
* For a nested ArrayExpr, we don't know how to get the
* actual scalar values out into a flat list, so we give
* up doing anything with this ScalarArrayOpExpr.
*/
if (arrexpr->multidims)
continue;
elem_exprs = list_copy(arrexpr->elements);
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
