From d758d9702e2f64f08565e18eb6cb7991efa2dc16 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Wed, 9 May 2018 10:51:23 -0300 Subject: [PATCH] Fix assorted partition pruning bugs MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit match_clause_to_partition_key failed to consider COERCION_PATH_ARRAYCOERCE cases in scalar-op-array expressions, so it was possible to crash the server easily. To handle this case properly (ie. prune partitions) we would need to run a bit of executor code during planning. Maybe it can be improved, but for now let's just not crash. Add a test case that used to trigger the crash. Author: Michaël Paquier match_clause_to_partition_key failed to indicate that operators that don't have a commutator in a btree opclass are unsupported. It is possible for this to cause a crash later if such an operator is used in a scalar-op-array expression. Add a test case that used to the crash. Author: Amit Langote One caller of gen_partprune_steps_internal in match_clause_to_partition_key was too optimistic about the former never returning an empty step list. Rid it of its innocence. (Having fixed the bug above, I no longer know how to exploit this, so no test case for it, but it remained a bug.) Revise code flow a little bit, for succintness. Author: Álvaro Herrera Reported-by: Marina Polyakova Reviewed-by: Michaël Paquier Reviewed-by: Amit Langote Reviewed-by: Álvaro Herrera Discussion: https://postgr.es/m/ff8f9bfa485ff961d6bb43e54120485b@postgrespro.ru --- src/backend/partitioning/partprune.c | 58 ++++++++-------- src/test/regress/expected/partition_prune.out | 66 +++++++++++++++++++ src/test/regress/sql/partition_prune.sql | 14 ++++ 3 files changed, 106 insertions(+), 32 deletions(-) diff --git a/src/backend/partitioning/partprune.c b/src/backend/partitioning/partprune.c index 69879bf3a4..58ec2a684d 100644 --- a/src/backend/partitioning/partprune.c +++ b/src/backend/partitioning/partprune.c @@ -573,8 +573,9 @@ get_matching_partitions(PartitionPruneContext *context, List *pruning_steps) * For BoolExpr clauses, we recursively generate steps for each argument, and * return a PartitionPruneStepCombine of their results. * - * The generated steps are added to the context's steps list. Each step is - * assigned a step identifier, unique even across recursive calls. + * The return value is a list of the steps generated, which are also added to + * the context's steps list. Each step is assigned a step identifier, unique + * even across recursive calls. * * If we find clauses that are mutually contradictory, or a pseudoconstant * clause that contains false, we set *contradictory to true and return NIL @@ -1601,6 +1602,7 @@ match_clause_to_partition_key(RelOptInfo *rel, List *elem_exprs, *elem_clauses; ListCell *lc1; + bool contradictory; if (IsA(leftop, RelabelType)) leftop = ((RelabelType *) leftop)->arg; @@ -1619,7 +1621,7 @@ match_clause_to_partition_key(RelOptInfo *rel, * Only allow strict operators. This will guarantee nulls are * filtered. */ - if (!op_strict(saop->opno)) + if (!op_strict(saop_op)) return PARTCLAUSE_UNSUPPORTED; /* Useless if the array has any volatile functions. */ @@ -1652,6 +1654,8 @@ match_clause_to_partition_key(RelOptInfo *rel, if (strategy != BTEqualStrategyNumber) return PARTCLAUSE_UNSUPPORTED; } + else + return PARTCLAUSE_UNSUPPORTED; /* no useful negator */ } /* @@ -1692,7 +1696,7 @@ match_clause_to_partition_key(RelOptInfo *rel, elem_exprs = lappend(elem_exprs, elem_expr); } } - else + else if (IsA(rightop, ArrayExpr)) { ArrayExpr *arrexpr = castNode(ArrayExpr, rightop); @@ -1706,6 +1710,11 @@ match_clause_to_partition_key(RelOptInfo *rel, elem_exprs = arrexpr->elements; } + else + { + /* Give up on any other clause types. */ + return PARTCLAUSE_UNSUPPORTED; + } /* * Now generate a list of clauses, one for each array element, of the @@ -1724,36 +1733,21 @@ match_clause_to_partition_key(RelOptInfo *rel, } /* - * Build a combine step as if for an OR clause or add the clauses to - * the end of the list that's being processed currently. + * If we have an ANY clause and multiple elements, first turn the list + * of clauses into an OR expression. */ if (saop->useOr && list_length(elem_clauses) > 1) - { - Expr *orexpr; - bool contradictory; - - orexpr = makeBoolExpr(OR_EXPR, elem_clauses, -1); - *clause_steps = - gen_partprune_steps_internal(context, rel, list_make1(orexpr), - &contradictory); - if (contradictory) - return PARTCLAUSE_MATCH_CONTRADICT; - - Assert(list_length(*clause_steps) == 1); - return PARTCLAUSE_MATCH_STEPS; - } - else - { - bool contradictory; - - *clause_steps = - gen_partprune_steps_internal(context, rel, elem_clauses, - &contradictory); - if (contradictory) - return PARTCLAUSE_MATCH_CONTRADICT; - Assert(list_length(*clause_steps) >= 1); - return PARTCLAUSE_MATCH_STEPS; - } + elem_clauses = list_make1(makeBoolExpr(OR_EXPR, elem_clauses, -1)); + + /* Finally, generate steps */ + *clause_steps = + gen_partprune_steps_internal(context, rel, elem_clauses, + &contradictory); + if (contradictory) + return PARTCLAUSE_MATCH_CONTRADICT; + else if (*clause_steps == NIL) + return PARTCLAUSE_UNSUPPORTED; /* step generation failed */ + return PARTCLAUSE_MATCH_STEPS; } else if (IsA(clause, NullTest)) { diff --git a/src/test/regress/expected/partition_prune.out b/src/test/regress/expected/partition_prune.out index e0cc5f3393..cf331e79c1 100644 --- a/src/test/regress/expected/partition_prune.out +++ b/src/test/regress/expected/partition_prune.out @@ -1073,6 +1073,72 @@ explain (costs off) select * from boolpart where a is not unknown; Filter: (a IS NOT UNKNOWN) (7 rows) +-- test scalar-to-array operators +create table coercepart (a varchar) partition by list (a); +create table coercepart_ab partition of coercepart for values in ('ab'); +create table coercepart_bc partition of coercepart for values in ('bc'); +create table coercepart_cd partition of coercepart for values in ('cd'); +explain (costs off) select * from coercepart where a in ('ab', to_char(125, '999')); + QUERY PLAN +------------------------------------------------------------------------------------------------------------------------------ + Append + -> Seq Scan on coercepart_ab + Filter: ((a)::text = ANY ((ARRAY['ab'::character varying, (to_char(125, '999'::text))::character varying])::text[])) + -> Seq Scan on coercepart_bc + Filter: ((a)::text = ANY ((ARRAY['ab'::character varying, (to_char(125, '999'::text))::character varying])::text[])) + -> Seq Scan on coercepart_cd + Filter: ((a)::text = ANY ((ARRAY['ab'::character varying, (to_char(125, '999'::text))::character varying])::text[])) +(7 rows) + +explain (costs off) select * from coercepart where a ~ any ('{ab}'); + QUERY PLAN +---------------------------------------------------- + Append + -> Seq Scan on coercepart_ab + Filter: ((a)::text ~ ANY ('{ab}'::text[])) + -> Seq Scan on coercepart_bc + Filter: ((a)::text ~ ANY ('{ab}'::text[])) + -> Seq Scan on coercepart_cd + Filter: ((a)::text ~ ANY ('{ab}'::text[])) +(7 rows) + +explain (costs off) select * from coercepart where a !~ all ('{ab}'); + QUERY PLAN +----------------------------------------------------- + Append + -> Seq Scan on coercepart_ab + Filter: ((a)::text !~ ALL ('{ab}'::text[])) + -> Seq Scan on coercepart_bc + Filter: ((a)::text !~ ALL ('{ab}'::text[])) + -> Seq Scan on coercepart_cd + Filter: ((a)::text !~ ALL ('{ab}'::text[])) +(7 rows) + +explain (costs off) select * from coercepart where a ~ any ('{ab,bc}'); + QUERY PLAN +------------------------------------------------------- + Append + -> Seq Scan on coercepart_ab + Filter: ((a)::text ~ ANY ('{ab,bc}'::text[])) + -> Seq Scan on coercepart_bc + Filter: ((a)::text ~ ANY ('{ab,bc}'::text[])) + -> Seq Scan on coercepart_cd + Filter: ((a)::text ~ ANY ('{ab,bc}'::text[])) +(7 rows) + +explain (costs off) select * from coercepart where a !~ all ('{ab,bc}'); + QUERY PLAN +-------------------------------------------------------- + Append + -> Seq Scan on coercepart_ab + Filter: ((a)::text !~ ALL ('{ab,bc}'::text[])) + -> Seq Scan on coercepart_bc + Filter: ((a)::text !~ ALL ('{ab,bc}'::text[])) + -> Seq Scan on coercepart_cd + Filter: ((a)::text !~ ALL ('{ab,bc}'::text[])) +(7 rows) + +drop table coercepart; -- -- some more cases -- diff --git a/src/test/regress/sql/partition_prune.sql b/src/test/regress/sql/partition_prune.sql index 6b7f57ab41..1464f4dcd9 100644 --- a/src/test/regress/sql/partition_prune.sql +++ b/src/test/regress/sql/partition_prune.sql @@ -152,6 +152,20 @@ explain (costs off) select * from boolpart where a is not true and a is not fals explain (costs off) select * from boolpart where a is unknown; explain (costs off) select * from boolpart where a is not unknown; +-- test scalar-to-array operators +create table coercepart (a varchar) partition by list (a); +create table coercepart_ab partition of coercepart for values in ('ab'); +create table coercepart_bc partition of coercepart for values in ('bc'); +create table coercepart_cd partition of coercepart for values in ('cd'); + +explain (costs off) select * from coercepart where a in ('ab', to_char(125, '999')); +explain (costs off) select * from coercepart where a ~ any ('{ab}'); +explain (costs off) select * from coercepart where a !~ all ('{ab}'); +explain (costs off) select * from coercepart where a ~ any ('{ab,bc}'); +explain (costs off) select * from coercepart where a !~ all ('{ab,bc}'); + +drop table coercepart; + -- -- some more cases -- -- 2.40.0