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

[Rafal Somla]

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 {