Re: bk commit into 5.1 tree (gluh:1.2608) WL#3732

[Sergei Golubchik]

Hi!

On Sep 19, gluh@stripped wrote:
> ChangeSet@stripped, 2007-09-19 13:30:10+05:00, gluh@stripped +10 -0
>   WL#3732 Information schema optimization(addon)
>   fill_schema_table_from_frm():

Great!

>   1. skip open_table_from_share() call if it's possible.
>   2. I_S TRIGGERS fields use OPEN_FRM_ONLY method

it'd be more clear if you write "open TRG file only".
you don't open frm file here, do you ?

>   3. I_S VIEWS fields(except IS_UPDATABLE) use OPEN_FRM_ONLY method

good, but why do you need to call open_new_frm() directly ?

What about

    4. use OPEN_FRM_ONLY for partitioning info

?
Did you forget about it, found to be impossible, or decided to do
in a separate patch ?

> diff -Nrup a/sql/sql_parse.cc b/sql/sql_parse.cc
> --- a/sql/sql_parse.cc	2007-08-30 00:59:33 +05:00
> +++ b/sql/sql_parse.cc	2007-09-19 13:30:07 +05:00
> @@ -4949,7 +4949,7 @@ check_table_access(THD *thd, ulong want_
>      }
>  
>      if (tables->derived ||
> -        (tables->table && (int)tables->table->s->tmp_table))
> +        (tables->table && tables->table->s &&
> (int)tables->table->s->tmp_table))

when tables->table->s can be 0 ? when you show information for views ?

>        continue;
>      thd->security_ctx= sctx;
>      if ((sctx->master_access & want_access) ==
> diff -Nrup a/sql/sql_show.cc b/sql/sql_show.cc
> --- a/sql/sql_show.cc	2007-08-28 05:23:41 +05:00
> +++ b/sql/sql_show.cc	2007-09-19 13:30:07 +05:00
> @@ -2999,7 +3017,22 @@ static int fill_schema_table_from_frm(TH
>      }
>    }
>  
> -  if (share->is_view ||
> +  if (share->is_view)
> +  {
> +    if (open_new_frm(thd, share, table_name->str,
> +                     (uint) (HA_OPEN_KEYFILE | HA_OPEN_RNDFILE |
> +                             HA_GET_INDEX | HA_TRY_READ_ONLY),
> +                     READ_KEYINFO | COMPUTE_TYPES | EXTRA_RECORD |
> +                     OPEN_VIEW_NO_PARSE,
> +                     thd->open_options, &tbl, &table_list,
> thd->mem_root))
> +      goto err1;
> +    table_list.view= (st_lex*) share->is_view;
> +    res= schema_table->process_table(thd, &table_list, table,
> +                                     res, db_name, table_name);
> +    goto err1;
> +  }
> +
> +  if (!(tables->table_open_method & OPEN_FRM_ONLY_WITH_TABLE) ||

now, I think the better name for this macro would be CREATE_TABLE_STRUCT,
and set it together with OPEN_FRM_ONLY.


>        !open_table_from_share(thd, share, table_name->str, 0,
>                               (READ_KEYINFO | COMPUTE_TYPES |
>                                EXTRA_RECORD | OPEN_FRM_FILE_ONLY),
> @@ -3081,6 +3117,10 @@ int get_all_tables(THD *thd, TABLE_LIST 
>    */
>    thd->reset_n_backup_open_tables_state(&open_tables_state_backup);
>  
> +  schema_table_idx= get_schema_table_idx(schema_table);
> +  tables->table_open_method= table_open_method=
> +    get_table_open_method(tables, schema_table, schema_table_idx);
> +    
>    /* 
>      this branch processes SHOW FIELDS, SHOW INDEXES commands.
>      see sql_parse.cc, prepare_schema_table() function where
>      this values are initialized
>    */
>    if (lsel && lsel->table_list.first)
>    {
> +    tables->table_open_method= table_open_method=
> +      get_table_open_method(tables, schema_table, schema_table_idx);

didn't you do it just a few lines before ?

>      error= fill_schema_show_cols_or_idxs(thd, tables, schema_table,
>                                           &open_tables_state_backup);
>      goto err;
>    }
>  
> -  schema_table_idx= get_schema_table_idx(schema_table);
>    get_lookup_field_values(thd, cond, tables, &lookup_field_vals);
>    DBUG_PRINT("INDEX VALUES",("db_name='%s', table_name='%s'",
>                               lookup_field_vals.db_value.str,
> @@ -3589,10 +3627,19 @@ static int get_schema_column_record(THD 
>  
>    show_table= tables->table;
>    count= 0;
> -  restore_record(show_table, s->default_values);
> -  show_table->use_all_columns();               // Required for default
> +  if (table->pos_in_table_list->table_open_method &
> OPEN_FRM_ONLY_WITH_TABLE)
> +  {
> +    restore_record(show_table, s->default_values);
> +    show_table->use_all_columns();               // Required for default
> +  }
>  
> -  for (ptr= show_table->field; (field= *ptr) ; ptr++)
> +  if (tables->view || tables->schema_table ||
> +      (table->pos_in_table_list->table_open_method &
> OPEN_FRM_ONLY_WITH_TABLE))
> +    ptr= show_table->field;
> +  else
> +    ptr= tables->table->s->field;

Do you really need a TABLE for default values ? Looking at
open_binary_frm() it looks like field->val_str() for a field from
tables->table->s->field array should give you that. I didn't test it
though.

> +  for (; (field= *ptr) ; ptr++)
>    {
>      const char *tmp_buff;
>      uchar *pos;
> @@ -3604,6 +3651,9 @@ static int get_schema_column_record(THD 
>      char *end;
>      int decimals, field_length;
>  
> +    field->table= show_table;
> +    show_table->in_use= thd;

Why ?

>      if (wild && wild[0] &&
>          wild_case_compare(system_charset_info, field->field_name,wild))
>        continue;

Regards / Mit vielen Grüssen,
Sergei

-- 
   __  ___     ___ ____  __
  /  |/  /_ __/ __/ __ \/ /   Sergei Golubchik <serg@stripped>
 / /|_/ / // /\ \/ /_/ / /__  Principal Software Developer
/_/  /_/\_, /___/\___\_\___/  MySQL GmbH, Dachauer Str. 37, D-80335 München
       <___/                  Geschäftsführer: Kaj Arnö - HRB
München 162140