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

[Rafal Somla]

STATUS
------
Patch approved. One minor fix in the test requested and few suggestions made. 
Implementing the fix and suggestions (if any) does not require additional review.

REQUEST
-------
Fix comment/code to match each other in the test, at the place indicated below.

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

1. In the test, refer to backupdir using a trick "let $bdir=`SELECT @backupdir`" 
(or equivalent) instead if using $MYSQLTEST_VARDIR and similar. Thus avoid 
making assumption that @backupdir==@datadir, or that @backupdir sits inside 
$MYSQLTEST_VARDIR (see below).

2. Don't extend LEX structure with backup_id member. Use the value_list instead, 
the same as it is done with the date for "BEFORE <date>" variant.

3. Don't mention backup log tables in the comment inside 
ha_tina::delete_row(...). See inlined suggestion for how to reformulate the comment.

DETAILS
-------
Chuck Bell wrote:
> #At file:///C:/source/bzr/mysql-6.0-bug-33364/
> 
>  2717 Chuck Bell	2008-10-24
>       BUG#33364 Online Backup: Purge statement missing 
>       
>       This patch adds the following commands:
>       
>       PURGE BACKUP LOGS;
>       PURGE BACKUP LOGS TO <id>;
>       PURGE BACKUP LOGS BEFORE <datetime>;
> added:
>   mysql-test/suite/backup/r/backup_logs_purge.result
>   mysql-test/suite/backup/t/backup_logs_purge.test
> modified:
>   include/my_base.h
>   sql/backup/kernel.cc
>   sql/log.cc
>   sql/log.h
>   sql/mysqld.cc
>   sql/share/errmsg.txt
>   sql/sql_lex.h
>   sql/sql_parse.cc
>   sql/sql_yacc.yy
>   storage/csv/ha_tina.cc
>   storage/csv/ha_tina.h
> 
(cut)
> === 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-24 21:01:03 +0000
(cut)
> +
> +--file_exists $MYSQLTEST_VARDIR/master-data/backup1.bak
> +--file_exists $MYSQLTEST_VARDIR/master-data/backup2.bak
> +--file_exists $MYSQLTEST_VARDIR/master-data/backup3.bak
> +--file_exists $MYSQLTEST_VARDIR/master-data/backup4.bak
> +

Suggestion: Instead of using the assumption that @backupdir==@datadir and 
@datadir is $MYSQLTEST_VARDIR/master-data, do the following:

let $bdir=`SELECT @backupdir`;
...
--file_exists $bdir/backup1.bak

> +--echo Display results from backup logs
> +SELECT backup_id FROM mysql.backup_history;
> +SELECT DISTINCT backup_id FROM mysql.backup_progress;
> +SELECT count(*) FROM mysql.backup_history;
> +SELECT count(*) FROM mysql.backup_progress;
> +
> +--echo Purge all backup log data prior to id = 504.
> +PURGE BACKUP LOGS TO 502;

REQUEST: To 502 or 504? - please fix one of them.

(cut)
> +
> +--echo Ensure backup image file went to the correct location
> +--file_exists $MYSQLTEST_VARDIR/bup_logs_dir.bak
> +

If the suggestion with $bir is followed, here I would use:

--file_exists $bdir/../bup_logs_dir.bak

and thus not assume that backupdir sits inside $MYSQLTEST_VARDIR.

(cut)
> === modified file 'sql/log.cc'
> --- a/sql/log.cc	2008-10-21 10:44:29 +0000
> +++ b/sql/log.cc	2008-10-24 21:01:03 +0000
(cut)
> @@ -1096,6 +1101,319 @@ err:
>    return result;
>  }
>  
> +/**
> +  Delete the current row from a log table.
> +
> +  This method is used to delete a row from a log that is being
> +  accessed as a table. The row to be deleted is the current
> +  row that the handler is pointing to via tbl->record[0].
> +
> +  Note: The handler must be positioned correctly and the
> +        tbl->record[0] must be fetched by the appropriate
> +        method (e.g., hdl->next()). T

typo: 'T' at the end of the note.

(cut)
> === modified file 'sql/sql_lex.h'
> --- a/sql/sql_lex.h	2008-10-07 22:05:23 +0000
> +++ b/sql/sql_lex.h	2008-10-24 21:01:03 +0000
(cut)
> @@ -1529,6 +1529,7 @@ struct LEX: public Query_tables_list
>    LEX_STRING backup_dir;				/* For RESTORE/BACKUP */
>    bool backup_compression;
>    char* to_log;                                 /* For PURGE MASTER LOGS TO */
> +  ulonglong backup_id;     /* For PURGE BACKUP LOGS */

Suggestion: Instead of extending LEX structure, store backup_id in the 
value_list, the same as it is done for the BEFORE <data> parameter.

(cut)
> === modified file 'sql/sql_yacc.yy'
> --- a/sql/sql_yacc.yy	2008-10-02 14:03:57 +0000
> +++ b/sql/sql_yacc.yy	2008-10-24 21:01:03 +0000
> @@ -10813,10 +10813,32 @@ purge:
>              lex->type=0;
>              lex->sql_command = SQLCOM_PURGE;
>            }
> -          purge_options
> -          {}
> -        ;
> +          purge_options {}
> +          | PURGE BACKUP_SYM LOGS_SYM 
> +          {
> +            LEX *lex=Lex;
> +            lex->type=TYPE_ENUM_PURGE_BACKUP_LOGS;
> +            lex->sql_command = SQLCOM_PURGE_BUP_LOG;
> +          }
> +          purge_bup_log_option {}
> +          ; 
>  
> +purge_bup_log_option:
> +          {}
> +          | TO_SYM NUM_literal
> +          {
> +            LEX *lex= Lex;
> +            lex->backup_id= (ulonglong)$2->val_int(); 

Suggestion: do the same as in case of BEFORE <date>, that is, store the 
backup_id inside lext->value_list and then there is no need to extend the LEX 
structure.

(cut)
> === modified file 'storage/csv/ha_tina.cc'
> --- a/storage/csv/ha_tina.cc	2008-08-23 00:18:35 +0000
> +++ b/storage/csv/ha_tina.cc	2008-10-24 21:01:03 +0000
(cut)
> @@ -995,8 +995,13 @@ int ha_tina::delete_row(const uchar * bu
>    share->rows_recorded--;
>    pthread_mutex_unlock(&share->mutex);
>  
> -  /* DELETE should never happen on the log table */
> -  DBUG_ASSERT(!share->is_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 then reset with 
> +     HA_EXTRA_MARK_AS_LOG_TABLE.   

I suggest not to mention backup log tables in this comment. You can simply say:

   DELETE should never happen in the log tables
   UNLESS extra() has been called with HA_EXTRA_ALLOW_LOG_DELETE which sets
   allow_log_delete flag. The flag can be reset with HA_EXTRA_MARK_AS_LOG_TABLE.


Rafal