From: Date: October 24 2007 1:20pm
Subject: Re: bk commit into 5.1 tree (gluh:1.2608) WL#3732
List-Archive: http://lists.mysql.com/commits/36251
Message-Id: <471F2A79.7090205@mysql.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Hi,

Sergei Golubchik wrote:

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

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

>
>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 ?
>
>
impossible because of HASH partitions

>>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 ?
>
>
tables->table->s is 0 when we use I_S.TRIGGERS.
(get_schema_triggers_record()->check_table_access())


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

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

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

>
>>+  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 ?
>
For example Field_string::sql_type() method uses '->in_use' thd(field.cc).
And some other ::sql_type() methods use it too.

New patch is here:
http://lists.mysql.com/commits/36250

Regards,
Gluh