Re: bk commit into 5.1 tree (ramil:1.2556) BUG#29253

[Guilhem Bichot]

Hello Ramil,

I'm not really reviewing it, just a question:

On Sat, Jul 14, 2007 at 12:13:41AM +0500, ramil@stripped wrote:
> ChangeSet@stripped, 2007-07-14 00:13:37+05:00, ramil@stripped +2 -0
>   Fix for bug #29253: csv table reportedly marked as crashed
>   
>   Problem: the data file changes made during delete/update are not visible to 
>   other threads as the file is reopened, so reading data 
>   with old descriptors might miss the changes.
>   
>   Fix: reopen the data file before reading if it was reopened during 
>   delete/update to ensure there's no data behind.
>   
>   Note: there's no simple test case.
> 
>   storage/csv/ha_tina.cc@stripped, 2007-07-14 00:13:35+05:00, ramil@stripped +46 -6
>     Fix for bug #29253: csv table reportedly marked as crashed
>       - use the data file version technic to ensure we always see changes
>         made by other threads:
>           a) increase share->data_file_version each time we reopen the data 
>              file, i.e. at the end of update/delete.
>           b) compare the local data file version with the shared one each time 
>              we want to read data, reopen it if they differ.
> 
>   storage/csv/ha_tina.h@stripped, 2007-07-14 00:13:35+05:00, ramil@stripped +3 -0
>     Fix for bug #29253: csv table reportedly marked as crashed
>       - use the data file version technic to ensure we always see changes
>         made by other threads:
>           a) increase share->data_file_version each time we reopen the data 
>              file, i.e. at the end og update/delete.
>           b) compare the local data file version with shared one each time 
>              we want to read data, reopen it if they differ.
> 
> diff -Nrup a/storage/csv/ha_tina.cc b/storage/csv/ha_tina.cc
> --- a/storage/csv/ha_tina.cc	2007-07-10 13:09:06 +05:00
> +++ b/storage/csv/ha_tina.cc	2007-07-14 00:13:35 +05:00
> @@ -163,6 +163,7 @@ static TINA_SHARE *get_share(const char 
>      share->rows_recorded= 0;
>      share->update_file_opened= FALSE;
>      share->tina_write_opened= FALSE;
> +    share->data_file_version= 0;
>      strmov(share->table_name, table_name);
>      fn_format(share->data_file_name, table_name, "", CSV_EXT,
>                MY_REPLACE_EXT|MY_UNPACK_FILENAME);
> @@ -440,7 +441,7 @@ ha_tina::ha_tina(handlerton *hton, TABLE
>    */
>    current_position(0), next_position(0), local_saved_data_file_length(0),
>    file_buff(0), chain_alloced(0), chain_size(DEFAULT_CHAIN_LENGTH),
> -  records_is_known(0)
> +  local_data_file_version(0), records_is_known(0)
>  {
>    /* Set our original buffers from pre-allocated memory */
>    buffer.set((char*)byte_buffer, IO_SIZE, &my_charset_bin);
> @@ -815,6 +816,7 @@ int ha_tina::open(const char *name, int 
>      DBUG_RETURN(HA_ERR_CRASHED_ON_USAGE);
>    }
>  
> +  local_data_file_version= share->data_file_version;
>    if ((data_file= my_open(share->data_file_name, O_RDONLY, MYF(0))) == -1)
>      DBUG_RETURN(0);
>  
> @@ -985,6 +987,33 @@ int ha_tina::delete_row(const uchar * bu
>  }
>  
>  
> +/**
> +  @brief Initialize the data file.

indentation?

> +  
> +  @details Compare the local version of the data file with the shared one.
> +  If they differ, there are some changes behind and we have to reopen
> +  the data file to make the changes visible.
> +  Call @c file_buff->init_buff() at the end to read the beginning of the 
> +  data file into buffer.
> +  
> +  @retval  0  OK.
> +  @retval  1  There was an error.
> +*/
> +
> +int ha_tina::init_data_file()
> +{
> +  if (local_data_file_version != share->data_file_version)
> +  {
> +    local_data_file_version= share->data_file_version;
> +    if (my_close(data_file, MYF(0)) ||
> +        (data_file= my_open(share->data_file_name, O_RDONLY, MYF(0))) == -1)
> +      return 1;
> +  }
> +  file_buff->init_buff(data_file);
> +  return 0;
> +}

Is it possible that just after thread1 has finished doing
my_close()+my_open() above, another thread2 quickly does an
update/delete, and so again thread1's descriptor is old?

Or is this prevented by the table-level lock?
And if this is possible, is it an issue?