maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

Hi Varun,

Please find more input, I'm commenting on the second patch
(the review is probably incomplete, I've made just one pass so might have
missed something)

> diff -urpN '--exclude=.*' 10.5-mdev8306-orig/sql/opt_subselect.cc 10.5-mdev8306/sql/opt_subselect.cc
> --- 10.5-mdev8306-orig/sql/opt_subselect.cc	2019-07-10 11:30:41.151696943 +0300
> +++ 10.5-mdev8306/sql/opt_subselect.cc	2019-07-16 13:07:13.948467315 +0300
> @@ -5517,6 +5517,31 @@ enum_nested_loop_state join_tab_executio
>        sjm->materialized= TRUE;
>      }
>    }
> +  /*
> +    This should be the place to add the sub_select call for the
> +    prefix of the join order
> +  */
> +
> +  else if (tab->is_order_nest)
> +  {
> +    enum_nested_loop_state rc;
> +    JOIN *join= tab->join;
> +    NEST_INFO *nest_info= join->order_nest_info;
> +
> +    if (!nest_info->materialized)
> +    {
> +      JOIN_TAB *join_tab= join->join_tab + join->const_tables;
> +      JOIN_TAB *save_return_tab= join->return_tab;
> +      if ((rc= sub_select(join, join_tab, FALSE)) < 0 ||
> +          (rc= sub_select(join, join_tab, TRUE)) < 0)
> +      {
> +        join->return_tab= save_return_tab;
> +        DBUG_RETURN(rc);
> +      }
> +      join->return_tab= save_return_tab;
> +      nest_info->materialized= TRUE;
> +    }

Looks a bit repetitive with the SJM nest code above, but this is not a big deal.

A question: if the JOIN_TABs for the tables inside the sort nest are still in
the join->join_tab array, where is the code to "jump over" them in regular join
execution?


> +  }
>  
>    DBUG_RETURN(NESTED_LOOP_OK);
>  }
...
> diff -urpN '--exclude=.*' 10.5-mdev8306-orig/sql/sql_class.h 10.5-mdev8306/sql/sql_class.h
> --- 10.5-mdev8306-orig/sql/sql_class.h	2019-07-16 13:10:05.448491384 +0300
> +++ 10.5-mdev8306/sql/sql_class.h	2019-07-16 13:07:13.948467315 +0300
> @@ -6037,6 +6037,24 @@ public:
>    Copy_field *copy_field; /* Needed for SJ_Materialization scan */
>  };
>  
> +class NEST_INFO : public Sql_alloc
> +{
> +public:
> +  ~NEST_INFO()
> +  {
> +    nest_tab= NULL;
> +    table= NULL;
> +  }
What is this for? Object members should not be used after destructor is
called. If you're concerned about accidental access, please use TRASH_XXX
macros to fill the variables with "already-freed" marker.

> +  TMP_TABLE_PARAM tmp_table_param;
> +  List<Item> nest_base_table_cols;
> +  List<Item> nest_temp_table_cols;
> +  TABLE *table;
> +  st_join_table *nest_tab;
> +  uint n_tables;
> +  bool materialized; /* TRUE <=> materialization already performed */
> +  table_map nest_tables_map;
> +};
> +
>  
>  /* Structs used when sorting */
>  struct SORT_FIELD_ATTR
...
> diff -urpN '--exclude=.*' 10.5-mdev8306-orig/sql/sql_select.cc 10.5-mdev8306/sql/sql_select.cc
> --- 10.5-mdev8306-orig/sql/sql_select.cc	2019-07-16 13:10:05.452491478 +0300
> +++ 10.5-mdev8306/sql/sql_select.cc	2019-07-16 13:07:13.952467409 +0300
>    return retval;
>  }
>  
> +void substitute_base_to_nest_items(JOIN *join)
> +{
> +  NEST_INFO *order_nest_info= join->order_nest_info;
> +  if (!order_nest_info)
> +    return;
> +  REPLACE_NEST_FIELD_ARG arg= {join};
> +  join->conds= join->conds->transform(join->thd, &Item::replace_with_nest_items,
> +                                      (uchar *) &arg);
> +
> +  List_iterator<Item> it(join->fields_list);
> +  Item *item, *new_item;
> +  uint i=0;
> +  while ((item= it++))
> +  {
> +    if ((new_item= item->transform(join->thd,
> +                                   &Item::replace_with_nest_items,
> +                                  (uchar *) &arg)) != item)
> +      it.replace(new_item);
> +  }
Ok, so you are processing the select list, and the WHERE condition.
What about the ON expressions, and the right sides of ref access? 

Also, how do we solve the issue of the same Item_field object being used in two
conditions at once:
- one inside the sort nest
- one outside the sort nest

(remember the example with 
"
  (inner_tbl_cond1 AND outer_tbl_con1) OR 
  (inner_tbl_cond2 AND outer_tbl_cond2)
"

> +}
> +
>  
>  /**
...
> @@ -12136,6 +12314,35 @@ end_sj_materialize(JOIN *join, JOIN_TAB
>  }
>  
>  
> +enum_nested_loop_state
> +end_nest_materialization(JOIN *join, JOIN_TAB *join_tab, bool end_of_records)
> +{
> +  int error;
> +  THD *thd= join->thd;
> +  NEST_INFO *nest_info= join->order_nest_info;
> +  DBUG_ENTER("end_sj_materialize");

Please change to match the function name.

> +  if (!end_of_records)
> +  {
> +    TABLE *table= nest_info->table;
> +    fill_record(thd, table, table->field,
> +                nest_info->nest_base_table_cols, TRUE, FALSE);
> +
> +    if (unlikely(thd->is_error()))
> +      DBUG_RETURN(NESTED_LOOP_ERROR); /* purecov: inspected */
> +    if (unlikely((error= table->file->ha_write_tmp_row(table->record[0]))))
> +    {
> +      /* create_myisam_from_heap will generate error if needed */
> +      if (table->file->is_fatal_error(error, HA_CHECK_DUP) &&

Is this check necessary? The table doesn't have any unique keys, why do we ignore
duplicates? 

> +          create_internal_tmp_table_from_heap(thd, table,
> +                                              nest_info->tmp_table_param.start_recinfo,
> +                                              &nest_info->tmp_table_param.recinfo,
> +                                              error, 1, NULL))
> +        DBUG_RETURN(NESTED_LOOP_ERROR); /* purecov: inspected */
> +    }
> +  }
> +  DBUG_RETURN(NESTED_LOOP_OK);
> +}
> +
>  /* 
>    Check whether a join buffer can be used to join the specified table   
>  

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