Re: [Maria-developers] MDEV-28602 Wrong result with outer join, merged derived table and view
4 Jan
2023
4 Jan
'23
11:36 a.m.
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
726
Age (days ago)
726
Last active (days ago)
0 comments
1 participants
participants (1)
-
Sergey Petrunia