Wrong result with OR + NOT NULL in maria-5.3

[Philip Stoev]

Not reproducible in maria-5.2. The following query:

SELECT * FROM t1 WHERE (f3 = 83) OR (f10 = 'q' AND f3 IS NULL);

returns

+------+------+
| f3 | f10 |
+------+------+
| NULL | r |
+------+------+

which is obviously wrong since neither f3 = 83 nor f10 = 'q'

test case:

CREATE TABLE t1 (
        f3 int(11),
        f10 varchar(1),
        KEY (f3)
);
INSERT IGNORE INTO t1 VALUES ('9','k'),(NULL,'r');

SELECT * FROM t1 WHERE (f3 = 83) OR (f10 = 'z' AND f3 IS NULL);

bzr version-info:

revision-id: <email address hidden>
date: 2011-03-01 10:22:22 +0300
build-date: 2011-03-02 11:45:01 +0200
revno: 2928
branch-nick: maria-5.3

[Sergey Petrunia (sergefp)]

In 5.3:

MariaDB [j28]> explain SELECT * FROM t1 WHERE (f3 = 83) OR (f10 = 'z' AND f3 IS NULL);
+----+-------------+-------+-------------+---------------+------+---------+-------+------+-------+
| id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra |
+----+-------------+-------+-------------+---------------+------+---------+-------+------+-------+
| 1 | SIMPLE | t1 | ref_or_null | f3 | f3 | 5 | const | 2 | |
+----+-------------+-------+-------------+---------------+------+---------+-------+------+-------+
1 row in set (0.01 sec)

Note the lack of "Using where". Debugging shows that
- WHERE clause is indeed not checked.
- it is not there because it was removed by the "remove parts of WHERE that are guaranteed to be true by use of ref-access" functionality inside make_cond_for_table().

[Sergey Petrunia (sergefp)]

This is 5.2, for comparison:
MariaDB [j28]> explain SELECT * FROM t1 WHERE (f3 = 83) OR (f10 = 'z' AND f3 IS NULL);
+----+-------------+-------+-------------+---------------+------+---------+-------+------+-------------+
| id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra |
+----+-------------+-------+-------------+---------------+------+---------+-------+------+-------------+
| 1 | SIMPLE | t1 | ref_or_null | f3 | f3 | 5 | const | 2 | Using where |
+----+-------------+-------+-------------+---------------+------+---------+-------+------+-------------+
1 row in set (0.00 sec)

[Sergey Petrunia (sergefp)]

Exact location of the problem:

test_if_ref()/part_of_refkey() will indicate to make_cond_for_table() that condition "f3 = 83" will be always true, which actually is not satisfied by the (NULL,'r'), which will be returned by ref-or-null access.

when make_cond_for_table() considers a WHERE clause of:

   (f3 = 83) OR (f10 = 'z' AND f3 IS NULL)

and it is told that (f3 = 83) is universally true, it (correctly) concludes that entire WHERE condition is universally true, so there is no point in checking it.

[Sergey Petrunia (sergefp)]

So, currently test_if_ref()/part_of_refkey() assume that "f3=83" is universally true. This bug was introduced when backporting DS-MRR/ICP/other-related-code into 5.3.

How it worked before the backport
-------------------------------------------------
Before the backport, the part_of_refkey() looked like this:

...
  {
    KEY_PART_INFO *key_part=
      table->key_info[table->reginfo.join_tab->ref.key].key_part;

    for (uint part=0 ; part < ref_parts ; part++,key_part++)
      if (field->eq(key_part->field) &&
   !(key_part->key_part_flag & (HA_PART_KEY_SEG | HA_NULL_PART)))
 return table->reginfo.join_tab->ref.items[part];
  }

The important part here: we check HA_NULL_PART bit. If it is present, equality will not be removed. It serves two purposes:
P1. Handle the case where we get NULL value from ref.items[part], make a lookup in the table, and get a matching record with NULLs. The equality will be checked and will filter the record out.

P2. Also cover ref_or_null access method. When ref_or_null is used, table access can return either the lookup value or a record with NULLs, so the equality is not universally guaranteed. ref_or_null is only employed for NULL-able columns (no point to look for NULLs in non-NULLable column), so if we keep the equality for NULLable columns, we cut off ref_or_null, too.

[Sergey Petrunia (sergefp)]

Why it got broken
---------------------------

Somewhere along with DS-MRR/ICP works, we've added "Early/Late NULLs filtering", which would cut off the case of P1 form the previous post. That is, if we have a ref access on "t.key=col" and we get NULL for col, the new code will not search for NULL value of t.key, and so will not need the equality check to filter out NULL results. We forgot about case p2, though.

The implementation in the code was odd. It worked as follows: the check in part_of_refkey() looked the same:

   !(key_part->key_part_flag & (HA_PART_KEY_SEG | HA_NULL_PART)))
        return table->reginfo.join_tab->ref.items[part];

However, the part that sets the HA_PART_KEY_SEG flag was gone. It had been located in table.cc:open_binary_frm() and looked like this:

...
 to_be_deleted:

        /*
          If the field can be NULL, don't optimize away the test
          key_part_column = expression from the WHERE clause
          as we need to test for NULL = NULL.
        */
        if (field->real_maybe_null())
          key_part->key_part_flag|= HA_NULL_PART;
...

so I went ahead and deleted that code. The effect was as desired: lots equalities weren't checked anymore, and lots of "Using where" were gone from ref access EXPLAINs.

[Sergey Petrunia (sergefp)]

What is in the fix
-------------------------

When I've tried to remove HA_NULL_PART flag completely (so that we don't have checks for something we don't set)
I discovered that the flag is used in MyISAM under some circumstances.

So, the solution is:
* Put the code that sets HA_NULL_PART flag back.
* Modify the check in part_of_refkey() so that it
 1. removes the equality in case P1
 2. keeps the equality in case P2

in other words:

1. NULL-ability of the lookup column should not prevent the removal of ref-equality.
2. but if ref_or_null access will alternate the column's value between the lookup value and NULL, then the equality should not be removed.