Thanks for the review.
On 2018/04/18 7:11, Alvaro Herrera wrote:
> Amit Langote wrote:
>
>> Ah, I think I got it after staring at the (btree) index code for a bit.
>>
>> What pruning code got wrong is that it's comparing the expression type
>> (type of the constant arg that will be compared with partition bound
>> datums when pruning) with the partopcintype to determine if we should look
>> up the cross-type comparison/hashing procedure, whereas what the latter
>> should be compare with is the clause operator's oprighttype. ISTM, if
>> op_in_opfamily() passed for the operator, that's the correct thing to do.
>
> I wonder why you left out the hash partitioning case? I don't really
> know that this is correct, but here's a delta patch as demonstration.
@@ -1546,8 +1546,8 @@ match_clause_to_partition_key(RelOptInfo *rel,
case PARTITION_STRATEGY_HASH:
cmpfn = get_opfamily_proc(part_scheme->partopfamily[partkeyidx],
- op_righttype, op_righttype,
- HASHEXTENDED_PROC);
+ part_scheme->partopcintype[partkeyidx],
+ op_righttype, HASHEXTENDED_PROC);
This change is not quite right, because it disables pruning. The above
returns InvalidOid as there are no hash AM procedures (in pg_amproc) whose
lefttype and righttype don't match.
select count(*)
from pg_amproc
where amprocfamily in
(select opf.oid
from pg_opfamily opf join pg_am am on (opf.opfmethod = am.oid)
where amname = 'hash')
and amproclefttype <> amprocrighttype;
count
-------
0
(1 row)
So, the original coding passes op_righttype for both lefttype and righttype.
Consider the following example:
create table h(a int) partition by hash (a);
create table h1 partition of foo for values with (modulus 2, remainder 0);
create table h2 partition of foo for values with (modulus 2, remainder 1);
insert into h values (1::bigint);
select tableoid::regclass, * from h;
tableoid | a
----------+---
h1 | 1
(1 row)
-- without the delta patch
explain select * from h where a = 1::bigint;
QUERY PLAN
------------------------------------------------------------
Append (cost=0.00..41.94 rows=13 width=4)
-> Seq Scan on h1 (cost=0.00..41.88 rows=13 width=4)
Filter: (a = '1'::bigint)
(3 rows)
-- with
explain select * from h where a = 1::bigint;
QUERY PLAN
------------------------------------------------------------
Append (cost=0.00..83.88 rows=26 width=4)
-> Seq Scan on h1 (cost=0.00..41.88 rows=13 width=4)
Filter: (a = '1'::bigint)
-> Seq Scan on h2 (cost=0.00..41.88 rows=13 width=4)
Filter: (a = '1'::bigint)
(5 rows)
> (v3 is your patch, I think the only change is I renamed the tables used
> in the test)
Thanks, that seems like a good idea.
Here's v4 with parts of your delta patch merged. I've also updated
comments in match_clause_to_partition_key() to describe the rationale of
support function look up in a bit more detail.
Regards,
Amit
>From d6652eb932a5e1cf6b63fa1dd09e4dd752267dbc Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Wed, 18 Apr 2018 10:40:14 +0900
Subject: [PATCH v4] Fix pruning code to determine comparison function
correctly
It's unreliable to determine one using the constant expression's
type directly (for example, it doesn't work correctly when the
expression contains an array, enum, etc.). Instead, use righttype
of the operator, the one that supposedly passed the op_in_opfamily
test using the partitioning opfamily.
---
src/backend/partitioning/partprune.c | 45 +++++++--
src/test/regress/expected/partition_prune.out | 138 ++++++++++++++++++++++++++
src/test/regress/sql/partition_prune.sql | 53 ++++++++++
3 files changed, 227 insertions(+), 9 deletions(-)
diff --git a/src/backend/partitioning/partprune.c
b/src/backend/partitioning/partprune.c
index 7666c6c412..6ab1066dea 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -1426,10 +1426,12 @@ match_clause_to_partition_key(RelOptInfo *rel,
OpExpr *opclause = (OpExpr *) clause;
Expr *leftop,
*rightop;
- Oid commutator = InvalidOid,
+ Oid op_lefttype,
+ op_righttype,
+ commutator = InvalidOid,
negator = InvalidOid;
Oid cmpfn;
- Oid exprtype;
+ int op_strategy;
bool is_opne_listp = false;
PartClauseInfo *partclause;
@@ -1517,24 +1519,51 @@ match_clause_to_partition_key(RelOptInfo *rel,
return PARTCLAUSE_UNSUPPORTED;
}
- /* Check if we're going to need a cross-type comparison
function. */
- exprtype = exprType((Node *) expr);
- if (exprtype != part_scheme->partopcintype[partkeyidx])
+ /*
+ * Check if we can perform pruning if the clause's other
argument is
+ * not of the same type as the partitioning opclass's declared
input
+ * type. If it *is* of the same type, we can simply use the
cached
+ * procedure, that is, the one in PartitionKey. If not, we
must find
+ * the correct procedure in pg_amproc, which if it doesn't
exist, we
+ * have to give up on pruning using this clause.
+ */
+ get_op_opfamily_properties(OidIsValid(negator) ?
+ negator :
opclause->opno,
+
partopfamily, false,
+
&op_strategy, &op_lefttype, &op_righttype);
+
+ if (op_righttype == part_scheme->partopcintype[partkeyidx])
+ cmpfn = part_scheme->partsupfunc[partkeyidx].fn_oid;
+ else
{
switch (part_scheme->strategy)
{
+ /*
+ * RANGE and LIST partitioning use a btree
operator family,
+ * wherein, we can expect to find a procedure
in pg_amproc
+ * whose righttype matches the clause
operator's righttype.
+ */
case PARTITION_STRATEGY_LIST:
case PARTITION_STRATEGY_RANGE:
cmpfn =
get_opfamily_proc(part_scheme->partopfamily[partkeyidx],
part_scheme->partopcintype[partkeyidx],
-
exprtype, BTORDER_PROC);
+
op_righttype, BTORDER_PROC);
break;
+ /*
+ * HASH partitioning uses a hash operator
family, wherein the
+ * support function only ever processes *one*
input value that
+ * is to be hashed. So, it doesn't make sense
to look up
+ * using two distinct type OIDs. Instead, pass
the desired
+ * type's OID as both lefttype and righttype
when searching in
+ * pg_amproc.
+ */
case PARTITION_STRATEGY_HASH:
cmpfn =
get_opfamily_proc(part_scheme->partopfamily[partkeyidx],
-
exprtype, exprtype, HASHEXTENDED_PROC);
+
op_righttype, op_righttype,
+
HASHEXTENDED_PROC);
break;
default:
@@ -1547,8 +1576,6 @@ match_clause_to_partition_key(RelOptInfo *rel,
if (!OidIsValid(cmpfn))
return PARTCLAUSE_UNSUPPORTED;
}
- else
- cmpfn = part_scheme->partsupfunc[partkeyidx].fn_oid;
partclause = (PartClauseInfo *) palloc(sizeof(PartClauseInfo));
partclause->keyno = partkeyidx;
diff --git a/src/test/regress/expected/partition_prune.out
b/src/test/regress/expected/partition_prune.out
index 844cfb3e42..9c65ee001d 100644
--- a/src/test/regress/expected/partition_prune.out
+++ b/src/test/regress/expected/partition_prune.out
@@ -2599,3 +2599,141 @@ select * from boolp where a = (select value from
boolvalues where not value);
drop table boolp;
reset enable_indexonlyscan;
+--
+-- 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;
+-- array type hash partition key
+create table pph_arrpart (a int[]) partition by hash (a);
+create table pph_arrpart1 partition of pph_arrpart for values with (modulus 2,
remainder 0);
+create table pph_arrpart2 partition of pph_arrpart for values with (modulus 2,
remainder 1);
+insert into pph_arrpart values ('{1}'), ('{1, 2}'), ('{4, 5}');
+select tableoid::regclass, * from pph_arrpart order by 1;
+ tableoid | a
+--------------+-------
+ pph_arrpart1 | {1,2}
+ pph_arrpart1 | {4,5}
+ pph_arrpart2 | {1}
+(3 rows)
+
+explain (costs off) select * from pph_arrpart where a = '{1}';
+ QUERY PLAN
+----------------------------------------
+ Append
+ -> Seq Scan on pph_arrpart2
+ Filter: (a = '{1}'::integer[])
+(3 rows)
+
+explain (costs off) select * from pph_arrpart where a = '{1, 2}';
+ QUERY PLAN
+------------------------------------------
+ Append
+ -> Seq Scan on pph_arrpart1
+ Filter: (a = '{1,2}'::integer[])
+(3 rows)
+
+explain (costs off) select * from pph_arrpart where a in ('{4, 5}', '{1}');
+ QUERY PLAN
+----------------------------------------------------------------------
+ Append
+ -> Seq Scan on pph_arrpart1
+ Filter: ((a = '{4,5}'::integer[]) OR (a = '{1}'::integer[]))
+ -> Seq Scan on pph_arrpart2
+ Filter: ((a = '{4,5}'::integer[]) OR (a = '{1}'::integer[]))
+(5 rows)
+
+drop table pph_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/partition_prune.sql
b/src/test/regress/sql/partition_prune.sql
index e808d1a439..b38b39c71e 100644
--- a/src/test/regress/sql/partition_prune.sql
+++ b/src/test/regress/sql/partition_prune.sql
@@ -645,3 +645,56 @@ select * from boolp where a = (select value from
boolvalues where not value);
drop table boolp;
reset enable_indexonlyscan;
+
+--
+-- 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;
+
+-- array type hash partition key
+create table pph_arrpart (a int[]) partition by hash (a);
+create table pph_arrpart1 partition of pph_arrpart for values with (modulus 2,
remainder 0);
+create table pph_arrpart2 partition of pph_arrpart for values with (modulus 2,
remainder 1);
+insert into pph_arrpart values ('{1}'), ('{1, 2}'), ('{4, 5}');
+select tableoid::regclass, * from pph_arrpart order by 1;
+explain (costs off) select * from pph_arrpart where a = '{1}';
+explain (costs off) select * from pph_arrpart where a = '{1, 2}';
+explain (costs off) select * from pph_arrpart where a in ('{4, 5}', '{1}');
+drop table pph_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
