no partition pruning when partitioning using array type
Hi.
I noticed that if you partition using a array type column, partition
pruning using constraint exclusion fails to work due to a minor problem.
Example:
create table p (a int[]) partition by list (a);
create table p1 partition of p for values in ('{1}');
create table p1 partition of p for values in ('{2, 3}', '{4, 5}');
explain select a from p where a = '{1}';
QUERY PLAN
|---------------------------------------------------------
Append (cost=0.00..54.00 rows=14 width=32)
-> Seq Scan on p1 (cost=0.00..27.00 rows=7 width=32)
Filter: (a = '{1}'::integer[])
-> Seq Scan on p2 (cost=0.00..27.00 rows=7 width=32)
Filter: (a = '{1}'::integer[])
explain select a from p where a = '{2, 3}';
QUERY PLAN
|---------------------------------------------------------
Append (cost=0.00..54.00 rows=14 width=32)
-> Seq Scan on p1 (cost=0.00..27.00 rows=7 width=32)
Filter: (a = '{2,3}'::integer[])
-> Seq Scan on p2 (cost=0.00..27.00 rows=7 width=32)
Filter: (a = '{2,3}'::integer[])
(5 rows)
In the case of array type partition key, make_partition_op_expr() will
have to put a RelabelType node on top of the partition key Var, after
having selected an = operator from the array_ops family. The RelabelType
causes operator_predicate_proof() to fail to consider predicate leftop and
clause leftop as equal, because only one of them ends up having the
RelabelType attached to it.
As a simple measure, the attached patch teaches operator_predicate_proof()
to strip RelabelType nodes from all the nodes it compares using equal().
I also added a relevant test in partition_prune.sql.
Thoughts?
Thanks,
Amit
Attachments:
0001-Teach-operator_predicate_proof-to-strip-RelabelType.patchtext/plain; charset=UTF-8; name=0001-Teach-operator_predicate_proof-to-strip-RelabelType.patchDownload
From 2a25202f2f0a415c6884c28fd5bc76243507d5c4 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Fri, 8 Dec 2017 19:09:31 +0900
Subject: [PATCH] Teach operator_predicate_proof() to strip RelabelType
---
src/backend/optimizer/util/predtest.c | 13 ++++++++----
src/test/regress/expected/partition_prune.out | 29 +++++++++++++++++++++++++++
src/test/regress/sql/partition_prune.sql | 8 ++++++++
3 files changed, 46 insertions(+), 4 deletions(-)
diff --git a/src/backend/optimizer/util/predtest.c b/src/backend/optimizer/util/predtest.c
index 134460cc13..d0f6278984 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 aabb0240a9..7c78f11d55 100644
--- a/src/test/regress/expected/partition_prune.out
+++ b/src/test/regress/expected/partition_prune.out
@@ -1092,4 +1092,33 @@ 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 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..c353387bb9 100644
--- a/src/test/regress/sql/partition_prune.sql
+++ b/src/test/regress/sql/partition_prune.sql
@@ -152,4 +152,12 @@ 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 lp, coll_pruning, rlp, mc3p, mc2p, boolpart;
--
2.11.0
On Fri, Dec 8, 2017 at 5:40 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> 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.Example:
create table p (a int[]) partition by list (a);
create table p1 partition of p for values in ('{1}');
create table p1 partition of p for values in ('{2, 3}', '{4, 5}');explain select a from p where a = '{1}';
QUERY PLAN
|---------------------------------------------------------
Append (cost=0.00..54.00 rows=14 width=32)
-> Seq Scan on p1 (cost=0.00..27.00 rows=7 width=32)
Filter: (a = '{1}'::integer[])
-> Seq Scan on p2 (cost=0.00..27.00 rows=7 width=32)
Filter: (a = '{1}'::integer[])explain select a from p where a = '{2, 3}';
QUERY PLAN
|---------------------------------------------------------
Append (cost=0.00..54.00 rows=14 width=32)
-> Seq Scan on p1 (cost=0.00..27.00 rows=7 width=32)
Filter: (a = '{2,3}'::integer[])
-> Seq Scan on p2 (cost=0.00..27.00 rows=7 width=32)
Filter: (a = '{2,3}'::integer[])
(5 rows)In the case of array type partition key, make_partition_op_expr() will
have to put a RelabelType node on top of the partition key Var, after
having selected an = operator from the array_ops family. The RelabelType
causes operator_predicate_proof() to fail to consider predicate leftop and
clause leftop as equal, because only one of them ends up having the
RelabelType attached to it.As a simple measure, the attached patch teaches operator_predicate_proof()
to strip RelabelType nodes from all the nodes it compares using equal().
I also added a relevant test in partition_prune.sql.
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.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 2017/12/09 3:46, Robert Haas wrote:
On Fri, Dec 8, 2017 at 5:40 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> 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.Example:
create table p (a int[]) partition by list (a);
create table p1 partition of p for values in ('{1}');
create table p1 partition of p for values in ('{2, 3}', '{4, 5}');explain select a from p where a = '{1}';
QUERY PLAN
|---------------------------------------------------------
Append (cost=0.00..54.00 rows=14 width=32)
-> Seq Scan on p1 (cost=0.00..27.00 rows=7 width=32)
Filter: (a = '{1}'::integer[])
-> Seq Scan on p2 (cost=0.00..27.00 rows=7 width=32)
Filter: (a = '{1}'::integer[])explain select a from p where a = '{2, 3}';
QUERY PLAN
|---------------------------------------------------------
Append (cost=0.00..54.00 rows=14 width=32)
-> Seq Scan on p1 (cost=0.00..27.00 rows=7 width=32)
Filter: (a = '{2,3}'::integer[])
-> Seq Scan on p2 (cost=0.00..27.00 rows=7 width=32)
Filter: (a = '{2,3}'::integer[])
(5 rows)In the case of array type partition key, make_partition_op_expr() will
have to put a RelabelType node on top of the partition key Var, after
having selected an = operator from the array_ops family. The RelabelType
causes operator_predicate_proof() to fail to consider predicate leftop and
clause leftop as equal, because only one of them ends up having the
RelabelType attached to it.As a simple measure, the attached patch teaches operator_predicate_proof()
to strip RelabelType nodes from all the nodes it compares using equal().
I also added a relevant test in partition_prune.sql.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. :)
Thanks,
Amit
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]/messages/by-id/151747550943.1247.2111555422760537959@wrigleys.postgresql.org 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]: /messages/by-id/151747550943.1247.2111555422760537959@wrigleys.postgresql.org
/messages/by-id/151747550943.1247.2111555422760537959@wrigleys.postgresql.org
Attachments:
v2-0001-Teach-operator_predicate_proof-to-strip-RelabelTy.patchtext/plain; charset=UTF-8; name=v2-0001-Teach-operator_predicate_proof-to-strip-RelabelTy.patchDownload
From c4e6a456309e97f0391b2bf8b417b8a71cfac778 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@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
On Thu, Feb 1, 2018 at 4:42 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
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.
Can I get anyone else to weigh in on whether this is likely to be
safe? Paging people who understand constraint exclusion...
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 2018/02/02 0:20, Robert Haas wrote:
On Thu, Feb 1, 2018 at 4:42 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote: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.
Can I get anyone else to weigh in on whether this is likely to be
safe? Paging people who understand constraint exclusion...
Added this to CF (actually moved to the September one after first having
added it to the CF that is just getting started).
It seems to me that we don't need to go with my originally proposed
approach to teach predtest.c to strip RelabelType nodes. Apparently, it's
only partition.c that's adding the RelableType node around partition key
nodes in such cases. That is, in the case of having an array, record,
enum, and range type as the partition key. No other part of the system
ends up adding one as far as I can see. Parser's make_op(), for example,
that is used to generate an OpExpr for a qual involving the partition key,
won't put RelabelType around the partition key node if the operator in
question has "pseudo"-types listed as declared types of its left and right
arguments.
So I revised the patch to drop all the predtest.c changes and instead
modify get_partition_operator() to avoid generating RelabelType in such
cases. Please find it attached.
Thanks,
Amit
Attachments:
v3-0001-In-get_partition_operator-avoid-RelabelType-in-so.patchtext/plain; charset=UTF-8; name=v3-0001-In-get_partition_operator-avoid-RelabelType-in-so.patchDownload
From b0ebef9526a2abb162e877bc3b73d8194ffb1d2f Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Fri, 8 Dec 2017 19:09:31 +0900
Subject: [PATCH v3] In get_partition_operator(), avoid RelabelType in some
cases
We never have pg_am catalog entries with "real" array, enum, record,
range types as leftop and rightop types. Instead, operators manipulating
such types have entries with "pseudo-types" listed as leftop and rightop
types. For example, for enums, all operator entries are marked with
anyenum as their leftop and rightop types. So, if we pass "real" type
OIDs to get_opfamily_member(), we get back an InvalidOid for the
aforementioned type categories. We'd normally ask the caller of
get_partition_operator to add a RelabelType in such case. But we don't
need to in these cases, as the rest of the system doesn't.
This fixes the issue that the constraint exclusion code would reject
matching partition key quals with restrictions due to these extraneous
RelabelType nodes surrounding partition key expressions generated
by partition.c.
Also, ruleutils.c can resolve the operator names properly when deparsing
an internally generated partition qual expressions.
---
src/backend/catalog/partition.c | 12 ++++++-
src/test/regress/expected/create_table.out | 2 +-
src/test/regress/expected/partition_prune.out | 45 +++++++++++++++++++++++++++
src/test/regress/sql/partition_prune.sql | 18 +++++++++++
4 files changed, 75 insertions(+), 2 deletions(-)
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index fcf7655553..cf71ba174e 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1605,7 +1605,17 @@ get_partition_operator(PartitionKey key, int col, StrategyNumber strategy,
elog(ERROR, "missing operator %d(%u,%u) in opfamily %u",
strategy, key->partopcintype[col], key->partopcintype[col],
key->partopfamily[col]);
- *need_relabel = true;
+
+ /*
+ * For certain type categories, there don't exist pg_amop entries with
+ * the given type OID as the operator's left and right operand type.
+ * Instead, the entries have corresponding pseudo-types listed as the
+ * left and right operand type; for example, anyenum for an enum type.
+ * For such cases, it's not necessary to add the RelabelType node,
+ * because other parts of the sytem won't add one either.
+ */
+ if (get_typtype(key->partopcintype[col]) != TYPTYPE_PSEUDO)
+ *need_relabel = true;
}
else
*need_relabel = false;
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 39a963888d..7764da3152 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -880,6 +880,6 @@ CREATE TABLE arrlp12 PARTITION OF arrlp FOR VALUES IN ('{1}', '{2}');
--------+-----------+-----------+----------+---------+----------+--------------+-------------
a | integer[] | | | | extended | |
Partition of: arrlp FOR VALUES IN ('{1}', '{2}')
-Partition constraint: ((a IS NOT NULL) AND (((a)::anyarray OPERATOR(pg_catalog.=) '{1}'::integer[]) OR ((a)::anyarray OPERATOR(pg_catalog.=) '{2}'::integer[])))
+Partition constraint: ((a IS NOT NULL) AND ((a = '{1}'::integer[]) OR (a = '{2}'::integer[])))
DROP TABLE arrlp;
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
On 2018/03/01 17:16, Amit Langote wrote:
Added this to CF (actually moved to the September one after first having
added it to the CF that is just getting started).It seems to me that we don't need to go with my originally proposed
approach to teach predtest.c to strip RelabelType nodes. Apparently, it's
only partition.c that's adding the RelableType node around partition key
nodes in such cases. That is, in the case of having an array, record,
enum, and range type as the partition key. No other part of the system
ends up adding one as far as I can see. Parser's make_op(), for example,
that is used to generate an OpExpr for a qual involving the partition key,
won't put RelabelType around the partition key node if the operator in
question has "pseudo"-types listed as declared types of its left and right
arguments.So I revised the patch to drop all the predtest.c changes and instead
modify get_partition_operator() to avoid generating RelabelType in such
cases. Please find it attached.
I would like to revisit this as a bug fix for get_partition_operator() to
be applied to both PG 10 and HEAD. In the former case, it fixes the bug
that constraint exclusion code will fail to prune correctly when partition
key is of array, enum, range, or record type due to the structural
mismatch between the OpExpr that partitioning code generates and one that
the parser generates for WHERE clauses involving partition key columns.
In HEAD, since we already fixed that case in e5dcbb88a15d [1]https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=e5dcbb88a15d which is a
different piece of code anyway, the patch only serves to improve the
deparse output emitted by ruleutils.c for partition constraint expressions
where pseudo-type partition key is involved. The change can be seen in
the updated test output for create_table test.
Attached are patches for PG 10 and HEAD, which implement a slightly
different approach to fixing this. Now, instead of passing the partition
key's type as lefttype and righttype to look up the operator, the operator
class declared type is passed. Also, we now decide whether to create a
RelabelType node on top based on whether the partition key's type is
different from the selected operator's input type, with exception for
pseudo-type types.
Thanks,
Amit
[1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=e5dcbb88a15d
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=e5dcbb88a15d
Attachments:
PG10-v1-0001-Fix-how-get_partition_operator-looks-up-the-opera.patchtext/plain; charset=UTF-8; name=PG10-v1-0001-Fix-how-get_partition_operator-looks-up-the-opera.patchDownload
From 9f0dff4e6f3c7e8204a73ef0734e38056de08571 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Tue, 8 May 2018 14:31:37 +0900
Subject: [PATCH v1] Fix how get_partition_operator looks up the operator
Instead of looking up using the underlying partition key's type as the
operator's lefttype and righttype, use the partition operator class
declared input type, which works reliably even in the cases where
pseudo-types are involved. Also, make its decision whether a
RelableType is needed depend on a different criteria than what's
currently there. That is, only add a RelabelType if the partition
key's type is different from the selected operator's input types.
---
src/backend/catalog/partition.c | 49 +++++++++++++-----------------
src/test/regress/expected/create_table.out | 2 +-
2 files changed, 22 insertions(+), 29 deletions(-)
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 17704f36b9..5603a5d992 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1176,40 +1176,33 @@ get_partition_operator(PartitionKey key, int col, StrategyNumber strategy,
Oid operoid;
/*
- * First check if there exists an operator of the given strategy, with
- * this column's type as both its lefttype and righttype, in the
- * partitioning operator family specified for the column.
+ * Get the operator in the partitioning operator family using the operator
+ * class declared input type as both its lefttype and righttype.
*/
operoid = get_opfamily_member(key->partopfamily[col],
- key->parttypid[col],
- key->parttypid[col],
+ key->partopcintype[col],
+ key->partopcintype[col],
strategy);
+ if (!OidIsValid(operoid))
+ elog(ERROR, "missing operator %d(%u,%u) in partition opfamily %u",
+ strategy, key->partopcintype[col], key->partopcintype[col],
+ key->partopfamily[col]);
/*
- * If one doesn't exist, we must resort to using an operator in the same
- * operator family but with the operator class declared input type. It is
- * OK to do so, because the column's type is known to be binary-coercible
- * with the operator class input type (otherwise, the operator class in
- * question would not have been accepted as the partitioning operator
- * class). We must however inform the caller to wrap the non-Const
- * expression with a RelabelType node to denote the implicit coercion. It
- * ensures that the resulting expression structurally matches similarly
- * processed expressions within the optimizer.
+ * If the partition key column is not of the same type as what the
+ * operator expects, we'll need to tell the caller to wrap the non-Const
+ * expression with a RelableType node in most cases, because that's what
+ * the parser would do for similar expressions (such as WHERE clauses
+ * involving this column and operator.) The intention is to make the
+ * expression that we generate here structurally match with expression
+ * generated elsewhere. We don't need the RelableType node for certain
+ * types though, because parse_coerce.c won't emit one either.
*/
- if (!OidIsValid(operoid))
- {
- operoid = get_opfamily_member(key->partopfamily[col],
- key->partopcintype[col],
- key->partopcintype[col],
- strategy);
- if (!OidIsValid(operoid))
- elog(ERROR, "missing operator %d(%u,%u) in opfamily %u",
- strategy, key->partopcintype[col], key->partopcintype[col],
- key->partopfamily[col]);
- *need_relabel = true;
- }
- else
- *need_relabel = false;
+ *need_relabel = (key->parttypid[col] != key->partopcintype[col] &&
+ !(key->partopcintype[col] == ANYARRAYOID ||
+ key->partopcintype[col] == ANYENUMOID ||
+ key->partopcintype[col] == ANYRANGEOID ||
+ key->partopcintype[col] == RECORDOID));
return operoid;
}
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 68dda4c0db..1caa3afbbf 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -808,7 +808,7 @@ CREATE TABLE arrlp12 PARTITION OF arrlp FOR VALUES IN ('{1}', '{2}');
--------+-----------+-----------+----------+---------+----------+--------------+-------------
a | integer[] | | | | extended | |
Partition of: arrlp FOR VALUES IN ('{1}', '{2}')
-Partition constraint: ((a IS NOT NULL) AND (((a)::anyarray OPERATOR(pg_catalog.=) '{1}'::integer[]) OR ((a)::anyarray OPERATOR(pg_catalog.=) '{2}'::integer[])))
+Partition constraint: ((a IS NOT NULL) AND ((a = '{1}'::integer[]) OR (a = '{2}'::integer[])))
DROP TABLE arrlp;
-- partition on boolean column
--
2.11.0
HEAD-v4-0001-Fix-how-get_partition_operator-looks-up-the-opera.patchtext/plain; charset=UTF-8; name=HEAD-v4-0001-Fix-how-get_partition_operator-looks-up-the-opera.patchDownload
From a4af5f05e6c3b79e0adfd99925cf9eba93a0f390 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Tue, 8 May 2018 14:57:47 +0900
Subject: [PATCH v4] Fix how get_partition_operator looks up the operator
Instead of looking up using the underlying partition key's type as the
operator's lefttype and righttype, use the partition operator class
declared input type, which works reliably even in the cases where
pseudo-types are involved. Also, make its decision whether a
RelableType is needed depend on a different criteria than what's
currently there. That is, only add a RelabelType if the partition
key's type is different from the selected operator's input types.
---
src/backend/partitioning/partbounds.c | 49 +++++++++++++-----------------
src/test/regress/expected/create_table.out | 2 +-
2 files changed, 22 insertions(+), 29 deletions(-)
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index 09c7c3e252..3b988fb1cf 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -1154,40 +1154,33 @@ get_partition_operator(PartitionKey key, int col, StrategyNumber strategy,
Oid operoid;
/*
- * First check if there exists an operator of the given strategy, with
- * this column's type as both its lefttype and righttype, in the
- * partitioning operator family specified for the column.
+ * Get the operator in the partitioning operator family using the operator
+ * class declared input type as both its lefttype and righttype.
*/
operoid = get_opfamily_member(key->partopfamily[col],
- key->parttypid[col],
- key->parttypid[col],
+ key->partopcintype[col],
+ key->partopcintype[col],
strategy);
+ if (!OidIsValid(operoid))
+ elog(ERROR, "missing operator %d(%u,%u) in partition opfamily %u",
+ strategy, key->partopcintype[col], key->partopcintype[col],
+ key->partopfamily[col]);
/*
- * If one doesn't exist, we must resort to using an operator in the same
- * operator family but with the operator class declared input type. It is
- * OK to do so, because the column's type is known to be binary-coercible
- * with the operator class input type (otherwise, the operator class in
- * question would not have been accepted as the partitioning operator
- * class). We must however inform the caller to wrap the non-Const
- * expression with a RelabelType node to denote the implicit coercion. It
- * ensures that the resulting expression structurally matches similarly
- * processed expressions within the optimizer.
+ * If the partition key column is not of the same type as what the
+ * operator expects, we'll need to tell the caller to wrap the non-Const
+ * expression with a RelableType node in most cases, because that's what
+ * the parser would do for similar expressions (such as WHERE clauses
+ * involving this column and operator.) The intention is to make the
+ * expression that we generate here structurally match with expression
+ * generated elsewhere. We don't need the RelableType node for certain
+ * types though, because parse_coerce.c won't emit one either.
*/
- if (!OidIsValid(operoid))
- {
- operoid = get_opfamily_member(key->partopfamily[col],
- key->partopcintype[col],
- key->partopcintype[col],
- strategy);
- if (!OidIsValid(operoid))
- elog(ERROR, "missing operator %d(%u,%u) in opfamily %u",
- strategy, key->partopcintype[col], key->partopcintype[col],
- key->partopfamily[col]);
- *need_relabel = true;
- }
- else
- *need_relabel = false;
+ *need_relabel = (key->parttypid[col] != key->partopcintype[col] &&
+ !(key->partopcintype[col] == ANYARRAYOID ||
+ key->partopcintype[col] == ANYENUMOID ||
+ key->partopcintype[col] == ANYRANGEOID ||
+ key->partopcintype[col] == RECORDOID));
return operoid;
}
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 470fca0cab..6c945a8d49 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -882,7 +882,7 @@ CREATE TABLE arrlp12 PARTITION OF arrlp FOR VALUES IN ('{1}', '{2}');
--------+-----------+-----------+----------+---------+----------+--------------+-------------
a | integer[] | | | | extended | |
Partition of: arrlp FOR VALUES IN ('{1}', '{2}')
-Partition constraint: ((a IS NOT NULL) AND (((a)::anyarray OPERATOR(pg_catalog.=) '{1}'::integer[]) OR ((a)::anyarray OPERATOR(pg_catalog.=) '{2}'::integer[])))
+Partition constraint: ((a IS NOT NULL) AND ((a = '{1}'::integer[]) OR (a = '{2}'::integer[])))
DROP TABLE arrlp;
-- partition on boolean column
--
2.11.0
On 05/08/2018 02:18 AM, Amit Langote wrote:
On 2018/03/01 17:16, Amit Langote wrote:
Added this to CF (actually moved to the September one after first having
added it to the CF that is just getting started).It seems to me that we don't need to go with my originally proposed
approach to teach predtest.c to strip RelabelType nodes. Apparently, it's
only partition.c that's adding the RelableType node around partition key
nodes in such cases. That is, in the case of having an array, record,
enum, and range type as the partition key. No other part of the system
ends up adding one as far as I can see. Parser's make_op(), for example,
that is used to generate an OpExpr for a qual involving the partition key,
won't put RelabelType around the partition key node if the operator in
question has "pseudo"-types listed as declared types of its left and right
arguments.So I revised the patch to drop all the predtest.c changes and instead
modify get_partition_operator() to avoid generating RelabelType in such
cases. Please find it attached.I would like to revisit this as a bug fix for get_partition_operator() to
be applied to both PG 10 and HEAD. In the former case, it fixes the bug
that constraint exclusion code will fail to prune correctly when partition
key is of array, enum, range, or record type due to the structural
mismatch between the OpExpr that partitioning code generates and one that
the parser generates for WHERE clauses involving partition key columns.In HEAD, since we already fixed that case in e5dcbb88a15d [1] which is a
different piece of code anyway, the patch only serves to improve the
deparse output emitted by ruleutils.c for partition constraint expressions
where pseudo-type partition key is involved. The change can be seen in
the updated test output for create_table test.Attached are patches for PG 10 and HEAD, which implement a slightly
different approach to fixing this. Now, instead of passing the partition
key's type as lefttype and righttype to look up the operator, the operator
class declared type is passed. Also, we now decide whether to create a
RelabelType node on top based on whether the partition key's type is
different from the selected operator's input type, with exception for
pseudo-type types.Thanks,
Amit[1]
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=e5dcbb88a15d
Since this email we have branched off REL_11_STABLE. Do we want to
consider this as a bug fix for Release 11? If so, should we add it to
the open items list?
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018-May-08, Amit Langote wrote:
I would like to revisit this as a bug fix for get_partition_operator() to
be applied to both PG 10 and HEAD. In the former case, it fixes the bug
that constraint exclusion code will fail to prune correctly when partition
key is of array, enum, range, or record type due to the structural
mismatch between the OpExpr that partitioning code generates and one that
the parser generates for WHERE clauses involving partition key columns.
Interesting patchset. Didn't read your previous v2, v3 versions; I only
checked your latest, v1 (???).
I'm wondering about the choice of OIDs in the new test. I wonder if
it's possible to get ANYNONARRAY (or others) by way of having a
polymorphic function that passes its polymorphic argument in a qual. I
suppose it won't do anything in v10, or will it? Worth checking :-)
Why not use IsPolymorphicType? Also, I think it'd be good to have tests
for all these cases (even in v10), just to make sure we don't break it
going forward. At least the array case is clearly broken today ...
A test for the RECORDOID case would be particularly welcome, since it's
somehow different from the other cases. (I didn't understand why did
you remove the test in the latest version.)
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018/07/07 0:13, Andrew Dunstan wrote:
On 05/08/2018 02:18 AM, Amit Langote wrote:
On 2018/03/01 17:16, Amit Langote wrote:
Added this to CF (actually moved to the September one after first having
added it to the CF that is just getting started).It seems to me that we don't need to go with my originally proposed
approach to teach predtest.c to strip RelabelType nodes. Apparently, it's
only partition.c that's adding the RelableType node around partition key
nodes in such cases. That is, in the case of having an array, record,
enum, and range type as the partition key. No other part of the system
ends up adding one as far as I can see. Parser's make_op(), for example,
that is used to generate an OpExpr for a qual involving the partition key,
won't put RelabelType around the partition key node if the operator in
question has "pseudo"-types listed as declared types of its left and right
arguments.So I revised the patch to drop all the predtest.c changes and instead
modify get_partition_operator() to avoid generating RelabelType in such
cases. Please find it attached.I would like to revisit this as a bug fix for get_partition_operator() to
be applied to both PG 10 and HEAD. In the former case, it fixes the bug
that constraint exclusion code will fail to prune correctly when partition
key is of array, enum, range, or record type due to the structural
mismatch between the OpExpr that partitioning code generates and one that
the parser generates for WHERE clauses involving partition key columns.In HEAD, since we already fixed that case in e5dcbb88a15d [1] which is a
different piece of code anyway, the patch only serves to improve the
deparse output emitted by ruleutils.c for partition constraint expressions
where pseudo-type partition key is involved. The change can be seen in
the updated test output for create_table test.Attached are patches for PG 10 and HEAD, which implement a slightly
different approach to fixing this. Now, instead of passing the partition
key's type as lefttype and righttype to look up the operator, the operator
class declared type is passed. Also, we now decide whether to create a
RelabelType node on top based on whether the partition key's type is
different from the selected operator's input type, with exception for
pseudo-type types.Thanks,
Amit[1]
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=e5dcbb88a15dSince this email we have branched off REL_11_STABLE. Do we want to
consider this as a bug fix for Release 11? If so, should we add it to the
open items list?
The code being fixed with the latest patch is not new in PG 11, it's
rather PG 10 code. That code affects how pruning works in PG 10 (used to
affect PG 11 until we rewrote the partition pruning code). I think it's a
good idea to apply this patch to all branches, as the code is not specific
to partition pruning and has its benefits even for HEAD and PG 11.
For PG 11 and HEAD, the benefit of this patch can be thought of as just
cosmetic, because it only affects the ruleutils.c's deparse output for
partition constraint expressions when showing it in the psql output for
example.
In PG 10, it directly affects the planner behavior whereby having a
RelabelType redundantly in a partition constraint expression limits the
planner's ability to do partition pruning with it.
Thanks,
Amit
Thanks for taking a look.
On 2018/07/07 9:19, Alvaro Herrera wrote:
On 2018-May-08, Amit Langote wrote:
I would like to revisit this as a bug fix for get_partition_operator() to
be applied to both PG 10 and HEAD. In the former case, it fixes the bug
that constraint exclusion code will fail to prune correctly when partition
key is of array, enum, range, or record type due to the structural
mismatch between the OpExpr that partitioning code generates and one that
the parser generates for WHERE clauses involving partition key columns.Interesting patchset. Didn't read your previous v2, v3 versions; I only
checked your latest, v1 (???).
Sorry, I think I messed up version numbering there.
I'm wondering about the choice of OIDs in the new test. I wonder if
it's possible to get ANYNONARRAY (or others) by way of having a
polymorphic function that passes its polymorphic argument in a qual. I
suppose it won't do anything in v10, or will it? Worth checking :-)> Why not use IsPolymorphicType?
Hmm, so IsPolymorphicType() test covers all of these pseudo-types except
RECORDOID. I rewrote the patch to use IsPolymorphicType.
I'm not able to think of a case where the partition constraint expression
would have to contain ANYNONARRAY though.
Also, I think it'd be good to have tests
for all these cases (even in v10), just to make sure we don't break it
going forward.
So, I had proposed this patch in last December, because partition pruning
using constraint exclusion was broken for these types and still is in PG
10. I have added the tests back in the patch for PG 10 to test that
partition pruning (using constraint exclusion) works for these cases. For
PG 11 and HEAD, we took care of that in e5dcbb88a15 (Rework code to
determine partition pruning procedure), so there does not appear to be any
need to add tests for pruning there.
At least the array case is clearly broken today ...
A test for the RECORDOID case would be particularly welcome, since it's
somehow different from the other cases. (I didn't understand why did
you remove the test in the latest version.)
I have added the tests in the patch for PG 10.
I have marked both patches as v5. One patch is for PG 10 and the other
applies unchanged to both HEAD and PG 11 branches.
Thanks,
Amit
Attachments:
PG10-v5-0001-Fix-how-get_partition_operator-looks-up-the-opera.patchtext/plain; charset=UTF-8; name=PG10-v5-0001-Fix-how-get_partition_operator-looks-up-the-opera.patchDownload
From 1ec0c064adc34c12ae5615f0f2bca27a9c61c30f Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Tue, 8 May 2018 14:31:37 +0900
Subject: [PATCH v4] Fix how get_partition_operator looks up the operator
Instead of looking up using the underlying partition key's type as the
operator's lefttype and righttype, use the partition operator class
declared input type, which works reliably even in the cases where
pseudo-types are involved. Also, make its decision whether a
RelableType is needed depend on a different criteria than what's
currently there. That is, only add a RelabelType if the partition
key's type is different from the selected operator's input types.
---
src/backend/catalog/partition.c | 47 ++++++--------
src/test/regress/expected/create_table.out | 2 +-
src/test/regress/expected/inherit.out | 98 ++++++++++++++++++++++++++++++
src/test/regress/sql/inherit.sql | 43 +++++++++++++
4 files changed, 161 insertions(+), 29 deletions(-)
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 17704f36b9..1f50b29a3f 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1176,40 +1176,31 @@ get_partition_operator(PartitionKey key, int col, StrategyNumber strategy,
Oid operoid;
/*
- * First check if there exists an operator of the given strategy, with
- * this column's type as both its lefttype and righttype, in the
- * partitioning operator family specified for the column.
+ * Get the operator in the partitioning operator family using the operator
+ * class declared input type as both its lefttype and righttype.
*/
operoid = get_opfamily_member(key->partopfamily[col],
- key->parttypid[col],
- key->parttypid[col],
+ key->partopcintype[col],
+ key->partopcintype[col],
strategy);
+ if (!OidIsValid(operoid))
+ elog(ERROR, "missing operator %d(%u,%u) in partition opfamily %u",
+ strategy, key->partopcintype[col], key->partopcintype[col],
+ key->partopfamily[col]);
/*
- * If one doesn't exist, we must resort to using an operator in the same
- * operator family but with the operator class declared input type. It is
- * OK to do so, because the column's type is known to be binary-coercible
- * with the operator class input type (otherwise, the operator class in
- * question would not have been accepted as the partitioning operator
- * class). We must however inform the caller to wrap the non-Const
- * expression with a RelabelType node to denote the implicit coercion. It
- * ensures that the resulting expression structurally matches similarly
- * processed expressions within the optimizer.
+ * If the partition key column is not of the same type as what the
+ * operator expects, we'll need to tell the caller to wrap the non-Const
+ * expression with a RelableType node in most cases, because that's what
+ * the parser would do for similar expressions (such as WHERE clauses
+ * involving this column and operator.) The intention is to make the
+ * expression that we generate here structurally match with expression
+ * generated elsewhere. We don't need the RelableType node for certain
+ * types though, because parse_coerce.c won't emit one either.
*/
- if (!OidIsValid(operoid))
- {
- operoid = get_opfamily_member(key->partopfamily[col],
- key->partopcintype[col],
- key->partopcintype[col],
- strategy);
- if (!OidIsValid(operoid))
- elog(ERROR, "missing operator %d(%u,%u) in opfamily %u",
- strategy, key->partopcintype[col], key->partopcintype[col],
- key->partopfamily[col]);
- *need_relabel = true;
- }
- else
- *need_relabel = false;
+ *need_relabel = (key->parttypid[col] != key->partopcintype[col] &&
+ key->partopcintype[col] != RECORDOID &&
+ !IsPolymorphicType(key->partopcintype[col]));
return operoid;
}
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index ab1d4ecaee..0026aa11dd 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -808,7 +808,7 @@ CREATE TABLE arrlp12 PARTITION OF arrlp FOR VALUES IN ('{1}', '{2}');
--------+-----------+-----------+----------+---------+----------+--------------+-------------
a | integer[] | | | | extended | |
Partition of: arrlp FOR VALUES IN ('{1}', '{2}')
-Partition constraint: ((a IS NOT NULL) AND (((a)::anyarray OPERATOR(pg_catalog.=) '{1}'::integer[]) OR ((a)::anyarray OPERATOR(pg_catalog.=) '{2}'::integer[])))
+Partition constraint: ((a IS NOT NULL) AND ((a = '{1}'::integer[]) OR (a = '{2}'::integer[])))
DROP TABLE arrlp;
-- partition on boolean column
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index 2efae8038f..26fdac0542 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -1973,3 +1973,101 @@ select min(a), max(a) from parted_minmax where b = '12345';
(1 row)
drop table parted_minmax;
+--
+-- check that pruning works properly when the partition key is of a
+-- pseudotype
+--
+-- array type list partition key
+create table pp_arrpart (a int[]) partition by list (a);
+create table pp_arrpart1 partition of pp_arrpart for values in ('{1}');
+create table pp_arrpart2 partition of pp_arrpart for values in ('{2, 3}', '{4, 5}');
+explain (costs off) select * from pp_arrpart where a = '{1}';
+ QUERY PLAN
+----------------------------------------
+ Append
+ -> Seq Scan on pp_arrpart1
+ Filter: (a = '{1}'::integer[])
+(3 rows)
+
+explain (costs off) select * from pp_arrpart where a = '{1, 2}';
+ QUERY PLAN
+--------------------------
+ Result
+ One-Time Filter: false
+(2 rows)
+
+explain (costs off) select * from pp_arrpart where a in ('{4, 5}', '{1}');
+ QUERY PLAN
+----------------------------------------------------------------------
+ Append
+ -> Seq Scan on pp_arrpart1
+ Filter: ((a = '{4,5}'::integer[]) OR (a = '{1}'::integer[]))
+ -> Seq Scan on pp_arrpart2
+ Filter: ((a = '{4,5}'::integer[]) OR (a = '{1}'::integer[]))
+(5 rows)
+
+drop table pp_arrpart;
+-- enum type list partition key
+create type pp_colors as enum ('green', 'blue', 'black');
+create table pp_enumpart (a pp_colors) partition by list (a);
+create table pp_enumpart_green partition of pp_enumpart for values in ('green');
+create table pp_enumpart_blue partition of pp_enumpart for values in ('blue');
+explain (costs off) select * from pp_enumpart where a = 'blue';
+ QUERY PLAN
+-----------------------------------------
+ Append
+ -> Seq Scan on pp_enumpart_blue
+ Filter: (a = 'blue'::pp_colors)
+(3 rows)
+
+explain (costs off) select * from pp_enumpart where a = 'black';
+ QUERY PLAN
+--------------------------
+ Result
+ One-Time Filter: false
+(2 rows)
+
+drop table pp_enumpart;
+drop type pp_colors;
+-- record type as partition key
+create type pp_rectype as (a int, b int);
+create table pp_recpart (a pp_rectype) partition by list (a);
+create table pp_recpart_11 partition of pp_recpart for values in ('(1,1)');
+create table pp_recpart_23 partition of pp_recpart for values in ('(2,3)');
+explain (costs off) select * from pp_recpart where a = '(1,1)'::pp_rectype;
+ QUERY PLAN
+-------------------------------------------
+ Append
+ -> Seq Scan on pp_recpart_11
+ Filter: (a = '(1,1)'::pp_rectype)
+(3 rows)
+
+explain (costs off) select * from pp_recpart where a = '(1,2)'::pp_rectype;
+ QUERY PLAN
+--------------------------
+ Result
+ One-Time Filter: false
+(2 rows)
+
+drop table pp_recpart;
+drop type pp_rectype;
+-- range type partition key
+create table pp_intrangepart (a int4range) partition by list (a);
+create table pp_intrangepart12 partition of pp_intrangepart for values in ('[1,2]');
+create table pp_intrangepart2inf partition of pp_intrangepart for values in ('[2,)');
+explain (costs off) select * from pp_intrangepart where a = '[1,2]'::int4range;
+ QUERY PLAN
+------------------------------------------
+ Append
+ -> Seq Scan on pp_intrangepart12
+ Filter: (a = '[1,3)'::int4range)
+(3 rows)
+
+explain (costs off) select * from pp_intrangepart where a = '(1,2)'::int4range;
+ QUERY PLAN
+--------------------------
+ Result
+ One-Time Filter: false
+(2 rows)
+
+drop table pp_intrangepart;
diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index 55770f5681..d157055d2b 100644
--- a/src/test/regress/sql/inherit.sql
+++ b/src/test/regress/sql/inherit.sql
@@ -683,3 +683,46 @@ insert into parted_minmax values (1,'12345');
explain (costs off) select min(a), max(a) from parted_minmax where b = '12345';
select min(a), max(a) from parted_minmax where b = '12345';
drop table parted_minmax;
+
+
+--
+-- check that pruning works properly when the partition key is of a
+-- pseudotype
+--
+
+-- array type list partition key
+create table pp_arrpart (a int[]) partition by list (a);
+create table pp_arrpart1 partition of pp_arrpart for values in ('{1}');
+create table pp_arrpart2 partition of pp_arrpart for values in ('{2, 3}', '{4, 5}');
+explain (costs off) select * from pp_arrpart where a = '{1}';
+explain (costs off) select * from pp_arrpart where a = '{1, 2}';
+explain (costs off) select * from pp_arrpart where a in ('{4, 5}', '{1}');
+drop table pp_arrpart;
+
+-- enum type list partition key
+create type pp_colors as enum ('green', 'blue', 'black');
+create table pp_enumpart (a pp_colors) partition by list (a);
+create table pp_enumpart_green partition of pp_enumpart for values in ('green');
+create table pp_enumpart_blue partition of pp_enumpart for values in ('blue');
+explain (costs off) select * from pp_enumpart where a = 'blue';
+explain (costs off) select * from pp_enumpart where a = 'black';
+drop table pp_enumpart;
+drop type pp_colors;
+
+-- record type as partition key
+create type pp_rectype as (a int, b int);
+create table pp_recpart (a pp_rectype) partition by list (a);
+create table pp_recpart_11 partition of pp_recpart for values in ('(1,1)');
+create table pp_recpart_23 partition of pp_recpart for values in ('(2,3)');
+explain (costs off) select * from pp_recpart where a = '(1,1)'::pp_rectype;
+explain (costs off) select * from pp_recpart where a = '(1,2)'::pp_rectype;
+drop table pp_recpart;
+drop type pp_rectype;
+
+-- range type partition key
+create table pp_intrangepart (a int4range) partition by list (a);
+create table pp_intrangepart12 partition of pp_intrangepart for values in ('[1,2]');
+create table pp_intrangepart2inf partition of pp_intrangepart for values in ('[2,)');
+explain (costs off) select * from pp_intrangepart where a = '[1,2]'::int4range;
+explain (costs off) select * from pp_intrangepart where a = '(1,2)'::int4range;
+drop table pp_intrangepart;
--
2.11.0
PG11-HEAD-v5-0001-Fix-how-get_partition_operator-looks-up-the-opera.patchtext/plain; charset=UTF-8; name=PG11-HEAD-v5-0001-Fix-how-get_partition_operator-looks-up-the-opera.patchDownload
From 800ac6bb7831c129b0843a9f3eb1b400bc2f5a3b Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Tue, 8 May 2018 14:57:47 +0900
Subject: [PATCH v5] Fix how get_partition_operator looks up the operator
Instead of looking up using the underlying partition key's type as the
operator's lefttype and righttype, use the partition operator class
declared input type, which works reliably even in the cases where
pseudo-types are involved. Also, make its decision whether a
RelableType is needed depend on a different criteria than what's
currently there. That is, only add a RelabelType if the partition
key's type is different from the selected operator's input types.
---
src/backend/partitioning/partbounds.c | 47 ++++++++++++------------------
src/test/regress/expected/create_table.out | 2 +-
2 files changed, 20 insertions(+), 29 deletions(-)
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index b19c76acc8..9a43515552 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -1154,40 +1154,31 @@ get_partition_operator(PartitionKey key, int col, StrategyNumber strategy,
Oid operoid;
/*
- * First check if there exists an operator of the given strategy, with
- * this column's type as both its lefttype and righttype, in the
- * partitioning operator family specified for the column.
+ * Get the operator in the partitioning operator family using the operator
+ * class declared input type as both its lefttype and righttype.
*/
operoid = get_opfamily_member(key->partopfamily[col],
- key->parttypid[col],
- key->parttypid[col],
+ key->partopcintype[col],
+ key->partopcintype[col],
strategy);
+ if (!OidIsValid(operoid))
+ elog(ERROR, "missing operator %d(%u,%u) in partition opfamily %u",
+ strategy, key->partopcintype[col], key->partopcintype[col],
+ key->partopfamily[col]);
/*
- * If one doesn't exist, we must resort to using an operator in the same
- * operator family but with the operator class declared input type. It is
- * OK to do so, because the column's type is known to be binary-coercible
- * with the operator class input type (otherwise, the operator class in
- * question would not have been accepted as the partitioning operator
- * class). We must however inform the caller to wrap the non-Const
- * expression with a RelabelType node to denote the implicit coercion. It
- * ensures that the resulting expression structurally matches similarly
- * processed expressions within the optimizer.
+ * If the partition key column is not of the same type as what the
+ * operator expects, we'll need to tell the caller to wrap the non-Const
+ * expression with a RelableType node in most cases, because that's what
+ * the parser would do for similar expressions (such as WHERE clauses
+ * involving this column and operator.) The intention is to make the
+ * expression that we generate here structurally match with expression
+ * generated elsewhere. We don't need the RelableType node for certain
+ * types though, because parse_coerce.c won't emit one either.
*/
- if (!OidIsValid(operoid))
- {
- operoid = get_opfamily_member(key->partopfamily[col],
- key->partopcintype[col],
- key->partopcintype[col],
- strategy);
- if (!OidIsValid(operoid))
- elog(ERROR, "missing operator %d(%u,%u) in opfamily %u",
- strategy, key->partopcintype[col], key->partopcintype[col],
- key->partopfamily[col]);
- *need_relabel = true;
- }
- else
- *need_relabel = false;
+ *need_relabel = (key->parttypid[col] != key->partopcintype[col] &&
+ key->partopcintype[col] != RECORDOID &&
+ !IsPolymorphicType(key->partopcintype[col]));
return operoid;
}
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 672719e5d5..8fdbca1345 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -882,7 +882,7 @@ CREATE TABLE arrlp12 PARTITION OF arrlp FOR VALUES IN ('{1}', '{2}');
--------+-----------+-----------+----------+---------+----------+--------------+-------------
a | integer[] | | | | extended | |
Partition of: arrlp FOR VALUES IN ('{1}', '{2}')
-Partition constraint: ((a IS NOT NULL) AND (((a)::anyarray OPERATOR(pg_catalog.=) '{1}'::integer[]) OR ((a)::anyarray OPERATOR(pg_catalog.=) '{2}'::integer[])))
+Partition constraint: ((a IS NOT NULL) AND ((a = '{1}'::integer[]) OR (a = '{2}'::integer[])))
DROP TABLE arrlp;
-- partition on boolean column
--
2.11.0
On 2018-Jul-09, Amit Langote wrote:
On 2018/07/07 9:19, Alvaro Herrera wrote:
On 2018-May-08, Amit Langote wrote:
I would like to revisit this as a bug fix for get_partition_operator() to
be applied to both PG 10 and HEAD. In the former case, it fixes the bug
that constraint exclusion code will fail to prune correctly when partition
key is of array, enum, range, or record type due to the structural
mismatch between the OpExpr that partitioning code generates and one that
the parser generates for WHERE clauses involving partition key columns.Interesting patchset. Didn't read your previous v2, v3 versions; I only
checked your latest, v1 (???).Sorry, I think I messed up version numbering there.
Well, I later realized that you had labelled the master version v4 and
the pg10 version v1, which made sense since you hadn't produced any
patch for pg10 before that ...
I'm wondering about the choice of OIDs in the new test. I wonder if
it's possible to get ANYNONARRAY (or others) by way of having a
polymorphic function that passes its polymorphic argument in a qual. I
suppose it won't do anything in v10, or will it? Worth checking :-)> Why not use IsPolymorphicType?Hmm, so IsPolymorphicType() test covers all of these pseudo-types except
RECORDOID. I rewrote the patch to use IsPolymorphicType.
I think that's good.
I'm not able to think of a case where the partition constraint expression
would have to contain ANYNONARRAY though.
I was about to give up trying to construct a case for this, when I
noticed this behavior (in pg10):
create or replace function f(anyelement) returns anynonarray immutable language plpgsql as $$
begin
return $1;
end;
$$;
create table pt (a int) partition by range (f(a));
create table pt1 partition of pt for values from (0) to (100);
create table pt2 partition of pt for values from (100) to (200);
and then pruning doesn't work:
alvherre=# explain select * from pt where a = 150;
QUERY PLAN
───────────────────────────────────────────────────────────
Append (cost=0.00..83.75 rows=26 width=4)
-> Seq Scan on pt1 (cost=0.00..41.88 rows=13 width=4)
Filter: (a = 150)
-> Seq Scan on pt2 (cost=0.00..41.88 rows=13 width=4)
Filter: (a = 150)
(5 filas)
The same occurs in 11 and master. I think this is because the
polymorphic type is resolved for the function ahead of time (at
table creation time); partexprs shows
({FUNCEXPR :funcid 35757 :funcresulttype 23 :funcretset false :funcvariadic false :funcformat 0 :funccollid 0 :inputcollid 0 :args ({VAR :varno 1 :varattno 1 :vartype 23 :vartypmod -1 :varcollid 0 :varlevelsup 0 :varnoold 1 :varoattno 1 :location 46}) :location 44})
where the ":funcresulttype 23" bit indicates that the function is
returning type integer, which I find a bit odd. I think if we were to
leave it as funcresulttype anynonarray, pruning would work. Not sure
yet where is that done.
Also, I think it'd be good to have tests
for all these cases (even in v10), just to make sure we don't break it
going forward.So, I had proposed this patch in last December, because partition pruning
using constraint exclusion was broken for these types and still is in PG
10. I have added the tests back in the patch for PG 10 to test that
partition pruning (using constraint exclusion) works for these cases. For
PG 11 and HEAD, we took care of that in e5dcbb88a15 (Rework code to
determine partition pruning procedure), so there does not appear to be any
need to add tests for pruning there.
Right.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
The same occurs in 11 and master. I think this is because the
polymorphic type is resolved for the function ahead of time (at
table creation time); partexprs shows
({FUNCEXPR :funcid 35757 :funcresulttype 23 :funcretset false :funcvariadic false :funcformat 0 :funccollid 0 :inputcollid 0 :args ({VAR :varno 1 :varattno 1 :vartype 23 :vartypmod -1 :varcollid 0 :varlevelsup 0 :varnoold 1 :varoattno 1 :location 46}) :location 44})
where the ":funcresulttype 23" bit indicates that the function is
returning type integer, which I find a bit odd. I think if we were to
leave it as funcresulttype anynonarray, pruning would work.
... at the cost of breaking many other things.
regards, tom lane
Another thing I realized when testing this is that partitioning over a
domain doesn't work very nicely (tested in 10 and master):
create domain overint as int;
create table pt (a overint) partition by range (a);
create table pt1 partition of pt for values from (0) to (100);
results in:
ERROR: specified value cannot be cast to type overint for column "a"
L�NEA 1: create table pt1 partition of pt for values from (0) to (100...
^
DETALLE: The cast requires a non-immutable conversion.
SUGERENCIA: Try putting the literal value in single quotes.
I tried to do what the HINT says, but it fails in the same way. I also
tried to add casts, but those are rejected as syntax errors.
Tracing it down, turns out that transformPartitionBoundValue gets from
coerce_to_target_type a CoerceToDomain node. It then tries to apply
expression_planner() to simplify the expression, but that one doesn't
want anything to do with a domain coercion (for apparently good reasons,
given other comments in that file). However, if we take out the
expression_planner() and replace it with a call to
strip_implicit_coercions(), not only it magically starts working, but
also the regression tests start failing with the attached diff, which
seems a Good Thing to me.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
money.patchtext/plain; charset=us-asciiDownload
*** /pgsql/source/REL_10_STABLE/src/test/regress/expected/create_table.out 2018-07-09 13:13:57.397491338 -0400
--- /home/alvherre/Code/pgsql/build/REL_10_STABLE/src/test/regress/results/create_table.out 2018-07-09 17:43:00.794556357 -0400
***************
*** 496,507 ****
a money
) PARTITION BY LIST (a);
CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
- ERROR: specified value cannot be cast to type money for column "a"
- LINE 1: ...EATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
- ^
- DETAIL: The cast requires a non-immutable conversion.
- HINT: Try putting the literal value in single quotes.
CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN ('10');
DROP TABLE moneyp;
-- immutable cast should work, though
CREATE TABLE bigintp (
--- 496,503 ----
a money
) PARTITION BY LIST (a);
CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN ('10');
+ ERROR: relation "moneyp_10" already exists
DROP TABLE moneyp;
-- immutable cast should work, though
CREATE TABLE bigintp (
======================================================================
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
Tracing it down, turns out that transformPartitionBoundValue gets from
coerce_to_target_type a CoerceToDomain node. It then tries to apply
expression_planner() to simplify the expression, but that one doesn't
want anything to do with a domain coercion (for apparently good reasons,
given other comments in that file).
Quite. Suppose you did
create domain overint as int;
create table pt (a overint) partition by range (a);
create table pt1 partition of pt for values from (0) to (100);
and the system took it, and then you did
alter domain overint add check (value > 100);
What happens now?
However, if we take out the
expression_planner() and replace it with a call to
strip_implicit_coercions(), not only it magically starts working, but
also the regression tests start failing with the attached diff, which
seems a Good Thing to me.
Why would you find that to be a good thing? The prohibition against
mutable coercions seems like something we need here, for more or less
the same reason in the domain example.
regards, tom lane
On 2018-Jul-09, Tom Lane wrote:
Suppose you did
create domain overint as int;
create table pt (a overint) partition by range (a);
create table pt1 partition of pt for values from (0) to (100);and the system took it, and then you did
alter domain overint add check (value > 100);
What happens now?
It scans the table to check whether any values violate that condition,
and raises an error if they do:
alvherre=# alter domain overint add check (value > 100);
ERROR: column "a" of table "ppt1" contains values that violate the new constraint
This looks sensible behavior to me.
Now if you don't have any values that violate the new constraint, then
the constraint can be created just fine, and you now have a partition
that can never accept any values. But that doesn't seem like a terrible
problem.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018-May-08, Amit Langote wrote:
In HEAD, since we already fixed that case in e5dcbb88a15d [1] which is a
different piece of code anyway, the patch only serves to improve the
deparse output emitted by ruleutils.c for partition constraint expressions
where pseudo-type partition key is involved. The change can be seen in
the updated test output for create_table test.
Actually, even in 11/master it also fixes this case:
alvherre=# explain update p set a = a || a where a = '{1}';
QUERY PLAN
──────────────────────────────────────────────────────────
Update on p (cost=0.00..54.03 rows=14 width=38)
Update on p1
Update on p2
-> Seq Scan on p1 (cost=0.00..27.02 rows=7 width=38)
Filter: (a = '{1}'::integer[])
-> Seq Scan on p2 (cost=0.00..27.02 rows=7 width=38)
Filter: (a = '{1}'::integer[])
(7 filas)
Because UPDATE uses the predtest.c prune code, not partprune. So it's
not just some ruleutils beautification.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
On 2018-Jul-09, Tom Lane wrote:
Suppose you did
create domain overint as int;
create table pt (a overint) partition by range (a);
create table pt1 partition of pt for values from (0) to (100);and the system took it, and then you did
alter domain overint add check (value > 100);
What happens now?
It scans the table to check whether any values violate that condition,
and raises an error if they do:
alvherre=# alter domain overint add check (value > 100);
ERROR: column "a" of table "ppt1" contains values that violate the new constraint
This looks sensible behavior to me.
And what about those partition bound values? They are now illegal
for the domain, so I would expect a dump/reload to fail, regardless
of whether there are any values in the table.
regards, tom lane
On 2018-Jul-10, Alvaro Herrera wrote:
alvherre=# explain update p set a = a || a where a = '{1}';
QUERY PLAN
──────────────────────────────────────────────────────────
Update on p (cost=0.00..54.03 rows=14 width=38)
Update on p1
Update on p2
-> Seq Scan on p1 (cost=0.00..27.02 rows=7 width=38)
Filter: (a = '{1}'::integer[])
-> Seq Scan on p2 (cost=0.00..27.02 rows=7 width=38)
Filter: (a = '{1}'::integer[])
(7 filas)Because UPDATE uses the predtest.c prune code, not partprune. So it's
not just some ruleutils beautification.
I added this test, modified some comments, and pushed.
Thanks for the patch.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018-Jul-10, Tom Lane wrote:
And what about those partition bound values? They are now illegal
for the domain, so I would expect a dump/reload to fail, regardless
of whether there are any values in the table.
Hmm, true.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018-Jul-09, Tom Lane wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
However, if we take out the
expression_planner() and replace it with a call to
strip_implicit_coercions(), not only it magically starts working, but
also the regression tests start failing with the attached diff, which
seems a Good Thing to me.Why would you find that to be a good thing? The prohibition against
mutable coercions seems like something we need here, for more or less
the same reason in the domain example.
By the way, while playing with a partition on type money and replacing
expression_planner() with strip_implicit_coercions(), the stored
partition bounds are completely broken -- they end up as literals of
type integer rather than money, so any insert at all into the partition
fails (even if the value is nominally the same). So clearly it's not a
change we want.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018/07/11 3:18, Alvaro Herrera wrote:
On 2018-May-08, Amit Langote wrote:
In HEAD, since we already fixed that case in e5dcbb88a15d [1] which is a
different piece of code anyway, the patch only serves to improve the
deparse output emitted by ruleutils.c for partition constraint expressions
where pseudo-type partition key is involved. The change can be seen in
the updated test output for create_table test.Actually, even in 11/master it also fixes this case:
alvherre=# explain update p set a = a || a where a = '{1}';
QUERY PLAN
──────────────────────────────────────────────────────────
Update on p (cost=0.00..54.03 rows=14 width=38)
Update on p1
Update on p2
-> Seq Scan on p1 (cost=0.00..27.02 rows=7 width=38)
Filter: (a = '{1}'::integer[])
-> Seq Scan on p2 (cost=0.00..27.02 rows=7 width=38)
Filter: (a = '{1}'::integer[])
(7 filas)Because UPDATE uses the predtest.c prune code, not partprune. So it's
not just some ruleutils beautification.
That's true. Shame I totally missed that.
Thanks,
Amit
On 2018/07/11 4:48, Alvaro Herrera wrote:
On 2018-Jul-10, Alvaro Herrera wrote:
alvherre=# explain update p set a = a || a where a = '{1}';
QUERY PLAN
──────────────────────────────────────────────────────────
Update on p (cost=0.00..54.03 rows=14 width=38)
Update on p1
Update on p2
-> Seq Scan on p1 (cost=0.00..27.02 rows=7 width=38)
Filter: (a = '{1}'::integer[])
-> Seq Scan on p2 (cost=0.00..27.02 rows=7 width=38)
Filter: (a = '{1}'::integer[])
(7 filas)Because UPDATE uses the predtest.c prune code, not partprune. So it's
not just some ruleutils beautification.I added this test, modified some comments, and pushed.
Thanks for the patch.
Thank you for committing.
Regards,
Amit
On 2018/07/11 4:50, Alvaro Herrera wrote:
On 2018-Jul-10, Tom Lane wrote:
And what about those partition bound values? They are now illegal
for the domain, so I would expect a dump/reload to fail, regardless
of whether there are any values in the table.Hmm, true.
There is a patch to overhaul how partition bound values are parsed:
https://commitfest.postgresql.org/18/1620/
With that patch, the error you reported upthread goes away (that is, one
can successfully create partitions), but the problem that Tom mentioned
above then appears.
What's the solution here then? Prevent domains as partition key?
Thanks,
Amit
On 2018-Jul-11, Amit Langote wrote:
What's the solution here then? Prevent domains as partition key?
Maybe if a domain is used in a partition key somewhere, prevent
constraints from being added?
Another thing worth considering: are you prevented from dropping a
domain that's used in a partition key? If not, you get an ugly message
when you later try to drop the table.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018/07/11 13:12, Alvaro Herrera wrote:
On 2018-Jul-11, Amit Langote wrote:
What's the solution here then? Prevent domains as partition key?
Maybe if a domain is used in a partition key somewhere, prevent
constraints from being added?
Maybe, but I guess you mean only prevent adding such a constraint
after-the-fact.
If a domain has a constraint before creating any partitions based on the
domain, then partition creation command would check that the partition
bound values satisfy those constraints.
Another thing worth considering: are you prevented from dropping a
domain that's used in a partition key? If not, you get an ugly message
when you later try to drop the table.
Yeah, I was about to write a message about that. I think we need to teach
RemoveAttributeById, which dependency.c calls when dropping the
referencing domain with cascade option, to abort if the attribute passed
to it belongs to the partition key of the input relation.
I tried that in the attached, but not sure about the order of messages
that appear in the output of DROP DOMAIN .. CASCADE. It contains a NOTICE
message followed by an ERROR message.
Thanks,
Amit
Attachments:
v1-0001-Prevent-RemoveAttributeById-from-dropping-partiti.patchtext/plain; charset=UTF-8; name=v1-0001-Prevent-RemoveAttributeById-from-dropping-partiti.patchDownload
From aca92efe08a45c7645720bf7c47ee5ca221f0a80 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Wed, 11 Jul 2018 10:26:55 +0900
Subject: [PATCH v1] Prevent RemoveAttributeById from dropping partition key
dependency.c is currently be able to drop whatever is referencing
the partition key column, because RemoveAttributeById lacks guards
checks to see if it's actually dropping a partition key.
---
src/backend/catalog/heap.c | 29 +++++++++++++++++++++++++++++
src/test/regress/expected/create_table.out | 10 ++++++++++
src/test/regress/sql/create_table.sql | 8 ++++++++
3 files changed, 47 insertions(+)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index d223ba8537..b69b9d9436 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1585,6 +1585,35 @@ RemoveAttributeById(Oid relid, AttrNumber attnum)
else
{
/* Dropping user attributes is lots harder */
+ bool is_expr;
+
+ /*
+ * Prevent dropping partition key attribute, making whatever that's
+ * trying to do this fail.
+ */
+ if (has_partition_attrs(rel,
+ bms_make_singleton(attnum - FirstLowInvalidHeapAttributeNumber),
+ &is_expr))
+ {
+ if (!is_expr)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+ errmsg("cannot drop column \"%s\" of table \"%s\"",
+ NameStr(attStruct->attname),
+ RelationGetRelationName(rel)),
+ errdetail("Column \"%s\" is named in the partition key of \"%s\"",
+ NameStr(attStruct->attname),
+ RelationGetRelationName(rel))));
+ else
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+ errmsg("cannot drop column \"%s\" of table \"%s\"",
+ NameStr(attStruct->attname),
+ RelationGetRelationName(rel)),
+ errdetail("Column \"%s\" is referenced in the partition key expression of \"%s\"",
+ NameStr(attStruct->attname),
+ RelationGetRelationName(rel))));
+ }
/* Mark the attribute as dropped */
attStruct->attisdropped = true;
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 8fdbca1345..ba32d781d1 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -909,3 +909,13 @@ ERROR: cannot create a temporary relation as partition of permanent relation "p
create temp table temp_part partition of temp_parted default; -- ok
drop table perm_parted cascade;
drop table temp_parted cascade;
+-- test domain partition key behavior
+create domain ct_overint as int;
+create table ct_domainpartkey (a ct_overint) partition by range (a);
+-- errors because partition key column cannot be dropped
+drop domain ct_overint cascade;
+NOTICE: drop cascades to column a of table ct_domainpartkey
+ERROR: cannot drop column "a" of table "ct_domainpartkey"
+DETAIL: Column "a" is named in the partition key of "ct_domainpartkey"
+drop table ct_domainpartkey;
+drop domain ct_overint;
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 78944950fe..5c6c37cdaa 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -735,3 +735,11 @@ create temp table temp_part partition of perm_parted default; -- error
create temp table temp_part partition of temp_parted default; -- ok
drop table perm_parted cascade;
drop table temp_parted cascade;
+
+-- test domain partition key behavior
+create domain ct_overint as int;
+create table ct_domainpartkey (a ct_overint) partition by range (a);
+-- errors because partition key column cannot be dropped
+drop domain ct_overint cascade;
+drop table ct_domainpartkey;
+drop domain ct_overint;
--
2.11.0
On 2018-Jul-11, Amit Langote wrote:
On 2018/07/11 13:12, Alvaro Herrera wrote:
On 2018-Jul-11, Amit Langote wrote:
What's the solution here then? Prevent domains as partition key?
Maybe if a domain is used in a partition key somewhere, prevent
constraints from being added?Maybe, but I guess you mean only prevent adding such a constraint
after-the-fact.
Yeah, any domain constraints added before won't be a problem. Another
angle on this problem is to verify partition bounds against the domain
constraint being added; if they all pass, there's no reason to reject
the constraint.
But I worry that Tom was using domain constraints as just an example of
a more general problem that we can get into.
If a domain has a constraint before creating any partitions based on the
domain, then partition creation command would check that the partition
bound values satisfy those constraints.
Of course.
Another thing worth considering: are you prevented from dropping a
domain that's used in a partition key? If not, you get an ugly message
when you later try to drop the table.Yeah, I was about to write a message about that. I think we need to teach
RemoveAttributeById, which dependency.c calls when dropping the
referencing domain with cascade option, to abort if the attribute passed
to it belongs to the partition key of the input relation.
Actually, here's another problem: why are we letting a column on a
domain become partition key, if you'll never be able to create a
partition? It seems that for pg11 the right reaction is to check
the partition key and reject this case.
I'm not sure of this implementation -- I couldn't find any case where we
reject the deletion on the function called from doDeletion (and we don't
have a preliminary phase on which to check for this, either). Maybe we
need a pg_depend entry to prevent this, though it seems a little silly.
Maybe we should add a preliminary verification phase in
deleteObjectsInList(), where we invoke object-type-specific checks.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018/07/12 2:33, Alvaro Herrera wrote:
On 2018-Jul-11, Amit Langote wrote:
On 2018/07/11 13:12, Alvaro Herrera wrote:
On 2018-Jul-11, Amit Langote wrote:
What's the solution here then? Prevent domains as partition key?
Maybe if a domain is used in a partition key somewhere, prevent
constraints from being added?Maybe, but I guess you mean only prevent adding such a constraint
after-the-fact.Yeah, any domain constraints added before won't be a problem. Another
angle on this problem is to verify partition bounds against the domain
constraint being added; if they all pass, there's no reason to reject
the constraint.
That's an idea. It's not clear to me how easy it is to get hold of all
the partition bounds that contain domain values. How do we get from the
domain on which a constraint is being added to the relpartbound which
would contain the partition bound value of the domain?
But I worry that Tom was using domain constraints as just an example of
a more general problem that we can get into.Another thing worth considering: are you prevented from dropping a
domain that's used in a partition key? If not, you get an ugly message
when you later try to drop the table.Yeah, I was about to write a message about that. I think we need to teach
RemoveAttributeById, which dependency.c calls when dropping the
referencing domain with cascade option, to abort if the attribute passed
to it belongs to the partition key of the input relation.Actually, here's another problem: why are we letting a column on a
domain become partition key, if you'll never be able to create a
partition? It seems that for pg11 the right reaction is to check
the partition key and reject this case.
Yeah, that seems much safer, and given how things stand today, no users
would complain if we do this.
I'm not sure of this implementation -- I couldn't find any case where we
reject the deletion on the function called from doDeletion (and we don't
have a preliminary phase on which to check for this, either). Maybe we
need a pg_depend entry to prevent this, though it seems a little silly.
Maybe we should add a preliminary verification phase in
deleteObjectsInList(), where we invoke object-type-specific checks.
Doing pre-check based fix had crossed my mind, but I didn't try hard to
pursue it. If we decide to just prevent domain partition keys, we don't
need to try too hard now to come up with a nice implementation for this,
right?
Thanks,
Amit
On 2018-Jul-12, Amit Langote wrote:
On 2018/07/12 2:33, Alvaro Herrera wrote:
Yeah, any domain constraints added before won't be a problem. Another
angle on this problem is to verify partition bounds against the domain
constraint being added; if they all pass, there's no reason to reject
the constraint.That's an idea. It's not clear to me how easy it is to get hold of all
the partition bounds that contain domain values. How do we get from the
domain on which a constraint is being added to the relpartbound which
would contain the partition bound value of the domain?
Well, we already get all table columns using a domain when the domain
gets a new constraint; I suppose it's just a matter of verifying for
each column whether it's part of the partition key, and then drill down
from there. (I'm not really familiar with that part of the catalog.)
Actually, here's another problem: why are we letting a column on a
domain become partition key, if you'll never be able to create a
partition? It seems that for pg11 the right reaction is to check
the partition key and reject this case.Yeah, that seems much safer, and given how things stand today, no users
would complain if we do this.
Agreed.
I'm not sure of this implementation -- I couldn't find any case where we
reject the deletion on the function called from doDeletion (and we don't
have a preliminary phase on which to check for this, either). Maybe we
need a pg_depend entry to prevent this, though it seems a little silly.
Maybe we should add a preliminary verification phase in
deleteObjectsInList(), where we invoke object-type-specific checks.Doing pre-check based fix had crossed my mind, but I didn't try hard to
pursue it. If we decide to just prevent domain partition keys, we don't
need to try too hard now to come up with a nice implementation for this,
right?
Absolutely.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018/07/26 1:41, Alvaro Herrera wrote:
On 2018-Jul-12, Amit Langote wrote:
On 2018/07/12 2:33, Alvaro Herrera wrote:
Yeah, any domain constraints added before won't be a problem. Another
angle on this problem is to verify partition bounds against the domain
constraint being added; if they all pass, there's no reason to reject
the constraint.That's an idea. It's not clear to me how easy it is to get hold of all
the partition bounds that contain domain values. How do we get from the
domain on which a constraint is being added to the relpartbound which
would contain the partition bound value of the domain?Well, we already get all table columns using a domain when the domain
gets a new constraint; I suppose it's just a matter of verifying for
each column whether it's part of the partition key, and then drill down
from there. (I'm not really familiar with that part of the catalog.)
Possibly doable, but maybe leave it for another day.
Actually, here's another problem: why are we letting a column on a
domain become partition key, if you'll never be able to create a
partition? It seems that for pg11 the right reaction is to check
the partition key and reject this case.Yeah, that seems much safer, and given how things stand today, no users
would complain if we do this.Agreed.
I'm not sure of this implementation -- I couldn't find any case where we
reject the deletion on the function called from doDeletion (and we don't
have a preliminary phase on which to check for this, either). Maybe we
need a pg_depend entry to prevent this, though it seems a little silly.
Maybe we should add a preliminary verification phase in
deleteObjectsInList(), where we invoke object-type-specific checks.Doing pre-check based fix had crossed my mind, but I didn't try hard to
pursue it. If we decide to just prevent domain partition keys, we don't
need to try too hard now to come up with a nice implementation for this,
right?Absolutely.
OK, attached is a patch for that.
Thanks,
Amit
Attachments:
v1-0001-Disallow-domain-type-partition-key.patchtext/plain; charset=UTF-8; name=v1-0001-Disallow-domain-type-partition-key.patchDownload
From 042aa582f717ceb695a1ab60517c2e9f7d04704b Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Thu, 26 Jul 2018 11:07:51 +0900
Subject: [PATCH v1] Disallow domain type partition key
---
src/backend/commands/tablecmds.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index eb2d33dd86..871c831cd2 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13693,6 +13693,12 @@ ComputePartitionAttrs(Relation rel, List *partParams, AttrNumber *partattrs,
atttype = attform->atttypid;
attcollation = attform->attcollation;
ReleaseSysCache(atttuple);
+
+ /* Prevent using domains as a partition key. */
+ if (get_typtype(atttype) == TYPTYPE_DOMAIN)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot use domain type column as partition key")));
}
else
{
@@ -13703,6 +13709,12 @@ ComputePartitionAttrs(Relation rel, List *partParams, AttrNumber *partattrs,
atttype = exprType(expr);
attcollation = exprCollation(expr);
+ /* Prevent using domains as a partition key. */
+ if (get_typtype(atttype) == TYPTYPE_DOMAIN)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot use domain type expression as partition key")));
+
/*
* Strip any top-level COLLATE clause. This ensures that we treat
* "x COLLATE y" and "(x COLLATE y)" alike.
--
2.11.0