maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

Hi Sanja,

Please find my feedback below. We should discuss it.

On Fri, Dec 19, 2014 at 01:24:12PM +0000, sanja@xxxxxxxxxxxx wrote:
> At file:///home/bell/maria/bzr/work-maria-5.5-MDEV-6892/
> 
> ------------------------------------------------------------
> revno: 4382
> revision-id: sanja@xxxxxxxxxxxx-20141219132350-6bo1g7q6gbivqdky
> parent: svoj@xxxxxxxxxxx-20141201105829-ctaoe194abtorhz2
> committer: sanja@xxxxxxxxxxxx
> 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?

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

> +  }
> +  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?

> +
>  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.

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

>  }
>  
> 
> === 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"
*/

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

>        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?


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