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
From 1ec0c064adc34c12ae5615f0f2bca27a9c61c30f Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@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
From 800ac6bb7831c129b0843a9f3eb1b400bc2f5a3b Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@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
