Hi.
On 2018/05/09 7:05, Alvaro Herrera wrote:
> So I found that this query also crashed (using your rig),
>
> 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 ~ any ('{ab}');
>
> The reason for this crash is that gen_partprune_steps_internal() is
> unable to generate any steps for the clause -- which is natural, since
> the operator is not in a btree opclass. There are various callers
> of gen_partprune_steps_internal that are aware that it could return an
> empty set of steps, but this one in match_clause_to_partition_key for
> the ScalarArrayOpExpr case was being a bit too optimistic.
Yeah, good catch! That fixes the crash, but looking around that code a
bit, it seems that we shouldn't even have gotten up to the point you're
proposing to fix. If saop_op is not in the partitioning opfamily, it
should have bailed out much sooner, that is, here:
/*
* In case of NOT IN (..), we get a '<>', which we handle if list
* partitioning is in use and we're able to confirm that it's negator
* is a btree equality operator belonging to the partitioning operator
* family.
*/
if (!op_in_opfamily(saop_op, partopfamily))
{
<snip>
negator = get_negator(saop_op);
if (OidIsValid(negator) && op_in_opfamily(negator, partopfamily))
{
<snip>
}
+ else
+ /* otherwise, unsupported! */
+ return PARTCLAUSE_UNSUPPORTED;
Let me propose that we also have this along with the rest of the changes
you made in that part of the function. So, attached is an updated patch.
Thanks,
Amit
diff --git a/src/backend/partitioning/partprune.c
b/src/backend/partitioning/partprune.c
index f954b92a6b..be9ea8a6db 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -571,8 +571,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
@@ -1599,6 +1600,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;
@@ -1617,7 +1619,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. */
@@ -1650,6 +1652,9 @@ match_clause_to_partition_key(RelOptInfo *rel,
if (strategy != BTEqualStrategyNumber)
return PARTCLAUSE_UNSUPPORTED;
}
+ else
+ /* otherwise, unsupported! */
+ return PARTCLAUSE_UNSUPPORTED;
}
/*
@@ -1690,7 +1695,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);
@@ -1704,6 +1709,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
@@ -1722,36 +1732,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;
+ elem_clauses = list_make1(makeBoolExpr(OR_EXPR,
elem_clauses, -1));
- 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;
- }
+ /* 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
--
