Thanks for the comments.
On 2018/02/01 16:40, Ashutosh Bapat wrote:
> On Thu, Feb 1, 2018 at 12:26 PM, Amit Langote
> <[email protected]> wrote:
>> Hi.
>>
>> When addressing a review comment on the fast partition pruning thread [1],
>> I noticed that specifying null in the IN-list will cause constraint
>> exclusion to wrongly fail to refute a table's check predicate.
>>
>> create table foo (a int check (a = 1));
>> insert into foo values (null), (1);
>>
>> -- ExecEvalScalarArrayOp() won't return true for any record in foo
>> select * from foo where a in (null, 2);
>> a
>> ---
>> (0 rows)
>
> AFAIU, that's true only when = operator is strict. For a non-strict
> operator which treats two NULL values as equivalent it would return
> row with NULL value.
That's true.
>> -- The null in the IN-list prevents constraint exclusion to exclude foo
>> -- from being scanned in the first place
>> explain (costs off) select * from foo where a in (null, 2);
>> QUERY PLAN
>> ---------------------------------------------
>> Seq Scan on foo
>> Filter: (a = ANY ('{NULL,2}'::integer[]))
>> (2 rows)
>>
>> I propose a patch that teaches predtest.c to disregard any null values in
>> a SAOP (i.e., the IN (..) expression) when performing constraint exclusion
>> using that SAOP, because they cause predicate_refuted_by_recurse()'s logic
>> to fail to conclude the refutation. There is a rule that all items of an
>> OR clause (SAOP is treated as one) must refute the predicate. But the
>> OpExpr constructed with null as its constant argument won't refute
>> anything and thus will cause the whole OR clause to fail to refute the
>> predicate.
>
> A cursory look through constraint exclusion code esp.
> operator_predicate_proof() doesn't show any special handling for
> strict/non-strict operators. Probably that's why that function refuses
> to refute/imply anything when it encounters NULLs.
> 1593 * We have two identical subexpressions, and two other
> subexpressions that
> 1594 * are not identical but are both Consts; and we have commuted the
> 1595 * operators if necessary so that the Consts are on the
> right. We'll need
> 1596 * to compare the Consts' values. If either is NULL, fail.
> 1597 */
> 1598 if (pred_const->constisnull)
> 1599 return false;
> 1600 if (clause_const->constisnull)
> 1601 return false;
There does actually exist strictness check in
predicate_refuted_by_simple_clause(), but comes into play only if the
predicate is a IS NULL clause. In our case, both predicate and clause
expressions would be operator clauses for which the strictness doesn't
seem to be considered.
>> -- With the patch
>> explain (costs off) select * from foo where a in (null, 2);
>> QUERY PLAN
>> --------------------------
>> Result
>> One-Time Filter: false
>> (2 rows)
>>
>> explain (costs off) select * from foo where a in (null, 2, 1);
>> QUERY PLAN
>> -----------------------------------------------
>> Seq Scan on foo
>> Filter: (a = ANY ('{NULL,2,1}'::integer[]))
>> (2 rows)
>>
>> Thoughts?
>
> AFAIU, this doesn't look to be the right way to fix the problem; it
> assumes that the operators are strict. Sorry, if I have misunderstood
> the patch and your thoughts behind it. May be constraint exclusion
> code should be taught to treat strict/non-strict operators separately.
> I am not sure.
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.
Thanks,
Amit
From b68afd6323852b7a503e634aef3699b922b9d567 Mon Sep 17 00:00:00 2001
From: amit <[email protected]>
Date: Thu, 1 Feb 2018 11:32:23 +0900
Subject: [PATCH v2] Disregard nulls in SAOP rightarg array/list during CE
---
src/backend/optimizer/util/predtest.c | 28 ++++++++++++++++++++++++++++
src/test/regress/expected/inherit.out | 6 +-----
2 files changed, 29 insertions(+), 5 deletions(-)
diff --git a/src/backend/optimizer/util/predtest.c
b/src/backend/optimizer/util/predtest.c
index 8106010329..5efd7f688b 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,12 @@ 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->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 +1005,7 @@ arrayconst_cleanup_fn(PredIterInfo info)
typedef struct
{
OpExpr opexpr;
+ bool opisstrict; /* Is the operator strict? */
ListCell *next;
} ArrayExprIterState;
@@ -1016,6 +1030,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 +1048,14 @@ arrayexpr_next_fn(PredIterInfo info)
if (state->next == NULL)
return NULL;
+ /* 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);
+ }
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..d56e5d25d9 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
--
2.11.0
