Re: bzr commit into mysql-6.0-backup branch (cbell:2712) Bug#33364

[Chuck Bell]

Rafal,

Ok, I will do as you suggest.

Rafal Somla wrote:
> Hi Chuck,
> 
> Chuck Bell wrote:
>> Rafal,
>>
>>> 2. Do not set share->is_log_table to FALSE for backup log tables - 
>>> this violates semantics of the flag. I suggest introducing a new flag 
>>> for your purpose (allowing deletion of rows in log tables).
>>
>> That flag is put there to prohibit deletion of tables used as logs. If 
>> I add a new flag, I will have to make extensive modifications to the 
>> csv storage engine. I do not feel that it violates the semantics of 
>> the flag. What I have done is added a very simple exception rule to 
>> allow the restriction to be turned off by the logging code -- 
>> specifically, the backup logging code. All other logs will perform the 
>> same way they always have. I do not see the need to make extensive 
>> changes to the CSV storage engine to satisfy this request when the 
>> existing solution is much less invasive.
>>
> 
> Documentation for the flag says:
> 
>   /*
>     Below flag is needed to make log tables work with concurrent insert.
>     For more details see comment to ha_tina::update_status.
>   */
>   my_bool is_log_table;
> 
> In your code you are using it for a different purpose and I think it is 
> not a good practice. However, I can't provide a concrete example where 
> it breaks something and in the patch you proposed, the new meaning of 
> the flag is used only briefly and in few places so it should work now. 
> If you, as the implementer, can take the full responsibility for using 
> the flag in that way, then I wont reject your patch because of that.
> 
> Rafal
> 
> PS
> I think you exaggerate when you talk about "extensive modifications to 
> the csv storage engine". Here is a small patch which implements 
> HA_EXTRA_ALLOW_LOG_DELETE operation without (ab)using the is_log_table 
> flag:
> 
> === modified file 'storage/csv/ha_tina.cc'
> --- storage/csv/ha_tina.cc      2008-08-23 00:18:35 +0000
> +++ storage/csv/ha_tina.cc      2008-10-24 06:49:32 +0000
> @@ -194,6 +194,8 @@ static TINA_SHARE *get_share(const char
>                                      O_RDWR|O_CREAT, MYF(MY_WME))) == 
> -1) ||
>          read_meta_file(share->meta_file, &share->rows_recorded))
>        share->crashed= TRUE;
> +
> +    share->allow_delete_rows= FALSE;
>    }
> 
>    share->use_count++;
> @@ -996,7 +998,7 @@ int ha_tina::delete_row(const uchar * bu
>    pthread_mutex_unlock(&share->mutex);
> 
>    /* DELETE should never happen on the log table */
> -  DBUG_ASSERT(!share->is_log_table);
> +  DBUG_ASSERT(!share->is_log_table || share->allow_delete_rows);
> 
>    DBUG_RETURN(0);
>  }
> @@ -1171,6 +1173,12 @@ int ha_tina::extra(enum ha_extra_functio
>     share->is_log_table= TRUE;
>     pthread_mutex_unlock(&share->mutex);
>   }
> + else if (operation == HA_EXTRA_ALLOW_LOG_DELETE)
> + {
> +   pthread_mutex_lock(&share->mutex);
> +   share->allow_delete_rows= TRUE;
> +   pthread_mutex_unlock(&share->mutex);
> + }
>    DBUG_RETURN(0);
>  }
> 
> 
> === modified file 'storage/csv/ha_tina.h'
> --- storage/csv/ha_tina.h       2008-06-28 11:00:59 +0000
> +++ storage/csv/ha_tina.h       2008-10-24 06:44:53 +0000
> @@ -50,6 +50,7 @@ typedef struct st_tina_share {
>    bool crashed;             /* Meta file is crashed */
>    ha_rows rows_recorded;    /* Number of rows in tables */
>    uint data_file_version;   /* Version of the data file used */
> +  my_bool allow_delete_rows;
>  } TINA_SHARE;
> 
>  struct tina_set {
>