Hi.
On 2018/03/04 22:12, Emre Hasegeli wrote:
>> Yeah, the patch in its current form is wrong, because it will give wrong
>> answers if the operator being used in a SAOP is non-strict. I modified
>> the patch to consider operator strictness before doing anything with nulls.
>
> I tried to review this patch without any familiarity to the code.
Thanks for the review.
> arrayconst_next_fn():
>
>> + /* skip nulls if ok to do so */
>> + if (state->opisstrict)
>> + {
>> + while (state->elem_nulls[state->next_elem])
>> + state->next_elem++;
>> + }
>
> Shouldn't we check if we consumed all elements (state->next_elem >=
> state->num_elems) inside the while loop?
You're right. Fixed.
> arrayexpr_next_fn():
>
>> + /* skip nulls if ok to do so */
>> + if (state->opisstrict)
>> + {
>> + Node *node = (Node *) lfirst(state->next);
>> +
>> + while (IsA(node, Const) && ((Const *) node)->constisnull)
>> + state->next = lnext(state->next);
>> + }
>
> I cannot find a way to test this change. Can you imagine a query to
> exercise it on the regression tests?
So far, I hadn't either. I figured one out and added it to inherit.sql.
Basically, I needed to write the query such that an IN () expression
doesn't get const-simplified to a Const containing array, but rather
remains an ArrayExpr. So, arrayexpr_*() functions in predtest.c are now
exercised.
Attached updated patch.
Thanks,
Amit
From d0f26929eccba2e5671e18db0466ea73af424430 Mon Sep 17 00:00:00 2001
From: amit <[email protected]>
Date: Thu, 1 Feb 2018 11:32:23 +0900
Subject: [PATCH v3] Disregard nulls in SAOP rightarg array/list during CE
---
src/backend/optimizer/util/predtest.c | 33 +++++++++++++++++++++++++++++++++
src/test/regress/expected/inherit.out | 34 +++++++++++++++++++++++++++++-----
src/test/regress/sql/inherit.sql | 1 +
3 files changed, 63 insertions(+), 5 deletions(-)
diff --git a/src/backend/optimizer/util/predtest.c
b/src/backend/optimizer/util/predtest.c
index 8106010329..a20a3070bf 100644
--- a/src/backend/optimizer/util/predtest.c
+++ b/src/backend/optimizer/util/predtest.c
@@ -905,6 +905,7 @@ boolexpr_startup_fn(Node *clause, PredIterInfo info)
typedef struct
{
OpExpr opexpr;
+ bool opisstrict; /* Is the operator strict? */
Const constexpr;
int next_elem;
int num_elems;
@@ -957,6 +958,12 @@ arrayconst_startup_fn(Node *clause, PredIterInfo info)
state->constexpr.constbyval = elmbyval;
lsecond(state->opexpr.args) = &state->constexpr;
+ /*
+ * Record if the operator is strict to perform appropriate action on
+ * encountering null values in the element array.
+ */
+ state->opisstrict = op_strict(saop->opno);
+
/* Initialize iteration state */
state->next_elem = 0;
}
@@ -968,6 +975,13 @@ arrayconst_next_fn(PredIterInfo info)
if (state->next_elem >= state->num_elems)
return NULL;
+ /* skip nulls if ok to do so */
+ if (state->opisstrict)
+ {
+ while (state->next_elem < state->num_elems &&
+ state->elem_nulls[state->next_elem])
+ state->next_elem++;
+ }
state->constexpr.constvalue = state->elem_values[state->next_elem];
state->constexpr.constisnull = state->elem_nulls[state->next_elem];
state->next_elem++;
@@ -992,6 +1006,7 @@ arrayconst_cleanup_fn(PredIterInfo info)
typedef struct
{
OpExpr opexpr;
+ bool opisstrict; /* Is the operator strict? */
ListCell *next;
} ArrayExprIterState;
@@ -1016,6 +1031,12 @@ arrayexpr_startup_fn(Node *clause, PredIterInfo info)
state->opexpr.inputcollid = saop->inputcollid;
state->opexpr.args = list_copy(saop->args);
+ /*
+ * Record if the operator is strict to perform appropriate action on
+ * encountering null values in the element array.
+ */
+ state->opisstrict = op_strict(saop->opno);
+
/* Initialize iteration variable to first member of ArrayExpr */
arrayexpr = (ArrayExpr *) lsecond(saop->args);
state->next = list_head(arrayexpr->elements);
@@ -1028,6 +1049,18 @@ arrayexpr_next_fn(PredIterInfo info)
if (state->next == NULL)
return NULL;
+ /* skip nulls if ok to do so */
+ if (state->opisstrict && state->next)
+ {
+ Node *node = (Node *) lfirst(state->next);
+
+ while (node && IsA(node, Const) && ((Const *)
node)->constisnull)
+ {
+ state->next = lnext(state->next);
+ if (state->next)
+ node = (Node *) lfirst(state->next);
+ }
+ }
lsecond(state->opexpr.args) = lfirst(state->next);
state->next = lnext(state->next);
return (Node *) &(state->opexpr);
diff --git a/src/test/regress/expected/inherit.out
b/src/test/regress/expected/inherit.out
index a79f891da7..065ea86d72 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -1715,11 +1715,7 @@ explain (costs off) select * from list_parted where a =
'ab' or a in (null, 'cd'
Append
-> Seq Scan on part_ab_cd
Filter: (((a)::text = 'ab'::text) OR ((a)::text = ANY
('{NULL,cd}'::text[])))
- -> Seq Scan on part_ef_gh
- Filter: (((a)::text = 'ab'::text) OR ((a)::text = ANY
('{NULL,cd}'::text[])))
- -> Seq Scan on part_null_xy
- Filter: (((a)::text = 'ab'::text) OR ((a)::text = ANY
('{NULL,cd}'::text[])))
-(7 rows)
+(3 rows)
explain (costs off) select * from list_parted where a = 'ab';
QUERY PLAN
@@ -1797,6 +1793,34 @@ explain (costs off) select * from range_list_parted
where a between 3 and 23 and
Filter: ((a >= 3) AND (a <= 23) AND (b = 'ab'::bpchar))
(7 rows)
+explain (costs off) select * from generate_series(11, 12) v(a) where exists
(select count(*) from range_list_parted where a = 1 or a in (null, v.a));
+ QUERY PLAN
+----------------------------------------------------------------------------------
+ Function Scan on generate_series v
+ Filter: (SubPlan 1)
+ SubPlan 1
+ -> Aggregate
+ -> Append
+ -> Seq Scan on part_1_10_ab
+ Filter: ((a = 1) OR (a = ANY (ARRAY[NULL::integer,
v.a])))
+ -> Seq Scan on part_1_10_cd
+ Filter: ((a = 1) OR (a = ANY (ARRAY[NULL::integer,
v.a])))
+ -> Seq Scan on part_10_20_ab
+ Filter: ((a = 1) OR (a = ANY (ARRAY[NULL::integer,
v.a])))
+ -> Seq Scan on part_10_20_cd
+ Filter: ((a = 1) OR (a = ANY (ARRAY[NULL::integer,
v.a])))
+ -> Seq Scan on part_21_30_ab
+ Filter: ((a = 1) OR (a = ANY (ARRAY[NULL::integer,
v.a])))
+ -> Seq Scan on part_21_30_cd
+ Filter: ((a = 1) OR (a = ANY (ARRAY[NULL::integer,
v.a])))
+ -> Seq Scan on part_40_inf_ab
+ Filter: ((a = 1) OR (a = ANY (ARRAY[NULL::integer,
v.a])))
+ -> Seq Scan on part_40_inf_cd
+ Filter: ((a = 1) OR (a = ANY (ARRAY[NULL::integer,
v.a])))
+ -> Seq Scan on part_40_inf_null
+ Filter: ((a = 1) OR (a = ANY (ARRAY[NULL::integer,
v.a])))
+(23 rows)
+
/* Should select no rows because range partition key cannot be null */
explain (costs off) select * from range_list_parted where a is null;
QUERY PLAN
diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index 2e42ae115d..c31cd6dab4 100644
--- a/src/test/regress/sql/inherit.sql
+++ b/src/test/regress/sql/inherit.sql
@@ -651,6 +651,7 @@ explain (costs off) select * from range_list_parted;
explain (costs off) select * from range_list_parted where a = 5;
explain (costs off) select * from range_list_parted where b = 'ab';
explain (costs off) select * from range_list_parted where a between 3 and 23
and b in ('ab');
+explain (costs off) select * from generate_series(11, 12) v(a) where exists
(select count(*) from range_list_parted where a = 1 or a in (null, v.a));
/* Should select no rows because range partition key cannot be null */
explain (costs off) select * from range_list_parted where a is null;
--
2.11.0
