On 04.03.15 21:45, Sergey Petrunia wrote:

Hi Sanja,

Please find my feedback below. We should discuss it.

On Fri, Dec 19, 2014 at 01:24:12PM +0000, sanja@askmonty.org wrote:

...

At file:///home/bell/maria/bzr/work-maria-5.5-MDEV-6892/

------------------------------------------------------------ revno: 4382 revision-id: sanja@askmonty.org-20141219132350-6bo1g7q6gbivqdky parent: svoj@mariadb.org-20141201105829-ctaoe194abtorhz2 committer: sanja@askmonty.org branch nick: work-maria-5.5-MDEV-6892 timestamp: Fri 2014-12-19 14:23:50 +0100 message: MDEV-6892: WHERE does not apply

Taking into account implicit dependence of constant view field from nullable table of left join added.

Fixed finding real table to check if ut turned to NULL (materialized view & derived taken into account) ... === modified file 'sql/item.cc' --- a/sql/item.cc 2014-10-06 17:53:55 +0000 +++ b/sql/item.cc 2014-12-19 13:23:50 +0000 @@ -9790,12 +9790,36 @@ void Item_ref::update_used_tables() (*ref)->update_used_tables(); }

+void Item_direct_view_ref::update_used_tables() +{ + if (view->is_inner_table_of_outer_join()) + { + null_ref_table= NULL; + check_null_ref(); update_used_tables() is called during optimization. Why does it call check_null_ref() which may check null_ref_table->null_row?

Is check_null_ref() function an execution-time function (and so it computes "current" value of an item)

or is it an optimization-time function (and so it computes static attributes and can be called at any point in the query)

It looks like it's a bit of both, which is really confusing. Could you please clarify? can you clarify such devision of functions (I hear first time that we have static atributes during optimisation (where we transform subqueries and many other things)

check_null_ref() was introduced by you in another views+outer joins bugfix. Can we get it documented?

yes, but not in this bugfix.

...

+ } + Item_direct_ref::update_used_tables(); +} Overall, I don't understand why some checks are in used_tables() and some are in update_used_tables(). Could you clarify?

used_tables declared static, so I have 2 posibilities: 1) remove static everywhere 2) repeat some code of check_null_ref() without changing class variables I chose second

...

+ table_map Item_direct_view_ref::used_tables() const { + TABLE *null_ref= null_ref_table; + + if (view->is_inner_table_of_outer_join()) + { + if (null_ref == NULL) + null_ref= view->get_real_join_table(); + else if (null_ref == NO_NULL_TABLE) + null_ref= NULL; + } + else + null_ref= NULL; + return get_depended_from() ? OUTER_REF_TABLE_BIT : ((view->is_merged_derived() || view->merged || !view->table) ? - (*ref)->used_tables() : + ((*ref)->used_tables() ? + (*ref)->used_tables() : + (null_ref ? null_ref->map : (table_map)0 )) : view->table->map); Two-level nesting of conditional expressions is too complicated. Please change into 'if' statement and a comment about what's going on.

OK

How can Item_direct_view_ref() deped on an outer table??

probably if view used in a subquery?

...

}

=== modified file 'sql/item.h' --- a/sql/item.h 2014-11-18 14:42:40 +0000 +++ b/sql/item.h 2014-12-19 13:23:50 +0000 @@ -3319,7 +3319,8 @@ class Item_direct_view_ref :public Item_ { if (null_ref_table == NULL) { - if (!(null_ref_table= view->get_real_join_table())) + if (!view->is_inner_table_of_outer_join() || + !(null_ref_table= view->get_real_join_table())) null_ref_table= NO_NULL_TABLE; } if (null_ref_table != NO_NULL_TABLE && null_ref_table->null_row) @@ -3329,6 +3330,7 @@ class Item_direct_view_ref :public Item_ } return FALSE; } + public: Item_direct_view_ref(Name_resolution_context *context_arg, Item **item, const char *table_name_arg, @@ -3353,8 +3355,10 @@ public: bool subst_argument_checker(uchar **arg); Item *equal_fields_propagator(uchar *arg); Item *replace_equal_field(uchar *arg); - table_map used_tables() const; + table_map used_tables() const; + void update_used_tables(); table_map not_null_tables() const; + bool const_item() const { return used_tables() == 0; } bool walk(Item_processor processor, bool walk_subquery, uchar *arg) { return (*ref)->walk(processor, walk_subquery, arg) ||

=== modified file 'sql/table.cc' --- a/sql/table.cc 2014-10-29 10:22:48 +0000 +++ b/sql/table.cc 2014-12-19 13:23:50 +0000 The function get_real_join_table() lacks comments, which makes it difficult to review. Do I understand correctly that its action can be described by this comment:

/* @brief Given a merged view (or a merged derived table), find its first* base** table.

@detail (*) first means as defined in the syntax (**) base means we need a physical table, one that has a bit in table->map and is present in select_lex->leaf_tables.

Example:

t1 join ( (select * from t2, t3 ) as derivedA join (select * from t5, t6) as derivedC ) as derivedB

here, derivedB->get_real_join_table()="t2" */

yes, but actually i'd replaced 'first' with 'any'

...

@@ -5012,7 +5012,8 @@ TABLE *TABLE_LIST::get_real_join_table() TABLE_LIST *tbl= this; while (tbl->table == NULL || tbl->table->reginfo.join_tab == NULL) I'm quite surprised that joins are run over tables that don't have reginfo.join_tab set appropriately... { - if (tbl->view == NULL && tbl->derived == NULL) + if ((tbl->view == NULL && tbl->derived == NULL) || + tbl->is_materialized_derived()) I'm wondering why part of conditions are in while(...) and the other part is here.

1) historically 2) readable

...

break; /* we do not support merging of union yet */ DBUG_ASSERT(tbl->view == NULL ||

The code below this line looks bizarre (and it is still from your recent fixes to VIEWs): { List_iterator_fast<TABLE_LIST> ti; { List_iterator_fast<TABLE_LIST> ti(tbl->view != NULL ?

why define 'ti' and then immediately redefine it? obviously it is an mistake (first definition is not needed)

BR Sergei