Hi Chuck,
My comments below.
cbell@stripped wrote:
> Below is the list of changes that have just been committed into a local
> 5.2 repository of cbell. When cbell does a push these changes will
> be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>
> ChangeSet@stripped, 2007-10-23 13:23:36-04:00, cbell@mysql_cab_desk. +3 -0
> WL#3576 : Online Backup: Point in time recovery (Sync w/ binlog & rpl)
>
> This patch saves the binlog information and datetime of the validation point.
>
> sql/backup/archive.h@stripped, 2007-10-23 13:23:26-04:00, cbell@mysql_cab_desk. +16 -0
> WL#3576 : Online Backup: Point in time recovery (Sync w/ binlog & rpl)
>
> Adds new structure to archive info for saving the binlog information. Also adds
> datetime of start and end of backup or restore.
>
> sql/backup/data_backup.cc@stripped, 2007-10-23 13:23:27-04:00, cbell@mysql_cab_desk.
> +25 -0
> WL#3576 : Online Backup: Point in time recovery (Sync w/ binlog & rpl)
>
> Saves the binlog information during the synchronization phase after the calls
> to lock() and before the first call to unlock() in the kernel.
>
> sql/backup/sql_backup.cc@stripped, 2007-10-23 13:23:28-04:00, cbell@mysql_cab_desk. +26
> -0
> WL#3576 : Online Backup: Point in time recovery (Sync w/ binlog & rpl)
>
> Saves the start and end of the backup and restore operation.
>
> diff -Nrup a/sql/backup/archive.h b/sql/backup/archive.h
> --- a/sql/backup/archive.h 2007-08-20 04:37:06 -04:00
> +++ b/sql/backup/archive.h 2007-10-23 13:23:26 -04:00
> @@ -35,6 +35,18 @@ class Image_info;
> typedef Image_info* Img_list[MAX_IMAGES]; ///< List (vector) of image
> descriptions.
>
> /**
> + Structure to hold information about binary log.
> +*/
> +typedef struct st_binlog_info
> +{
> + char binlog_file_name[FN_REFLEN]; ///< the file name
> + my_off_t index_file_offset; ///< offset into file
> + my_off_t index_file_start_offset; ///< starting point of offset
Why do you store there offsets? Why are they representing? Is this information
important for a user?
I tend to think that all information needed to specify position of an event in a
binlog is name of the binlog file and position of the first byte of that event
in the file. Actually, it could be also useful to store position of the first
event in event's event group.
> + my_off_t position; ///< binlog position
> + struct tm vp_time; ///< time of validation point
> +} BINLOG_INFO;
In all backup kernel code the convention used to define such types is more like
this (which is more in C++ style):
struct Binlog_info
{
...
};
Maybe we can stick to it to make the code look more consistent.
> +
> +/**
> Describes contents of a backup archive.
>
> This class stores a catalogue of a backup archive, that is, description of
> @@ -69,6 +81,10 @@ class Archive_info
> size_t header_size; ///< size of archive's header (after reading or writing
> an archive)
> size_t meta_size; ///< size of archive's meta-data (after reading or
> writing an archive)
> size_t data_size; ///< size of archive's table data images (after reading
> or writing an archive)
> +
> + BINLOG_INFO *binlog_information; ///< stores binlog information for PTR
Why pointer? We can have the structure here:
BINLOG_INFO binlog_information;
> + struct tm start_time; ///< the start datetime of the backup
> + struct tm end_time; ///< the end datetime of the backup
I suggest to move vp_time member here, outside the BINLOG_INFO structure. After
all, vp_time makes sense even if there is no binlog. This is a piece of
information which is independent from the information about binlog position and
thus should not be stored inside BINLOG_INFO structure.
>
> // Classes representing various types of meta-data items.
>
> diff -Nrup a/sql/backup/data_backup.cc b/sql/backup/data_backup.cc
> --- a/sql/backup/data_backup.cc 2007-09-11 05:29:29 -04:00
> +++ b/sql/backup/data_backup.cc 2007-10-23 13:23:27 -04:00
> @@ -534,6 +534,31 @@ int write_table_data(THD*, Backup_info &
> if (sch.lock())
> goto error;
>
> + /*
> + Save binlog information for point in time recovery on restore.
> + */
> + if (mysql_bin_log.is_open())
> + {
> + LOG_INFO li;
> + BINLOG_INFO binlog_data;
> + mysql_bin_log.get_current_log(&li);
> + binlog_data.position= li.pos;
> + memcpy(binlog_data.binlog_file_name, li.log_file_name,
> strlen(li.log_file_name));
> + binlog_data.index_file_offset= li.index_file_offset;
> + binlog_data.index_file_start_offset= li.index_file_start_offset;
> +
> + /*
> + Save VP creation time.
> + */
This should be done even if no binlog is in use.
> + time_t skr= my_time(0);
> + gmtime_r(&skr, &binlog_data.vp_time);
> +
> + /*
> + Save information to archive image data.
> + */
> + info.binlog_information= &binlog_data;
This is bad - you can't store a pointer to a temporary object in the member like
this. When someone wants to access this structure later, the address will be
invalid...
Instead, make info.binlog_information a structure (not pointer to it) and fill
it with data here.
> + }
> +
> BACKUP_SYNC("data_unlock");
> if (sch.unlock())
> goto error;
> diff -Nrup a/sql/backup/sql_backup.cc b/sql/backup/sql_backup.cc
> --- a/sql/backup/sql_backup.cc 2007-10-17 16:20:36 -04:00
> +++ b/sql/backup/sql_backup.cc 2007-10-23 13:23:28 -04:00
> @@ -70,6 +70,8 @@ bool test_error_flag= FALSE;
> int
> execute_backup_command(THD *thd, LEX *lex)
> {
> + time_t skr;
> +
> DBUG_ENTER("execute_backup_command");
> DBUG_ASSERT(thd && lex);
>
> @@ -118,6 +120,12 @@ execute_backup_command(THD *thd, LEX *le
> {
> info.report_error(backup::log_level::INFO,ER_BACKUP_RESTORE_START);
>
> + /*
> + Save starting datetime of restore.
> + */
> + skr= my_time(0);
> + gmtime_r(&skr, &info.start_time);
> +
> // TODO: freeze all DDL operations here
>
> info.save_errors();
> @@ -140,6 +148,12 @@ execute_backup_command(THD *thd, LEX *le
> send_summary(thd,info);
> }
>
> + /*
> + Save ending datetime of restore.
> + */
> + skr= my_time(0);
> + gmtime_r(&skr, &info.end_time);
> +
> // TODO: unfreeze DDL here
> }
> } // if (!stream)
> @@ -178,6 +192,12 @@ execute_backup_command(THD *thd, LEX *le
>
> // TODO: freeze all DDL operations here
>
> + /*
> + Save starting datetime of backup.
> + */
> + skr= my_time(0);
> + gmtime_r(&skr, &info.start_time);
> +
> info.save_errors();
>
> if (lex->db_list.is_empty())
> @@ -211,6 +231,12 @@ execute_backup_command(THD *thd, LEX *le
> info.report_error(backup::log_level::INFO,ER_BACKUP_BACKUP_DONE);
> send_summary(thd,info);
> }
> +
> + /*
> + Save ending datetime of backup.
> + */
> + skr= my_time(0);
> + gmtime_r(&skr, &info.end_time);
>
> // TODO: unfreeze DDL here
> } // if (!stream)
>
>
I think start/end time should be saved only during backup operation. During
restore the info.start_time and info.end_time members will be filled with the
values stored in the backup image.
Rafal
