List:Commits« Previous MessageNext Message »
From:Konstantin Osipov Date:April 28 2008 11:46am
Subject:Re: bk commit into 6.1 tree (dlenev:1.2607) BUG#35529
View as plain text  
* dlenev@stripped <dlenev@stripped> [08/04/22 16:21]:
> @@ -761,18 +761,27 @@ bool fk_drop_all_constraint_names_for_ta
>  
>    safe_mutex_assert_owner(&LOCK_open);
>  
> +  /*
> +    Even if this temporary table was created by ALTER as new version of
> +    normal table we should not drop foreign key names associated with it
> +    as they are also associated with old version of the table.
> +  */
> +  if (table->internal_tmp_table)
> +    return FALSE;
> +

This looks like a hack. 
A temporary table should not have foreign keys associated with it,
so the comment isn't describing the case when this is check
necessary very clearly.

Please try to move this logic out of the data dictionary method, which is
supposed to simply perform an operation on .frm/filesystem, and
explain exactly why it's necessary:
 - when there was an ALTER of a base table with a foreign key
 - the new .FRM file of internal temporary table contains foreign
   key names
 - ALTER execution was terminated abnormally, so the temporary
   .frm was left behind
   crashed
 - we attempt to drop the database and discover this .frm file, 
 and stumble over foreign key names which (perhaps) no longer
 exist).

>    VOID(build_table_filename(path, FN_REFLEN - 1, table->db,
>                              table->table_name, "", 0));
>  
>    /*
> -    To get list of foreign keys we need get access table's share.
> -    Instead of doing full-blown share creation and going through
> -    table definition cache we use object allocated on the stack
> -    and initialize as if we were opening temporary table.
> +    To get list of foreign keys we need get access to table's share.
> +    Instead of doing full-blown share creation and filling we use
> +    object allocated on the stack, initialized as if we were opening
> +    temporary table and routine which reads only part of .FRM which
> +    holds foreign key descriptions.
>    */
>    init_tmp_table_share(thd, &share, table->db, 0, table->table_name,
> path);
>  
> -  if (open_table_def(thd, &share, 0))
> +  if (fill_table_def_fk_list(thd, &share))
>    {
>      free_table_share(&share);
>      return TRUE;
> diff -Nrup a/sql/mysql_priv.h b/sql/mysql_priv.h
> --- a/sql/mysql_priv.h	2008-03-21 13:55:40 +03:00
> +++ b/sql/mysql_priv.h	2008-04-22 16:18:49 +04:00
> @@ -2171,6 +2171,7 @@ void init_tmp_table_share(THD *thd, TABL
>  void free_table_share(TABLE_SHARE *share);
>  int open_table_def(THD *thd, TABLE_SHARE *share, uint db_flags);
>  void open_table_error(TABLE_SHARE *share, int error, int db_errno, int errarg);
> +bool fill_table_def_fk_list(THD *thd, TABLE_SHARE *share);
>  int open_table_from_share(THD *thd, TABLE_SHARE *share, const char *alias,
>                            uint db_stat, uint prgflag, uint ha_open_flags,
>                            TABLE *outparam, open_table_mode open_mode);
> diff -Nrup a/sql/table.cc b/sql/table.cc
> --- a/sql/table.cc	2008-03-21 13:55:43 +03:00
> +++ b/sql/table.cc	2008-04-22 16:18:49 +04:00
> @@ -1667,6 +1667,226 @@ static int open_binary_frm(THD *thd, TAB
>  } /* open_binary_frm */
>  
>  
> +/**
> +   Initialize foreign key list in TABLE_SHARE from .FRM section describing
> +   foreign keys.
> +
> +   @param thd    Thread context.
> +   @param share  TABLE_SHARE object to be initialized.
> +
> +   @note This is auxiliary function which allows to get information
> +         about table's foreign keys without doing full-blown .FRM reading.
> +         It might go away on one of the later milestones of WL#148.
> +         If this won't happen then long-term it is going to be replaced
> +         with function getting list of foreign keys directly from data
> +         dictionary tables and not .FRMs.
> +   @note Unlike open_table_def()/open_binary_frm() it does not create
> +         handler object in the process, so one can safely call this
> +         it even if handler files have been already deleted.
> +
> +   QQ: This is a fairly big piece of code which is to some extent
> +       duplicates code in open_table_def() and open_binary_frm().
> +       May be it is better to use them instead (this will require
> +       changing them to be able to work even if handler files are
> +       gone)?

Do you have a suggestion how to make open_table_def()
parameterizable to suit your needs?

I'm okay with this solution, there is some code duplication that
may lead to future merge issues, if someone else modifies frm format
in 6.0 or 6.1, but on the other hand the code is at least readable
(unlike the code of open_table_def()).

Alternatively you could extract the format-related logic from
open_binary_frm() and create a sort of iterator, which would
return one section after another. Then this functionality could be
used in open_binary_frm() and in fill_table_def_fk_list().
Please feel free to invest some time to research this
possibility.

> +
> +   @retval FALSE  Success.
> +   @retval TRUE   Failure.
> +*/


> +
> +bool fill_table_def_fk_list(THD *thd, TABLE_SHARE *share)
> +{
> +  File file;
> +  char	path[FN_REFLEN];
> +  uchar head[64], forminfo[288], *buff= 0, *strpos;
> +  uint mysql_version;
> +  uint i, keys, keys_with_parser, key_flags, key_parts;
> +  ulong form_pos, extra_offset, extra_length;
> +  bool has_long_table_comment;
> +  bool result= TRUE;
> +
> +  strxmov(path, share->normalized_path.str, reg_ext, NullS);
> +  if ((file= my_open(path, O_RDONLY | O_SHARE, MYF(MY_WME))) < 0)
> +    return TRUE;
> +
> +  /* Read .FRM header and check that it has proper version and type. */
> +  if (my_read(file, head, 64, MYF(MY_NABP)))
> +    goto err;
> +
> +  if (head[0] == (uchar) 254 && head[1] == 1)
> +  {
> +    if (!(head[2] == FRM_VER || head[2] == FRM_VER+1 ||
> +          (head[2] >= FRM_VER+3 && head[2] <= FRM_VER+4)))
> +    {
> +      /* Unknown .FRM version. */
> +      goto err;
> +    }
> +    /* Normal .FRM */
> +  }
> +  else if (memcmp(head, STRING_WITH_LEN("TYPE=")) == 0)
> +  {
> +    if (memcmp(head+5,"VIEW",4) == 0)
> +    {
> +      /* View can't have foreign keys so we can return immediately. */
> +      goto ok;
> +    }
> +    goto err;
> +  }
> +  else
> +  {
> +    /* Something completely unintelligible. */
> +    goto err;
> +  }
> +
> +  mysql_version= uint4korr(head+51);
> +  if (mysql_version < MYSQL_VERSION_FOREIGN_KEYS_IN_FRM)
> +  {
> +    /*
> +      This is .FRM file from one of older versions of server,
> +      so it can't contain information about foreign keys.
> +    */
> +    goto ok;
> +  }
> +  /*
> +    Recent version of the server also means that version of .FRM format
> +    is fairly recent, so parsing of .FRM file can be simplified.
> +  */
> +  DBUG_ASSERT(head[2] >= FRM_VER + 3);
> +
> +  /*
> +    To be able correctly read information in extra data segment,
> +    where info about foreign keys resides, we need need to know
> +    if this table has long table comment.
> +  */
> +  if (!(form_pos= get_form_pos(file, head, (TYPELIB*) 0)))
> +    goto err;
> +  if (my_pread(file, forminfo, 288, form_pos, MYF(MY_NABP)))
> +    goto err;
> +  has_long_table_comment= (forminfo[46] == (uchar)255);
> +
> +  /*
> +    Also we need to find out number of full-text keys that use pluggable
> +    parsers. To do this we have to read a bit of description of each key.
> +  */
> +  VOID(my_seek(file, (ulong) uint2korr(head+6), MY_SEEK_SET, MYF(0)));
> +  if (read_string(file, (uchar**) &buff, uint2korr(head+28)))
> +    goto err;
> +
> +  if (buff[0] & 0x80)
> +    keys= (buff[1] << 7) | (buff[0] & 0x7f);
> +  else
> +    keys= buff[0];
> +
> +  strpos= buff + 6;
> +  keys_with_parser= 0;
> +
> +  for (i=0 ; i < keys ; i++)
> +  {
> +    key_flags= (uint) uint2korr(strpos) ^ HA_NOSAME;
> +    key_parts= (uint) strpos[4];
> +    strpos+= 8 + key_parts * 9;
> +
> +    if (key_flags & HA_USES_PARSER)
> +      keys_with_parser++;
> +  }
> +  my_free(buff, MYF(0));
> +  buff= 0;
> +  /* End of work with key descriptions. */
> +
> +  /* Now we can work with extra data segment. */
> +  extra_offset= (ulong) (uint2korr(head+6) +
> +                         ((uint2korr(head+14) == 0xffff ?
> +                           uint4korr(head+47) : uint2korr(head+14))) +
> +                         uint2korr(head+16));
> +
> +  if ((extra_length= uint4korr(head+55)))
> +  {
> +    uchar *next_chunk, *buff_end;
> +    DBUG_PRINT("info", ("extra segment size is %lu bytes", extra_length));
> +    if (!(next_chunk= buff= (uchar*) my_malloc(extra_length, MYF(MY_WME))))
> +      goto err;
> +    buff_end= buff + extra_length;
> +
> +    if (my_pread(file, buff, extra_length, extra_offset, MYF(MY_NABP)))
> +      goto err;
> +
> +    /* Skip connect string. */
> +    next_chunk+= uint2korr(next_chunk) + 2;
> +
> +    /* Skip the engine name. */
> +    if (next_chunk + 2 < buff_end)
> +      next_chunk+= uint2korr(next_chunk) + 2;
> +
> +    /* Skip partition info. */
> +    if (next_chunk + 5 < buff_end)
> +      next_chunk+= uint4korr(next_chunk) + 5;
> +
> +    /* Skip new auto_partitioned indicator, which was introduced in 5.1.11 */
> +    DBUG_ASSERT (mysql_version >= 50110);
> +    next_chunk++;
> +
> +    /* Skip names of parsers used by full text keys. */
> +    for (i= 0; i < keys_with_parser; i++)
> +    {
> +      if (next_chunk >= buff_end)
> +      {
> +        DBUG_PRINT("error",
> +                   ("fulltext key uses parser that is not defined in .frm"));
> +        goto err;
> +      }
> +      next_chunk+= strlen((char*) next_chunk) + 1;
> +    }
> +
> +    /* Skip long table comment. */
> +    if (has_long_table_comment)
> +    {
> +      if (next_chunk + 2 > buff_end)
> +      {
> +          DBUG_PRINT("error",
> +                     ("long table comment is not defined in .frm"));
> +          goto err;
> +      }
> +      next_chunk+= 2 + uint2korr(next_chunk);
> +    }
> +
> +    /* Skip section with column's COLUMN_FORMAT and STORAGE properties. */
> +    DBUG_ASSERT (mysql_version >= MYSQL_VERSION_TABLESPACE_IN_FRM &&
> +                 mysql_version >= MYSQL_VERSION_TABLESPACE_IN_FRM_CGE);
> +    if (next_chunk >= buff_end)
> +    {
> +      DBUG_PRINT("error", ("Found no field extra info"));
> +      goto err;
> +    }
> +    else
> +    {
> +      DBUG_PRINT("info", ("Found field extra info"));
> +      next_chunk+= uint2korr(next_chunk);
> +    }
> +
> +    if (fk_restore_from_frm_section(thd, share, (const char**)&next_chunk,
> +                                    (const char*)buff_end))
> +      goto err;
> +
> +    if (next_chunk > buff_end)
> +    {
> +      DBUG_PRINT("error", ("Buffer overflow in field extra info"));
> +      goto err;
> +    }
> +  }
> +
> +ok:
> +  result= FALSE;
> +
> +err:
> +  if (result)
> +    my_error(ER_NOT_FORM_FILE, MYF(0), path);
> +  if (buff)
> +    my_free(buff, MYF(0));
> +  my_close(file, MYF(0));
> +  return result;
> +}
> +
> +
>  /*
>    Open a table based on a TABLE_SHARE
>  
> diff -Nrup a/sql/unireg.h b/sql/unireg.h
> --- a/sql/unireg.h	2008-03-12 11:25:25 +03:00
> +++ b/sql/unireg.h	2008-04-22 16:18:49 +04:00
> @@ -202,6 +202,7 @@
>  #define MYSQL_VERSION_TABLESPACE_IN_FRM_CGE 50120
>  #define MYSQL_VERSION_TABLESPACE_IN_FRM 50205
>  #define MYSQL_VERSION_TABLESPACE_IN_FRM_STR "50205"
> +#define MYSQL_VERSION_FOREIGN_KEYS_IN_FRM 60100
>  
>  /*
>    Minimum length pattern before Turbo Boyer-Moore is used

-- 
Konstantin Osipov                    Moscow, Russia
MySQL Server Runtime Team Lead       Database Group, Sun Microsystems
Thread
bk commit into 6.1 tree (dlenev:1.2607) BUG#35529dlenev22 Apr
  • Re: bk commit into 6.1 tree (dlenev:1.2607) BUG#35529Konstantin Osipov28 Apr