On 28 December 2017 at 15:07, David Rowley <david.row...@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
