Hi Chuck,
Thank you for the review. I have prepared new patch
<http://lists.mysql.com/commits/45753> correcting the errors you spotted. See
also my replies below.
Rafal
Chuck Bell wrote:
> Hi Rafal,
>
> See comments below.
>
> Chuck
>
>> +uchar* Backup_info::Ts_hash_node::get_key(const uchar *record,
>> + size_t *key_length,
>> + my_bool) {
>> + Ts_hash_node *n= (Ts_hash_node*)record;
>> +
>> + // ts_hash entries are indexed by tablespace name.
>
> This is ok for this release, but in the future it is possible for 2
> tablespaces to have the same name but different attributes -- e.g., one used
> in NDB and another used in Falcon.
>
Right, the code is written assuming that tablespace names are unique. If it is
not the case then few things would have to be changed, including format of the
backup stream. I think that in that case also si_objects API should be changed.
E.g., when creating a tablespace object with materialzie_tablespace(), both
tablespace and storage engine name should be provided, not just tablespace name
as it is now.
The idea here would be that si_objects methods operating on various objects get
full object identity as input. For example the matarialize_table() method takes
both table and database name because only both these names identify a table
uniquely. In general, a materialize method for a given type of object accepts as
input:
1. Identity of the object to be created,
2. A string containing serialization of that object,
3. Version number of the serialziation string.
In case of a database, the identity is just database name, in case of table, it
is (db name, table name) pair and in case of tablespace it is now just
tablespace name but perhaps should be changed to (storage engine, tablespace
name) pair.
>> diff -Nrup a/sql/backup/image_info.h b/sql/backup/image_info.h
>> --- a/sql/backup/image_info.h 2008-03-21 09:43:43 +01:00
>> +++ b/sql/backup/image_info.h 2008-04-08 17:14:07 +02:00
>> @@ -539,6 +575,35 @@ inline
>> Image_info::Iterator::~Iterator()
>> {}
>>
>> +
>> +/**
>> + Used to iterate over all tablespaces stored in a backup image.
>> +
>> + @note Backup stram library infers position of each
>> tablespace in the catalogue
>
> stram -> stream
>
Corrected.
>> + from the order in which they are enumerated by this
>> iterator. Therefore it
>> + is important that databases are listed in correct order -
>> first database at
>> + position 0, then at position 1 and so on.
>
> Did you mean to say tablespace and not database?
>
Yes, corrected.
>> @@ -625,6 +690,86 @@ Image_info::Dbobj_iterator::Dbobj_iterat
>> {}
>>
>>
>> +class Image_info::Global_iterator
>> + : public Image_info::Iterator
>> +{
>> + /**
>> + Indicates whether tablespaces or databases are being
>> currently enumearated.
>
> Enumerated
>
Corrected.
>> +inline
>> +bool
>> +Image_info::Global_iterator::next()
>> +{
>> + if (mode == DONE)
>> + return FALSE;
>> +
>> + DBUG_ASSERT(m_it);
>> +
>> + // get next object from the current iterator
>> + m_obj= (*m_it)++;
>> +
>> + if (m_obj)
>> + return TRUE;
>> +
>> + /*
>> + If the current iterator has finished (m_obj == NULL)
>> then, depending on
>> + the mode, either switch to the next iterator or mark end
>> of the sequence.
>> + */
>> +
>> + delete m_it;
>> +
>> + switch (mode) {
>> +
>> + case TABLESPACES:
>> +
>> + mode= DATABASES;
>> + m_it= m_info.get_dbs();
>> + m_obj= (*m_it)++;
>> + return m_obj != NULL;
>
> Suggestion: move 'mode=' down and/or place a comment explaining why mode is
> being set to next case.
>
I added comment for this case:
// We have finished enumerating tablespaces, move on to databases.
>> +
>> + case DATABASES:
>> +
>> + mode= DONE;
>> +
>> + case DONE:
>> +
>> + break;
>> + }
>> +
>> + return FALSE;
>> +}
>> +
>> diff -Nrup a/sql/backup/stream_v1.h b/sql/backup/stream_v1.h
>> --- a/sql/backup/stream_v1.h 2008-04-04 12:16:25 +02:00
>> +++ b/sql/backup/stream_v1.h 2008-04-08 17:14:07 +02:00
>> @@ -239,6 +239,16 @@ struct st_bstream_item_info
>> };
>>
>> /**
>> + Describes tablespace item.
>> +
>> + Currently no data specific to tablespace items is used.
>
> ...are used.
>
Corrected.
>> +*/
>> +struct st_bstream_ts_info
>> +{
>> + struct st_bstream_item_info base;
>> +};
>> +
>> +/**
>> Describes database item.
>>
>> Currently no data specific to database items is used.
>> diff -Nrup a/sql/share/errmsg.txt b/sql/share/errmsg.txt
>> --- a/sql/share/errmsg.txt 2008-03-21 10:05:36 +01:00
>> +++ b/sql/share/errmsg.txt 2008-04-08 17:14:07 +02:00
>> @@ -6332,3 +6332,14 @@ ER_BACKUP_LIST_DB_TRIGGERS
>>
>> ER_BACKUP_LOG_WRITE_ERROR
>> eng "Can't write to the online backup progress log %-.64s."
>> +
>> +ER_BACKUP_CAT_ENUM
>> + eng "Could not access the contents of the backup
>> catalog when writing backup image preamble"
>> +ER_BACKUP_CANT_RESTORE_TS
>> + eng "Could not create tablespace %-.64s needed
>> by tables being restored."
>> +ER_BACKUP_TS_CHANGE
>> + eng "Tablespace %-.64s needed by tables being
>> restored has changed on the server. The original definition
>> of the required tablespace is '%-.256s'."
>
> I think this error should display both the original and the tablespace
> definition in the backup image so users can see at a glance what has
> changed. Please add the extra statement.
>
The current version is:
ER_BACKUP_TS_CHANGE
eng "Tablespace `%-.64s` needed by tables being restored has changed on
the server. The original definition of the required tablespace is '%-.256s'
while the same tablespace is defined on the server as '%-.256s'"
Rafal
