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

[Rafal Somla]

STATUS
------
Patch not approved. New patch requested.

REQUESTS
--------
Related to the code:

1. Please explain what is your idea of how the PURGE statements will behave if 
there is an ongoing BACKUP/RESTORE operation and vice versa (how BACKUP/RESTORE 
should behave if there is ongoing PURGE operation).

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

3. When executing SQLCOM_PURGE_BUP_* commands, do privilege checks before 
anything else, so that a user without enough privileges will not see any other 
error messages.

4. Fix error handling inside scan_backup_log() - with current logic errors will 
be ignored (see inlined comments).

5. Remove fixed computations from inside the loop in scan_backup_log() (see 
inlined comments).

The following two items assume that we don't spend time now on implementing and 
reviewing the PURGE BACKUP IMAGES part of the patch. When it comes to 
implementing this, I can elaborate more on my requests.

6. Do not call form scan_backup_log() function delete_image() as this function 
does not check if deleted file is really a backup image.

7. Remove is_backup_image() function - I have several objections to the way it 
is implemented.

Related to the documentation:

8. Fix the documentation of scan_backup_log() function as explained below. In 
particular make it clear which rows exactly are being removed by the function.

9. Explain which row is deleted by delete_row() method.

Related to the tests:

10. I think the output of the test is nondeterministic (in the part testing 
PURGE ... BEFORE statement). If I'm correct, fix that.

11. Please test the error replies from PURGE BACKUP ... statements.

12. I'd like to see a test checking what happens if PURGE BACKUP ... and 
BACKUP/RESTORE commands run in parallel.

SUGGESTIONS
-----------

1. Instead of using 6 SQLCOM_* commands, use only two: SQLCOM_PURGE_BUP_LOG and 
SQLCOM_PURGE_BUP_IMG. Make further distinctions using lex->type flags, as is 
done in case of other SQL commands (e.g., see SQLCOM_RESTET).

2. Simplify the grammar for PURGE statement as suggested below.

3. I think scan_backup_log() does not have to be a method of 
Log_cvs_event_handler as it does not access any of its (private) members. In 
that case, turn it into a static helper function.

4. Do not pas my_time_t values by pointer but directly.

COMMENTARY
----------
I think the gramar rules for PURGE will be easier to follow if formulated like I 
show below. I assume that my suggestion 1 is followed.

purge:
           PURGE
           {
             LEX *lex=Lex;
             lex->type=0;
             lex->sql_command = SQLCOM_PURGE;
           }
           purge_options
           {}
         ;

purge_options:
           master_or_binary LOGS_SYM purge_option
         | BACKUP_SYM LOGS_SYM
           {
              LEX *lex= Lex;
              lex->sql_command= SQLCOM_PURGE_BUP_LOG;
              lex->type= BUP_PURGE_ALL;
           }
           optional_bup_purge_range
           {}
         | BACKUP_SYM IMAGES
           {
              LEX *lex= Lex;
              lex->sql_command = SQLCOM_PURGE_BUP_IMG
           }
           optional_bup_purge_range
           {}
         ;

optional_bup_purge_range:
         {}
         | BEFORE_SYM expr
           {
             LEX *lex= Lex;
             lex->type= BUP_PURGE_BEFORE;
             ...
           }
         | TO_SYM NUM_literal
           {
             LEX *lex= Lex;
             lex->type= BUP_PURGE_TO;
             ...
           }
         ;


DETAILS
-------
Chuck Bell wrote:

> === added file 'mysql-test/suite/backup/t/backup_logs_purge.test'
> --- a/mysql-test/suite/backup/t/backup_logs_purge.test	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/backup/t/backup_logs_purge.test	2008-10-18 22:10:41 +0000
<cut>
> +--echo Display the results.
> +SELECT backup_id FROM mysql.backup_history;
> +SELECT DISTINCT backup_id FROM mysql.backup_progress;
> +
> +#
> +# We need to sleep to allow time to pass in order to ensure the time
> +# compare method in log.cc has two different times to compare.
> +#
> +real_sleep 3;
> +
> +SET @now_time= sysdate();
> +
> +PURGE BACKUP IMAGES BEFORE @now_time;
> +
> +--error 1
> +--file_exists $MYSQLTEST_VARDIR/master-data/backup1.bak
> +--error 1
> +--file_exists $MYSQLTEST_VARDIR/master-data/backup2.bak
> +--error 1
> +--file_exists $MYSQLTEST_VARDIR/master-data/backup3.bak
> +--file_exists $MYSQLTEST_VARDIR/master-data/backup4.bak

Why the last file should exist? Does it not depend on the timing above?

> +
> +PURGE BACKUP LOGS BEFORE @now_time;
> +
> +SELECT backup_id FROM mysql.backup_history;
> +SELECT DISTINCT backup_id FROM mysql.backup_progress;
> +

I think the output of these will be non-deterministic, or?

> +PURGE BACKUP LOGS;
> +
> +#
> +# Test purge of images for locations other than backupdir.
> +#
> +--echo Perform backup
> +SET SESSION debug="d,set_backup_id";
> +
> +BACKUP DATABASE backup_logs TO '../bup_logs_dir.bak';
> +
> +SET SESSION debug="d";
> +
> +--echo Ensure backup image file went to the correct location
> +--file_exists $MYSQLTEST_VARDIR/bup_logs_dir.bak
> +
> +SELECT count(*) FROM mysql.backup_history;
> +SELECT count(*) FROM mysql.backup_progress;
> +
> +PURGE BACKUP IMAGES TO 501;
> +
> +--error 1
> +--file_exists $MYSQLTEST_VARDIR/bup_backupdir2.bak
> +
> +PURGE BACKUP LOGS TO 501;
> +
> +SELECT count(*) FROM mysql.backup_history;
> +SELECT count(*) FROM mysql.backup_progress;
> +
> +#
> +# Cleanup.
> +#
> +DROP DATABASE backup_logs;
> +--error 0,1
> +--remove_file $MYSQLTEST_VARDIR/master-data/backup1.bak
> +--error 0,1
> +--remove_file $MYSQLTEST_VARDIR/master-data/backup2.bak
> +--error 0,1
> +--remove_file $MYSQLTEST_VARDIR/master-data/backup3.bak
> +--error 0,1
> +--remove_file $MYSQLTEST_VARDIR/master-data/backup4.bak
> +--error 0,1
> +--remove_file $MYSQLTEST_VARDIR/bup_backupdir2.bak
> +
> +PURGE BACKUP LOGS;
> +

> === modified file 'sql/log.cc'
> --- a/sql/log.cc	2008-10-15 20:00:48 +0000
> +++ b/sql/log.cc	2008-10-18 22:10:41 +0000
> @@ -55,6 +55,11 @@ LOGGER logger;
>  MYSQL_BIN_LOG mysql_bin_log;
>  ulong sync_binlog_counter= 0;
>  
> +/*
> +  Backup kernel API method
> +*/
> +bool is_backup_image(THD *thd, String *path);
> +
>  static bool test_if_number(const char *str,
>  			   long *res, bool allow_wildcards);
>  static int binlog_init(void *p);
> @@ -856,6 +861,11 @@ bool Log_to_csv_event_handler::
>    if (history_data->start)
>    {
>      MYSQL_TIME time;
> +    /*
> +      Set time ahead a few hours to allow backup purge test to test
> +      PURGE BACKUP LOGS/IMAGES BEFORE command.
> +    */
> +    DBUG_EXECUTE_IF("set_start_time", history_data->start= my_time(0) +
> 100000;);

Perhaps use more specific name for this error insertion point.

>      my_tz_OFFSET0->gmt_sec_to_TIME(&time,
> (my_time_t)history_data->start);
>  
>      table->field[ET_OBH_FIELD_START_TIME]->set_notnull();
> @@ -1096,6 +1106,331 @@ err:
>    return result;
>  }
>  
> +/**
> +  Delete a row from a log table.
> +
> +  This method is used to delete a row from a log that is being
> +  accessed as a table.

Which row? The current I guess, or the one stored in tbl->record[0], or both? Is 
it required that tbl->record[0] holds the current record, or I can load 
tbl->record[0] with anything and the function will delete the corresponding row 
if it exists? Please clarify in the documentation.

> +
> +  @param[IN]  hdl   Table handler.
> +  @param[IN]  tbl   Table class.
> +
> +  @returns Result of delete operation
> +*/
> +bool Log_to_csv_event_handler::delete_log_row(handler *hdl, TABLE *tbl)
> +{
> +  bool result= 0;
> +
> +  /*
> +    Tell the handler to allow deletes for this log table
> +    by turning off log table flag.
> +  */
> +  hdl->extra(HA_EXTRA_ALLOW_LOG_DELETE);
> +  result= hdl->ha_delete_row(tbl->record[0]);
> +  hdl->extra(HA_EXTRA_MARK_AS_LOG_TABLE);
> +  return result;
> +}
> +
> +/**
> +  Delete an image specified by the table field.
> +
> +  This method attempts to delete a file using the my_delete()
> +  method. The name of the file is combined with the path stored
> +  in the backup_file_path field.
> +
> +  @param[IN]  tbl  Table class.
> +  @returns Result of my_delete() method call.
> +*/
> +bool Log_to_csv_event_handler::delete_image(TABLE *tbl)
> +{
> +  String path;
> +  String fname;
> +
> +  tbl->field[ET_OBH_FIELD_BACKUP_FILE_PATH]->val_str(&path);
> +  tbl->field[ET_OBH_FIELD_BACKUP_FILE]->val_str(&fname);
> +  path.append(fname);
> +  return (my_delete(path.c_ptr(), MYF(0)));

Does not check if file is a real backup image.

> +}
> +
> +/**
> +  Perform a table scan of the backup log tables for deleting rows.
> +
> +  This method is a helper method designed to delete rows either by ID or DATE
> +  (the table columns backup_id and start respectfully). 
> +
> +  There are several parameters used to control what is being deleted.
> +  @c log_name - is used to determine which log to process 
> +                (backup_history or backup_progress).
> +  @c backup_id - value specified on the command for deleting rows by id.

What is the semantics? Do you want to delete rows up to and including given 
backup_id? Please make it clear in the documentation.

> +  @ field_num - is used to tell the method which field to process. For ID,
> +                the fields are either ET_OBH_FIELD_BACKUP_ID or
> +                ET_OBP_FIELD_BACKUP_ID_FK.

Why passing it as a parameter? Can't it be determined within the code? If the 
intention of this parameter is to distinguish between "delete by id" and "delete 
by date", it would be more clear to have a simple boolean flag choosing one of 
the alternatives.

> +  @dtm - value specified on the command for deleting rows by date
> +  @delete_log_entries - if TRUE, process rows from the logs else delete images
> +                        specified in the backup_history log file via column 
> +                        'backup_file'.
> +  @rows - the number of rows
> +

These descriptions should be included in @param items below - no need for a 
separate list.

> +  For example, to delete all rows < backup_id for the backup_history table, 
> +  set the following:
> +    * log_name = BACKUP_HISTORY_LOG_NAME
> +    * field_num = ET_OBH_FIELD_BACKUP_ID
> +    * backup_id = id specified by the user
> +    * delete_log_entries = TRUE
> +
> +  @param[IN]  THD                 The current thread
> +  @param[IN]  log_name            Name of the log to process
> +  @param[IN]  field_num           Number of field where value is stored
> +  @param[IN]  backup_id           Value for backup id if specified
> +  @param[IN]  dtm                 Value for start (date) if specified
> +  @param[IN]  delete_log_entries  If TRUE, process logs else process images
> +  @param[IN]  delete_exact_row    If TRUE, delete only the rows = backup_id
> +  @param[OUT] num                 Number of rows affected.
> +
> +  @retval num  Number of rows affected.
> +

The @retval keyword is for documenting the return value of the function, in this 
case bool value indicating error, not the number of rows affected.

> +  @returns TRUE if error
> +*/
> +bool Log_to_csv_event_handler::scan_backup_log(THD *thd, 
> +                                               LEX_STRING log_name, 
> +                                               uint field_num, 
> +                                               ulonglong backup_id, 
> +                                               my_time_t *dtm,
> +                                               bool delete_log_entries,
> +                                               bool delete_exact_row,
> +                                               int *rows)
> +{
> +  TABLE_LIST table_list;
> +  handler *hdl;
> +  TABLE *tbl;
> +  bool res= FALSE;
> +  uint result= 0;
> +  int num_rows= 0;
> +  Open_tables_state open_tables_backup;
> +
> +  /*
> +    Setup table list for open.
> +  */
> +  bzero(&table_list, sizeof(TABLE_LIST));
> +  table_list.alias= table_list.table_name= log_name.str;
> +  table_list.table_name_length= log_name.length;
> +
> +  table_list.lock_type= TL_WRITE_CONCURRENT_INSERT;
> +
> +  table_list.db= MYSQL_SCHEMA_NAME.str;
> +  table_list.db_length= MYSQL_SCHEMA_NAME.length;
> +
> +  tbl= open_performance_schema_table(thd, &table_list,
> +                                       &open_tables_backup);
> +  hdl= tbl->file;
> +  hdl->extra(HA_EXTRA_MARK_AS_LOG_TABLE);
> +
> +  /*
> +    Begin table scan.
> +  */
> +  result= hdl->ha_rnd_init(1);
> +  result= hdl->rnd_next(tbl->record[0]);
> +  while (result != HA_ERR_END_OF_FILE)
> +  {
> +    ulonglong id= tbl->field[0]->val_int();
> +    /*
> +      Delete by id.
> +    */
> +    if ((field_num == ET_OBH_FIELD_BACKUP_ID) ||
> +        (field_num == ET_OBP_FIELD_BACKUP_ID_FK))
> +    {
> +      if (delete_exact_row)
> +      {
> +        if (delete_log_entries && (id == backup_id)) // delete log row
> +          result= delete_log_row(hdl, tbl);
> +      }
> 
> +      else if (id < backup_id)

This logic means that if delete_exact_row is FALSE then we do not delete the row 
with id=backup_id. Is it intentional?

> +      {
> +        if (delete_log_entries) // delete log row
> +        {
> +          result= delete_log_row(hdl, tbl);

If delete_log_row(...) reports error, result will be overwritten at the bottom 
of the loop and the error will be ignored. It should not. Similar problem in 
other places in this function.

> +          num_rows++;
> +        }
> +        else if (field_num == ET_OBH_FIELD_BACKUP_ID)  // delete image specified
> +        {
> +          result= delete_image(tbl);
> +          if (!result) // only count deletes that succeed
> +            num_rows++;

Why this is not executed if field_num == ET_OBP_FIELD_BACKUP_ID_FK?

> +        }
> +      }
> +    }
> +    /*
> +      Delete by date
> +    */
> +    else  
> +    {
> +      MYSQL_TIME time;      
> +      MYSQL_TIME tmp;
> +
> +      tbl->field[field_num]->get_date(&time, TIME_NO_ZERO_DATE);
> +      bzero(&tmp, sizeof(MYSQL_TIME));
> +      thd->variables.time_zone->gmt_sec_to_TIME(&tmp, *dtm);

This computation can, and thus should, be done outside the loop.

> +      if (my_time_compare(&time, &tmp) < 0)
> +      {
> +        if (delete_log_entries) // delete log row
> +        {
> +          /*
> +            When deleting by date for the backup_history table,
> +            we must also delete rows from the backup_progress table
> +            for this backup id.
> +          */

Why we don't do this when delteting by backup_id?

Please mention this behaviour in the documentation of the function.

An idea for alternative implementation: Define a trigger on backup_history table 
which deletes corresponding entries in backup_progress whenever an entry is 
deleted from backup_history.

> +          if ((my_strcasecmp(system_charset_info, log_name.str, 
> +               BACKUP_HISTORY_LOG_NAME.str) == 0) &&
> +              opt_backup_progress_log && 
> +              (log_backup_output_options & LOG_TABLE))
> +          {
> +            int r= 0;
> +
> +            scan_backup_log(thd, BACKUP_PROGRESS_LOG_NAME, 
> +              ET_OBP_FIELD_BACKUP_ID_FK, id, dtm, TRUE, TRUE, &r);
> +          }
> +          result= delete_log_row(hdl, tbl);
> +          num_rows++;
> +        }
> +        else if (field_num == ET_OBH_FIELD_START_TIME)  // delete images
> +        {
> +          result= delete_image(tbl);
> +          if (!result) // only count deletes that succeed
> +            num_rows++;
> +        }
> +      }
> +    }
> +    result= hdl->rnd_next(tbl->record[0]);
> +  }
> +  result= hdl->ha_rnd_end();
> +  *rows= num_rows;
> +  close_performance_schema_table(thd, &open_tables_backup);
> +  return result;
> +}

<cut>

> +/**
> +  Purge backup logs of rows less than backup_id passed.
> +
> +  This method walks the backup log tables deleting all rows whose
> +  backup id is less than @c backup_id passed. The parameter
> +  @c delete_log_rows is used to indicate delete applies to log rows (TRUE)
> +  or images files (FALSE).
> +
> +  @param[IN]  THD                 The current thread
> +  @param[IN]  backup_id           Value for backup id
> +  @param[IN]  delete_log_rows     If TRUE, process logs else process images
> +  @param[OUT] num                 Number of rows affected.
> +
> +  @retval num  Number of rows/images affected.
> +
> +  @returns TRUE if error
> +*/
> +bool 
> +Log_to_csv_event_handler::purge_backup_logs_before_id(THD *thd, 
> +                                                      ulonglong backup_id,
> +                                                      bool delete_log_rows,
> +                                                      int *rows)
> +{
> +  bool res= FALSE;
> +  my_time_t t= 0;
> +
> +  if (opt_backup_history_log && (log_backup_output_options &
> LOG_TABLE))
> +  { 
> +    res= scan_backup_log(thd, BACKUP_HISTORY_LOG_NAME, 
> +                         ET_OBH_FIELD_BACKUP_ID, backup_id, 
> +                         &t, delete_log_rows, FALSE, rows);
> +    if (res)
> +      goto err;
> +  }
> +  if (opt_backup_progress_log && (log_backup_output_options & LOG_TABLE)
> &&
> +      delete_log_rows)
> +  {
> +    int r= 0;   //ignore this for progress log
> +
> +    res= scan_backup_log(thd, BACKUP_PROGRESS_LOG_NAME, 
> +                         ET_OBP_FIELD_BACKUP_ID_FK, backup_id, 
> +                         &t, delete_log_rows, FALSE, &r);
> +  }
> +err:
> +  return res;
> +}
> +
> +/**
> +  Purge backup logs of rows previous to start date passed.
> +
> +  This method walks the backup log tables deleting all rows whose
> +  start column value is less than @c t passed. The parameter
> +  @c delete_log_rows is used to indicate delete applies to log rows (TRUE)
> +  or images files (FALSE).

Please make it more explicit how both backup_history and backup_progress tables 
are handled. Entries from backup_history are removed based on the date stored in 
the entry. Then for each entry deleted from backup_history, all corresponding 
entries in backup_progress are removed as well.

> +
> +  @param[IN]  THD                 The current thread
> +  @param[IN]  t                   Value for start datetime
> +  @param[IN]  delete_log_rows     If TRUE, process logs else process images
> +  @param[OUT] num                 Number of rows affected.
> +
> +  @retval num  Number of rows/images affected.
> +
> +  @returns TRUE if error
> +*/
> +bool 
> +Log_to_csv_event_handler::purge_backup_logs_before_date(THD *thd, 
> +                                                        my_time_t *t,

Why t is a pointer, not a direct value?

> +                                                        bool delete_log_rows,
> +                                                        int *rows)
> +{
> +  bool res= FALSE;
> +
> +  if (opt_backup_history_log && (log_backup_output_options &
> LOG_TABLE))
> +    res= scan_backup_log(thd, BACKUP_HISTORY_LOG_NAME, 
> +                         ET_OBH_FIELD_START_TIME, 0, 
> +                         t, delete_log_rows, FALSE, rows);
> +  return res;
> +}
> +

<cut>

> +/**
> +  Delete backup images.
> +
> +  This method deletes all backup images in the backupdir path. 
> +
> +  @param[IN]  thd   The current thread.
> +  @param[IN]  wild  Wildcard specified on the command.
> +  @param[OUT] num   The number of images deleted.
> +
> +  @retval  num  The number of images deleted.
> +
> +  @returns TRUE if error.
> +*/
> +bool LOGGER::purge_backup_imgs(THD *thd, 
> +                               char *wild, 
> +                               int *num)
> +{

Why new code here if purge_backup_logs* have an option to remove backup images?

> +  my_bool res= FALSE;
> +  DBUG_ENTER("purge_backup_images");
> +
> +  /*
> +    Delete all backup images in this folder that match the wildcard string.
> +  */
> +  int num_del= 0;
> +  uint                  i;
> +  struct st_my_dir     *dir_info;
> +  reg1 struct fileinfo *file_info;
> +  FILEINFO *file;
> +
> +  if (!(dir_info = my_dir(sys_var_backupdir.value,MYF(MY_DONT_SORT))))
> +  {		
> +    DBUG_RETURN(1);
> +  }
> +  file_info= dir_info->dir_entry;
> +  for (i=dir_info->number_off_files ; i-- ; file_info++)
> +  {
> +    /*
> +      Skip the usual suspects...and directories.
> +    */
> +    if ((file_info->name[0] == '.' &&
> +        ((file_info->name[1] == '.' && 
> +          file_info->name[2] == '\0') ||
> +          file_info->name[1] == '\0')))
> +      continue;
> +    file=dir_info->dir_entry+i;
> +    MY_STAT *st= my_stat(file_info->name, file->mystat, MYF(0));
> +    if (st && MY_S_ISDIR(st->st_mode))
> +      continue;
> +    if (wild && !wild_compare(file_info->name, wild, 1))
> +    {
> +      String full_path;
> +      full_path.copy(sys_var_backupdir.value, 
> +                     sys_var_backupdir.value_length,
> +                     system_charset_info);
> +      full_path.append(file_info->name);
> +#ifndef EMBEDDED_LIBRARY
> +      /*
> +        Check to see if file is a backup image and delete it IFF
> +        it is a backup image.
> +      */
> +      if (is_backup_image(thd, &full_path))
> +      {
> +        res= my_delete(full_path.c_ptr(), MYF(0));
> +        if (res)
> +          goto err;
> +        num_del++;
> +      }
> +#endif
> +    }
> +  }
> +  my_dirend(dir_info);
> +  *num= num_del;
> +#ifndef EMBEDDED_LIBRARY
> +err:
> +#endif
> +  DBUG_RETURN(res);
> +}
> +

<cut>

> === modified file 'sql/sql_parse.cc'
> --- a/sql/sql_parse.cc	2008-09-26 16:30:56 +0000
> +++ b/sql/sql_parse.cc	2008-10-18 22:10:41 +0000
> @@ -2098,6 +2098,147 @@ mysql_execute_command(THD *thd)
>      res = purge_master_logs_before_date(thd, (ulong)it->val_int());
>      break;
>    }
> +  /*
> +    Check log status to see if the command applies.
> +  */
> +  case SQLCOM_PURGE_BUP_LOG_DATE:
> +  case SQLCOM_PURGE_BUP_LOG_ID:
> +  case SQLCOM_PURGE_BUP_IMG_DATE:
> +  case SQLCOM_PURGE_BUP_IMG_ID:
> +  {
> +    char buff[256];
> +    int num= 0;
> +    res= 0;
> +
> +    /*
> +      If we are attempting to purge to a specified date or backup_id, we
> +      must ensure the backup history log is turned on and is
> +      being written to a table.
> +    */
> +    if (opt_backup_history_log && !(log_backup_output_options &
> LOG_TABLE))
> +    {
> +      my_error(ER_BACKUP_LOG_OUTPUT, MYF(0), ER(ER_BACKUP_LOG_OUTPUT));
> +      goto error;
> +    }
> +

Check the privileges first. If user has no SUPER privileges he should see "Not 
enough privileges" error, not anything else.

> +    if (check_global_access(thd, SUPER_ACL))
> +      goto error;
> +

<cut>

> +  case SQLCOM_PURGE_BUP_LOG:
> +  case SQLCOM_PURGE_BUP_IMG:
> +  {
> +    char buff[256];
> +    int num= 0;
> +
> +    res= 0;
> +    if (check_global_access(thd, SUPER_ACL))
> +      goto error;
> +
> +    /*
> +      We allow the other purge backup commands (except PURGE BACKUP IMAGES;)
> +      to float down the switch so that we can consolidate some of the 
> +      repetitive code.
> +    */
> +    if (lex->sql_command == SQLCOM_PURGE_BUP_LOG)        // PURGE BACKUP LOGS;
> +    {
> +      if (sys_var_backupdir.value_length > 0)
> +        res= logger.purge_backup_logs(thd);
> +      if (!res)
> +      {
> +        my_sprintf(buff, (buff, "%s %d.", ER(ER_BACKUP_LOGS_DELETED), num));
> +        my_ok(thd, num, 0, buff);

I don't see where num is set to anything else than the initial 0...

> +      }
> +      break;
> +    }
> +    else        // PURGE BACKUP IMAGES;
> +    {
> +      /*
> +        Check to see if user is attempting a delete when backupdir is
> +        the same as datadir and a global wildcard is specified.
> +      */
> +      if ((my_strcasecmp(system_charset_info, sys_var_backupdir.value,
> +          mysql_real_data_home) == 0) && 
> +          (my_strcasecmp(system_charset_info, lex->backup_log_wild, "%") == 0))
> +      {
> +        my_error(ER_BACKUP_PURGE_NOT_ALLOWED, MYF(0));
> +        DBUG_RETURN(-1);
> +      }
> +      if (sys_var_backupdir.value_length > 0)
> +        res= logger.purge_backup_imgs(thd, lex->backup_log_wild, &num);
> +      if (!res)
> +      {
> +        my_sprintf(buff, (buff, "%s %d.", ER(ER_BACKUP_IMAGES_DELETED), num));
> +        my_ok(thd, 0, 0, buff);
> +      }
> +      break;
> +    }
> +
> +    if (res)
> +      goto error;
> +
> +    break;
> +  }
>  #endif
>    case SQLCOM_SHOW_WARNS:
>    {
>

<cut>

> === modified file 'storage/csv/ha_tina.cc'
> --- a/storage/csv/ha_tina.cc	2008-08-15 18:34:18 +0000
> +++ b/storage/csv/ha_tina.cc	2008-10-18 22:10:41 +0000
> @@ -80,7 +80,6 @@ static handler *tina_create_handler(hand
>                                      TABLE_SHARE *table, 
>                                      MEM_ROOT *mem_root);
>  
> -
>  /*****************************************************************************
>   ** TINA tables
>   *****************************************************************************/
> @@ -995,7 +994,11 @@ int ha_tina::delete_row(const uchar * bu
>    share->rows_recorded--;
>    pthread_mutex_unlock(&share->mutex);
>  
> -  /* DELETE should never happen on the log table */
> +  /* 
> +     DELETE should never happen on the log table
> +     UNLESS it is a backup log table in which case extra() should be
> +     called with HA_EXTRA_ALLOW_LOG_DELETE
> +  */
>    DBUG_ASSERT(!share->is_log_table);
>  
>    DBUG_RETURN(0);
> @@ -1169,6 +1172,12 @@ int ha_tina::extra(enum ha_extra_functio
>   {
>     pthread_mutex_lock(&share->mutex);
>     share->is_log_table= TRUE;
> +   pthread_mutex_unlock(&share->mutex);
> + }
> + else if (operation == HA_EXTRA_ALLOW_LOG_DELETE)
> + {
> +   pthread_mutex_lock(&share->mutex);
> +   share->is_log_table= FALSE;

This looks dangerous - although the backup log table *is* a log table, you set 
share->is_log_table to FALSE. This is violating the semantics of is_log_table flag.

I would rather introduce a new flag, allowing deletes of rows even for log 
tables. This flag would be set if HA_EXTRA_ALLOW_LOG_DELETE operation is executed.

>     pthread_mutex_unlock(&share->mutex);
>   }
>    DBUG_RETURN(0);
> 
>

Rafal