Re: constraint exclusion and nulls in IN (..) clause

Thanks for the comments.

On 2018/02/01 16:40, Ashutosh Bapat wrote:
> On Thu, Feb 1, 2018 at 12:26 PM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> 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