On 2017/12/11 14:31, Amit Langote wrote:
> On 2017/12/09 3:46, Robert Haas wrote:
>> On Fri, Dec 8, 2017 at 5:40 AM, Amit Langote wrote:
>>> I noticed that if you partition using a array type column, partition
>>> pruning using constraint exclusion fails to work due to a minor problem.
>>
>> I guess the question is whether that's guaranteed to be safe. I spent
>> a little bit of time thinking about it and I don't see a problem. The
>> function is careful to check that the opclasses and collations of the
>> OpExprs are compatible, and it is the behavior of the operator that is
>> in question here, not the column type, so your change seems OK to me.
>> But I hope somebody else will also study this, because this stuff is
>> fairly subtle and I would not like to be the one who breaks it.
>
> Thanks for taking a look at it.
>
> I will try to say a little more on why it seems safe. RelabelType node
> exists (if any) on top of a given expression node only to denote that the
> operator for which the node is an input will interpret its result as of
> the type RelableType.resulttype, instead of the node's original type. No
> conversion of values actually occurs before making any decisions that this
> function is in charge of making, because the mismatching types in question
> are known to be binary coercible. Or more to the point, the operator that
> will be used in the proof will give correct answers for the values without
> having to do any conversion of values. IOW, it's okay if we simply drop
> the RelabelType, because it doesn't alter in any way the result of the
> proof that operator_predicate_proof() performs.
>
> That said, I've to come think in this particular case that the
> partitioning code that generates the predicate expression should be a bit
> smarter about the various types it manipulates such that RelabelType won't
> be added in the first place. In contrast, make_op(), that generates an
> OpExpr from the parser representation of a = '{1}' appearing in the
> query's WHERE clause, won't add the RelabelType because the underlying
> type machinery that it invokes is able to conclude that that's
> unnecessary. The original patch may still be worth considering as a
> solution though.
>
> I hope someone else chimes in as well. :)
Bug #15042 [1] seems to be caused by this same problem. There, a
RelabelType node is being slapped (by the partitioning code) on a Var node
of a partition key on enum.
Attached updated patch.
Thanks,
Amit
[1]
https://www.postgresql.org/message-id/151747550943.1247.2111555422760537959%40wrigleys.postgresql.org
From c4e6a456309e97f0391b2bf8b417b8a71cfac778 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Fri, 8 Dec 2017 19:09:31 +0900
Subject: [PATCH v2] Teach operator_predicate_proof() to strip RelabelType
---
src/backend/optimizer/util/predtest.c | 13 +++++---
src/test/regress/expected/partition_prune.out | 45 +++++++++++++++++++++++++++
src/test/regress/sql/partition_prune.sql | 18 +++++++++++
3 files changed, 72 insertions(+), 4 deletions(-)
diff --git a/src/backend/optimizer/util/predtest.c
b/src/backend/optimizer/util/predtest.c
index 8106010329..64b0926641 100644
--- a/src/backend/optimizer/util/predtest.c
+++ b/src/backend/optimizer/util/predtest.c
@@ -1407,6 +1407,11 @@ static const StrategyNumber BT_refute_table[6][6] = {
{none, none, BTEQ, none, none, none} /* NE */
};
+/* Strip expr of the surrounding RelabelType, if any. */
+#define strip_relabel(expr) \
+ ((Node *) (IsA((expr), RelabelType) \
+ ? ((RelabelType *) (expr))->arg \
+ : (expr)))
/*
* operator_predicate_proof
@@ -1503,10 +1508,10 @@ operator_predicate_proof(Expr *predicate, Node *clause,
bool refute_it)
/*
* We have to match up at least one pair of input expressions.
*/
- pred_leftop = (Node *) linitial(pred_opexpr->args);
- pred_rightop = (Node *) lsecond(pred_opexpr->args);
- clause_leftop = (Node *) linitial(clause_opexpr->args);
- clause_rightop = (Node *) lsecond(clause_opexpr->args);
+ pred_leftop = strip_relabel(linitial(pred_opexpr->args));
+ pred_rightop = strip_relabel(lsecond(pred_opexpr->args));
+ clause_leftop = strip_relabel(linitial(clause_opexpr->args));
+ clause_rightop = strip_relabel(lsecond(clause_opexpr->args));
if (equal(pred_leftop, clause_leftop))
{
diff --git a/src/test/regress/expected/partition_prune.out
b/src/test/regress/expected/partition_prune.out
index 348719bd62..cf25c8ec86 100644
--- a/src/test/regress/expected/partition_prune.out
+++ b/src/test/regress/expected/partition_prune.out
@@ -1088,4 +1088,49 @@ explain (costs off) select * from boolpart where a is
not unknown;
Filter: (a IS NOT UNKNOWN)
(7 rows)
+-- array type list partition key
+create table arrpart (a int[]) partition by list (a);
+create table arrpart1 partition of arrpart for values in ('{1}');
+create table arrpart2 partition of arrpart for values in ('{2, 3}', '{4, 5}');
+explain (costs off) select * from arrpart where a = '{1}';
+ QUERY PLAN
+----------------------------------------
+ Append
+ -> Seq Scan on arrpart1
+ Filter: (a = '{1}'::integer[])
+(3 rows)
+
+explain (costs off) select * from arrpart where a = '{1, 2}';
+ QUERY PLAN
+--------------------------
+ Result
+ One-Time Filter: false
+(2 rows)
+
+explain (costs off) select * from arrpart where a in ('{4, 5}', '{1}');
+ QUERY PLAN
+----------------------------------------------------------------------
+ Append
+ -> Seq Scan on arrpart1
+ Filter: ((a = '{4,5}'::integer[]) OR (a = '{1}'::integer[]))
+ -> Seq Scan on arrpart2
+ Filter: ((a = '{4,5}'::integer[]) OR (a = '{1}'::integer[]))
+(5 rows)
+
+drop table arrpart;
+-- enum type list partition key
+create type colors as enum ('green', 'blue');
+create table enumpart (a colors) partition by list (a);
+create table enumpart_green partition of enumpart for values in ('green');
+create table enumpart_blue partition of enumpart for values in ('blue');
+explain (costs off) select * from enumpart where a = 'blue';
+ QUERY PLAN
+--------------------------------------
+ Append
+ -> Seq Scan on enumpart_blue
+ Filter: (a = 'blue'::colors)
+(3 rows)
+
+drop table enumpart;
+drop type colors;
drop table lp, coll_pruning, rlp, mc3p, mc2p, boolpart;
diff --git a/src/test/regress/sql/partition_prune.sql
b/src/test/regress/sql/partition_prune.sql
index 514f8e5ce1..abe1b56c80 100644
--- a/src/test/regress/sql/partition_prune.sql
+++ b/src/test/regress/sql/partition_prune.sql
@@ -152,4 +152,22 @@ 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;
+-- array type list partition key
+create table arrpart (a int[]) partition by list (a);
+create table arrpart1 partition of arrpart for values in ('{1}');
+create table arrpart2 partition of arrpart for values in ('{2, 3}', '{4, 5}');
+explain (costs off) select * from arrpart where a = '{1}';
+explain (costs off) select * from arrpart where a = '{1, 2}';
+explain (costs off) select * from arrpart where a in ('{4, 5}', '{1}');
+drop table arrpart;
+
+-- enum type list partition key
+create type colors as enum ('green', 'blue');
+create table enumpart (a colors) partition by list (a);
+create table enumpart_green partition of enumpart for values in ('green');
+create table enumpart_blue partition of enumpart for values in ('blue');
+explain (costs off) select * from enumpart where a = 'blue';
+drop table enumpart;
+drop type colors;
+
drop table lp, coll_pruning, rlp, mc3p, mc2p, boolpart;
--
2.11.0
