Re: [Maria-developers] MDEV-28602 Wrong result with outer join, merged derived table and view

Hi Rex,

Please find below input for the commit. 

(I've actually committed an alternative fix now, but before doing that I wrote
all the trivial comments below so I'm sending these anyway in order to show
what is expected of a patch. I'm also including the rationale for creating
an alternative patch)

> commit 75af3195482d3da277825663d1150c0dfc55420a
> Author: Rex <rex.johnston@mariadb.com>
> Date:   Fri Dec 23 05:28:59 2022 +1200
> 
>     MDEV-28602  Wrong result with outer join, merged derived table and view
>     
>         const item reference being mishandled in outer to inner join on
>         filling in null values.
> 

First, fixVersion. It is currently set to 10.11, but affectsVersion starts from 10.3
I think the bug and the fix are applicable for all versions.

Second: commit comment. This is not descriptive enough:

>         const item reference being mishandled in outer to inner join on
>         filling in null values.


Something like this would be better: 

<commit-comment>
  A LEFT JOIN with a constant as a column of the inner table produced wrong query 
  result if the optimizer had to write the inner table column into a temp table:

  SELECT ... 
  FROM (SELECT /*non-mergeable select*/ 
        FROM t1 LEFT JOIN (SELECT 'Y' as Val) t2 ON ...) 

  Fixed this by:
  1. Making Item_ref::save_in_field() call check_null_ref() to see if the referred
     table has a NULL-complemented row.
  2. In order to do #1, moved null_ref_table() and check_null_ref() from
     Item_direct_view_ref to Item_ref.
</commit-comment>

Note: it starts with a siccint problem description, which is followed by a
description of the fix.

> diff --git a/sql/sql_join_cache.cc b/sql/sql_join_cache.cc
> index 620c52a3f40..65bedc3d337 100644
> --- a/sql/sql_join_cache.cc
> +++ b/sql/sql_join_cache.cc
> @@ -2622,7 +2622,7 @@ enum_nested_loop_state JOIN_CACHE::join_null_complements(bool skip_last)
>        get_record();
>        /* The outer row is complemented by nulls for each inner table */
>        restore_record(join_tab->table, s->default_values);
> -      mark_as_null_row(join_tab->table);  
> +      mark_as_null_row(join_tab->table);

In the future please make sure patches do not contain unrelated meaningless 
changes like this one.

>        rc= generate_full_extensions(get_curr_rec());
>        if (rc != NESTED_LOOP_OK && rc != NESTED_LOOP_NO_MORE_ROWS)
>          goto finish;

> diff --git a/mysql-test/main/merge.test b/mysql-test/main/merge.test
> index 0485f3ed1c3..05b05287e1b 100644
> --- a/mysql-test/main/merge.test
> +++ b/mysql-test/main/merge.test

Why is the patch in the merge.test ?
merge.test at the top says clearly "Test of MERGE TABLES".
That is, this is a test for tables with ENGINE=MERGE, a feature not related to this bug.

The right test files for this bug would be main/join_outer.test and main/derived.test, 
I would prefer the first one.

> @@ -2886,3 +2886,50 @@ drop table tm, t;
>  --echo #
>  --echo # End of 10.8 tests
>  --echo #
> +
> +--echo #
> +--echo # MDEV-28602 Wrong result with outer join, merged derived table and view
> +--echo #
> +
> +drop table if exists t1, t2;

^^^ This is not needed as there is a DROP TABLE right above

> +create table t1 (
> +  Election int(10) unsigned NOT NULL
> +);
> +
> +insert into t1 (Election) values (1);
> + 
+create table t2 (
+  VoteID int(10),
+  ElectionID int(10),
+  UserID int(10)
+);
+
+insert into t2 (ElectionID, UserID) values (2,  30), (3, 30);
+#  INSERT INTO t2 (ElectionID, UserID) VALUES (1,  30);

^^ No need to have commented-out lines in the commit.
+drop view if exists v1;

^^^ same as above: this is not needed.
+create view v1 as select * from t1
+  left join ( select 'Y' AS Voted, ElectionID from t2 ) AS T 
+    on T.ElectionID = t1.Election
+limit 9;
+# limit X causes merge algorithm select as opposed to temp table
+select * from v1;
...
> diff --git a/sql/item.h b/sql/item.h
> index 2d598546b91..8684477b558 100644
> --- a/sql/item.h
> +++ b/sql/item.h
> @@ -5531,18 +5531,42 @@ class Item_sp
>  
>  class Item_ref :public Item_ident
>  {
> +public:
> +
> +#define NO_NULL_TABLE (reinterpret_cast<TABLE *>(0x1))
> +
> +  void set_null_ref_table()
> +  {
> +      null_ref_table= NO_NULL_TABLE;
> +  }
> +
> +  bool check_null_ref()

This works, but looks like a change that is
 1. Too big
 2. Looks like a partial implementation

I'm looking at methods of Item_direct_view_ref:
- save_val()
- save_org_in_field()
- save_in_result_field()
- val_real() and other val_XXX()

and they all follow the same pattern:

  if (check_null_ref())
    produce SQL NULL;
  else
    Call Item_direct_ref::same_method();

This suggests we could add Item_direct_view_ref::save_in_field() and use the 
same approach.

Conversely, if we move check_null_ref() to be in Item_ref, we will end up with:
- check_null_ref() is in Item_ref
- methods that make use of it are in Item_direct_view_ref()
- methods of Item_ref() do not make check_null_ref() check, except for save_in_field() 
  which does it.

I think the former looks more logical than the latter. I've made the patch for it
and I'll ask Sanja (the Item_ref/views expert) to check it.

BR
 Sergei
-- 
Sergei Petrunia, Software Developer
MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net