maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

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@xxxxxxxxxxx>
> 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