Fujita-san,
Thanks for the review.
On 2018/01/23 20:13, Etsuro Fujita wrote:
> (2017/12/13 16:38), Amit Langote wrote:
>> Attached patch is an attempt at that. With the patch, instead of
>> internally generating an ANY/ALL expression, generate an OR expression
>> instead. So:
>>
>> \d+ p1
>> ...
>> Partition of: p FOR VALUES IN ('{1}')
>> Partition constraint: ((a IS NOT NULL) AND ((a)::anyarray
>> OPERATOR(pg_catalog.=) '{1}'::integer[]))
>>
>> \d+ p2
>> ...
>> Partition of: p FOR VALUES IN ('{2,3}', '{4,5}')
>> Partition constraint: ((a IS NOT NULL) AND (((a)::anyarray
>> OPERATOR(pg_catalog.=) '{2,3}'::integer[]) OR ((a)::anyarray
>> OPERATOR(pg_catalog.=) '{4,5}'::integer[])))
>>
>> The expressions above get through the parser just fine:
>>
>> select tableoid::regclass, (a)::anyarray OPERATOR(pg_catalog.=)
>> '{1}'::integer[] from p;
>> tableoid | ?column?
>> |---------+----------
>> p1 | t
>> p2 | f
>> p2 | f
>> (3 rows)
>>
>> alter table p1 add constraint check_a check ((a)::anyarray
>> OPERATOR(pg_catalog.=) '{1}'::integer[]);
>> ALTER TABLE
>>
>> \d+ p1
>> ...
>> Check constraints:
>> "check_a" CHECK (a = '{1}'::integer[])
>
> Thanks for the patch! Here are review comments that I have for now:
>
> * I think it's a good idea to generate an OR expression tree for the case
> where the type of the partitioning key is an array, but I'm not sure we
> should handle other cases the same way because partition constraints
> represented by OR-expression trees would not be efficiently processed by
> the executor, compared to ScalarArrayOpExpr, when the number of elements
> that are ORed together is large. So what about generating the OR
> expression tree only if the partitioning-key's type is an array, instead?
> That would be more consistent with the handling of IN-list check
> constraints in eg, CREATE/ALTER TABLE, which I think is a good thing.
Agreed, done that way.
So now, make_partition_op_expr() will generate an OR tree if the key is of
an array type, a ScalarArrayOpExpr if the IN-list contains more than one
value, and an OpExpr if the list contains just one value.
> * I think it'd be better to add a test case where multiple elements are
> ORed together as a partition constraint.
OK, added a test in create_table.sql.
Updated patch attached.
Thanks,
Amit
From a83ebfe05d20e6086a9efacba0bcfbbdb7a2e570 Mon Sep 17 00:00:00 2001
From: amit <[email protected]>
Date: Fri, 8 Dec 2017 19:00:46 +0900
Subject: [PATCH v2] Emit list partition constraint as OR expression in some
cases
... instead of key op ANY (<array>) as is done now. If key is
itself of an array type, the expression in its current form
is not accepted by the backend's parser.
---
src/backend/catalog/partition.c | 96 +++++++++++++++++++--------
src/test/regress/expected/create_table.out | 18 ++++-
src/test/regress/expected/foreign_data.out | 6 +-
src/test/regress/expected/partition_prune.out | 8 +--
src/test/regress/sql/create_table.sql | 6 ++
5 files changed, 93 insertions(+), 41 deletions(-)
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 8adc4ee977..58527357a9 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1625,18 +1625,68 @@ make_partition_op_expr(PartitionKey key, int keynum,
{
case PARTITION_STRATEGY_LIST:
{
- ScalarArrayOpExpr *saopexpr;
-
- /* Build leftop = ANY (rightop) */
- saopexpr = makeNode(ScalarArrayOpExpr);
- saopexpr->opno = operoid;
- saopexpr->opfuncid = get_opcode(operoid);
- saopexpr->useOr = true;
- saopexpr->inputcollid =
key->partcollation[keynum];
- saopexpr->args = list_make2(arg1, arg2);
- saopexpr->location = -1;
-
- result = (Expr *) saopexpr;
+ ListCell *lc;
+ List *elems = (List *) arg2,
+ *elemops = NIL;
+
+ if (type_is_array(key->parttypid[keynum]))
+ {
+ foreach (lc, elems)
+ {
+ Expr *elem = lfirst(lc),
+ *elemop;
+
+ elemop = make_opclause(operoid,
+
BOOLOID,
+
false,
+
arg1, elem,
+
InvalidOid,
+
key->partcollation[keynum]);
+ elemops = lappend(elemops,
elemop);
+ }
+
+ result = list_length(elemops) > 1
+ ?
makeBoolExpr(OR_EXPR, elemops, -1)
+ :
linitial(elemops);
+ }
+ else if (list_length(elems) > 1)
+ {
+ ArrayExpr *arrexpr;
+ ScalarArrayOpExpr *saopexpr;
+
+ /*
+ * Construct an ArrayExpr for the
non-null partition
+ * values
+ */
+ arrexpr = makeNode(ArrayExpr);
+ arrexpr->array_typeid =
+
!type_is_array(key->parttypid[0])
+
? get_array_type(key->parttypid[0])
+
: key->parttypid[0];
+ arrexpr->array_collid =
key->parttypcoll[0];
+ arrexpr->element_typeid =
key->parttypid[0];
+ arrexpr->elements = elems;
+ arrexpr->multidims = false;
+ arrexpr->location = -1;
+
+ /* Build leftop = ANY (rightop) */
+ saopexpr = makeNode(ScalarArrayOpExpr);
+ saopexpr->opno = operoid;
+ saopexpr->opfuncid =
get_opcode(operoid);
+ saopexpr->useOr = true;
+ saopexpr->inputcollid =
key->partcollation[keynum];
+ saopexpr->args = list_make2(arg1,
arrexpr);
+ saopexpr->location = -1;
+
+ result = (Expr *) saopexpr;
+ }
+ else
+ result = make_opclause(operoid,
+
BOOLOID,
+
false,
+
arg1, linitial(elems),
+
InvalidOid,
+
key->partcollation[keynum]);
break;
}
@@ -1758,11 +1808,10 @@ get_qual_for_list(Relation parent, PartitionBoundSpec
*spec)
PartitionKey key = RelationGetPartitionKey(parent);
List *result;
Expr *keyCol;
- ArrayExpr *arr;
Expr *opexpr;
NullTest *nulltest;
ListCell *cell;
- List *arrelems = NIL;
+ List *elems = NIL;
bool list_has_null = false;
/*
@@ -1828,7 +1877,7 @@ get_qual_for_list(Relation parent, PartitionBoundSpec
*spec)
false, /* isnull */
key->parttypbyval[0]);
- arrelems = lappend(arrelems, val);
+ elems = lappend(elems, val);
}
}
else
@@ -1843,26 +1892,15 @@ get_qual_for_list(Relation parent, PartitionBoundSpec
*spec)
if (val->constisnull)
list_has_null = true;
else
- arrelems = lappend(arrelems, copyObject(val));
+ elems = lappend(elems, copyObject(val));
}
}
- if (arrelems)
+ if (elems)
{
- /* Construct an ArrayExpr for the non-null partition values */
- arr = makeNode(ArrayExpr);
- arr->array_typeid = !type_is_array(key->parttypid[0])
- ? get_array_type(key->parttypid[0])
- : key->parttypid[0];
- arr->array_collid = key->parttypcoll[0];
- arr->element_typeid = key->parttypid[0];
- arr->elements = arrelems;
- arr->multidims = false;
- arr->location = -1;
-
/* Generate the main expression, i.e., keyCol = ANY (arr) */
opexpr = make_partition_op_expr(key, 0, BTEqualStrategyNumber,
-
keyCol, (Expr *) arr);
+
keyCol, (Expr *) elems);
}
else
{
diff --git a/src/test/regress/expected/create_table.out
b/src/test/regress/expected/create_table.out
index 8e745402ae..14bcf7eaf3 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -737,7 +737,7 @@ CREATE TABLE part_c_1_10 PARTITION OF part_c FOR VALUES
FROM (1) TO (10);
a | text | | | | extended | |
b | integer | | not null | 1 | plain | |
Partition of: parted FOR VALUES IN ('b')
-Partition constraint: ((a IS NOT NULL) AND (a = ANY (ARRAY['b'::text])))
+Partition constraint: ((a IS NOT NULL) AND (a = 'b'::text))
Check constraints:
"check_a" CHECK (length(a) > 0)
"part_b_b_check" CHECK (b >= 0)
@@ -750,7 +750,7 @@ Check constraints:
a | text | | | | extended | |
b | integer | | not null | 0 | plain | |
Partition of: parted FOR VALUES IN ('c')
-Partition constraint: ((a IS NOT NULL) AND (a = ANY (ARRAY['c'::text])))
+Partition constraint: ((a IS NOT NULL) AND (a = 'c'::text))
Partition key: RANGE (b)
Check constraints:
"check_a" CHECK (length(a) > 0)
@@ -764,7 +764,7 @@ Partitions: part_c_1_10 FOR VALUES FROM (1) TO (10)
a | text | | | | extended | |
b | integer | | not null | 0 | plain | |
Partition of: part_c FOR VALUES FROM (1) TO (10)
-Partition constraint: ((a IS NOT NULL) AND (a = ANY (ARRAY['c'::text])) AND (b
IS NOT NULL) AND (b >= 1) AND (b < 10))
+Partition constraint: ((a IS NOT NULL) AND (a = 'c'::text) AND (b IS NOT NULL)
AND (b >= 1) AND (b < 10))
Check constraints:
"check_a" CHECK (length(a) > 0)
@@ -863,3 +863,15 @@ Partition key: LIST (a)
Number of partitions: 0
DROP TABLE parted_col_comment;
+-- list partitioning on array type column
+CREATE TABLE arrlp (a int[]) PARTITION BY LIST (a);
+CREATE TABLE arrlp12 PARTITION OF arrlp FOR VALUES IN ('{1}', '{2}');
+\d+ arrlp12
+ Table "public.arrlp12"
+ Column | Type | Collation | Nullable | Default | Storage | Stats target
| Description
+--------+-----------+-----------+----------+---------+----------+--------------+-------------
+ 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[])))
+
+DROP TABLE arrlp;
diff --git a/src/test/regress/expected/foreign_data.out
b/src/test/regress/expected/foreign_data.out
index d2c184f2cf..6a1b278e5a 100644
--- a/src/test/regress/expected/foreign_data.out
+++ b/src/test/regress/expected/foreign_data.out
@@ -1863,7 +1863,7 @@ Partitions: pt2_1 FOR VALUES IN (1)
c2 | text | | | | | extended |
|
c3 | date | | | | | plain |
|
Partition of: pt2 FOR VALUES IN (1)
-Partition constraint: ((c1 IS NOT NULL) AND (c1 = ANY (ARRAY[1])))
+Partition constraint: ((c1 IS NOT NULL) AND (c1 = 1))
Server: s0
FDW options: (delimiter ',', quote '"', "be quoted" 'value')
@@ -1935,7 +1935,7 @@ Partitions: pt2_1 FOR VALUES IN (1)
c2 | text | | | | | extended |
|
c3 | date | | | | | plain |
|
Partition of: pt2 FOR VALUES IN (1)
-Partition constraint: ((c1 IS NOT NULL) AND (c1 = ANY (ARRAY[1])))
+Partition constraint: ((c1 IS NOT NULL) AND (c1 = 1))
Server: s0
FDW options: (delimiter ',', quote '"', "be quoted" 'value')
@@ -1963,7 +1963,7 @@ Partitions: pt2_1 FOR VALUES IN (1)
c2 | text | | | | | extended |
|
c3 | date | | not null | | | plain |
|
Partition of: pt2 FOR VALUES IN (1)
-Partition constraint: ((c1 IS NOT NULL) AND (c1 = ANY (ARRAY[1])))
+Partition constraint: ((c1 IS NOT NULL) AND (c1 = 1))
Check constraints:
"p21chk" CHECK (c2 <> ''::text)
Server: s0
diff --git a/src/test/regress/expected/partition_prune.out
b/src/test/regress/expected/partition_prune.out
index aabb0240a9..348719bd62 100644
--- a/src/test/regress/expected/partition_prune.out
+++ b/src/test/regress/expected/partition_prune.out
@@ -1014,23 +1014,19 @@ explain (costs off) select * from boolpart where a =
false;
Append
-> Seq Scan on boolpart_f
Filter: (NOT a)
- -> Seq Scan on boolpart_t
- Filter: (NOT a)
-> Seq Scan on boolpart_default
Filter: (NOT a)
-(7 rows)
+(5 rows)
explain (costs off) select * from boolpart where not a = false;
QUERY PLAN
------------------------------------
Append
- -> Seq Scan on boolpart_f
- Filter: a
-> Seq Scan on boolpart_t
Filter: a
-> Seq Scan on boolpart_default
Filter: a
-(7 rows)
+(5 rows)
explain (costs off) select * from boolpart where a is true or a is not true;
QUERY PLAN
diff --git a/src/test/regress/sql/create_table.sql
b/src/test/regress/sql/create_table.sql
index 8f9991ef18..f04d6c6114 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -707,3 +707,9 @@ COMMENT ON COLUMN parted_col_comment.a IS 'Partition key';
SELECT obj_description('parted_col_comment'::regclass);
\d+ parted_col_comment
DROP TABLE parted_col_comment;
+
+-- list partitioning on array type column
+CREATE TABLE arrlp (a int[]) PARTITION BY LIST (a);
+CREATE TABLE arrlp12 PARTITION OF arrlp FOR VALUES IN ('{1}', '{2}');
+\d+ arrlp12
+DROP TABLE arrlp;
--
2.11.0
