Rafal,
Since this is a very large patch, it would be inefficient for me to make my
comments interdelinia. I have prepared the following lists instead.
I would like you to respond to my comments. I think I should also see new
patches with the corrections. I have one overriding request: please make all
patches compatible with the current backup tree so that I can compile and
test the code.
Lastly, please extend the hours on the worklog. It is not complete yet the
estimate hours have expired.
Note: This review is for patch http://lists.mysql.com/commits/42107 only.
Chuck
PS This was a monster patch. Please excuse any of my terse statements.
Nothing was meant as a critique, only criticism in the teamwork sense. This
goes for the other patches too. ;)
Minor changes needed (may be placed as low priority)
----------------------------------------------------
* Wrong worklog specified in the patch comments.
* Several misspellings in patch comments, e.g. "...now the context calss
provides"
* Several misspellings in the code comments.
* Why was this line changed?
- bzero(m_snap, sizeof(m_snap));
+ bzero(m_snap,sizeof(m_snap));
* Please fix spacing issues on assignments (there are a lot of these).
+ virtual bool is_valid() =0;
* Wording issues in comments.
+ Instances of this class store name of the object and other relevant
+ information. For each type of objects a subclass of this class is derived
+ which is specialized in storing information about that kind of objects.
...reads strangely. It should read:
+ Instances of this class store the name of the object and other relevant
+ information. For each type of object, a subclass is derived which is
+ specialized in storing specific information about an object type.
In this particular comment, what is 'other relevant information' and why
isn't this information in the subclasses?
* Error in comments: obs:;Obj should be obs::Obj.
* More spacing issues.
+ if (info.snap_count()==0 || info.table_count()==0) // nothing to backup
* Non-standard comments found. Also, mixed terms -- 'start' should 'end',
'begin' should 'done'. Please repair.
+ /**** VP creation (start) ********************************************/
+ /**** VP creation (done) ********************************************/
* Watch references to methods, attributes, classes in comments, e.g.,
Backup_crate_ctx.
* Please remove references to online backup being a plugin until such time
as it is a plugin.
+ @note This function is called when server shuts down its plugins.
* There are several lines of code that exceed the 80 character limit per
line. Please fix these so that they do not exceed 80 characters per line.
+ Image_info::Table *tbl= info->add_table(*db,name_str, *snap,
item->pos); // reports error
* Format of doxygen comment incorrect.
+/** @file
Changes needed (med/high priority)
----------------------------------
* Please add a note that your OStream and IStream are not to be confused
with ostream and istream respectfully. Also, 'OStream' violates our coding
guidelines. It should be 'Ostream'.
* Watch out for name misuse. The comment 'Path to the file where backup
image is located' should read 'Path to where the backup image file is
located'.
* The second sentence in this comment block doesn't make any sense. Should
it read ... 'is @c 0...'? I would request that 'no' be changed to something
more meaningful ('num' perhaps) -- 'no' is not a normal abbreviation for
number -- 'no.' is the correct abbreviation. Likewise, every m_no should be
changed.
+ The snapshot should be non-empty, that is contain data of at least one
table.
+ Snapshot is added to the list of snapshots used in the image and a number
is
+ assigned to it. This number is stored in @c snap.m_no. If snapshot's
number
+ is @c no then pointer to a corresponding @c Snapshot_info object is
stored in
+ @c m_snap[no-1].
* What is this? This cannot be correct. Table names may start with 'b' in
any database. Please remove this comment.
+ FIXME: add_table should generate error if table name starts with 'b' (?)
* Please make this doxygen format (kernel.cc):
+/*
+ How to use backup kernel API to perform backup and restore operations
+ =====================================================================
[...]
* Although I do not like the reorganization of the code WRT calls to
progress tables functions and DDL operations, I must request that you change
the DDL calls to use the new si_objects abstractions for DDL blocker.
Questions
---------
* Why were the backup progress calls moved to the logger? Does the logger
really log now (ref: comments/dialog about error reporting)? I am confused.
* What does this mean? "When writing messages, cover the case when the type
of the operation (backup or restore) has not been decided yet."
* What is this? "Use base POD structure fd_stream..."
* Does the CREATED state replace the READY and INIT states for the
Backup_info class?
* Why implement this function? Allow it to be virtual so that we do not need
to define a 'base' operation. The compiler is our friend (sometimes). ;)
+const char* Snapshot_info::name() const
+{ return "<Unknown>"; }
* You plan to replace all of the ER_BACKUP_UNKNOWN_ERROR errors in the error
reporting II worklog, right?
* Are any of your reorganizations of the code going to impede making this
code multithreaded? If so, please consider either fixing the problem now or
annotating the code itself with comments on how to mitigate any issues you
see.
* This didn't make sense to me the first time. Why 'a'? What is the
significance? According to the comment, all dbs that start with 'a' are an
error? If this is wrong, please change comment to further explain what is
happening here.
/*
If error debugging is switched on (see debug.h) then I_S.TABLES access
error will be triggered when backing up database whose name starts with
'a'.
*/
- TEST_ERROR_IF(dbi.name().ptr()[0]=='a');
+ TEST_ERROR_IF(db.name().ptr()[0]=='a');
* Are we certain of the order in which the default and consistent snapshot
drivers will be searched for a given table? Consistent snapshot should
always be 'asked' first.
* Will the code continue to pollute the output to the console? If yes,
please change all such statements to DBUG_PRINT calls or possibly some other
logging method.
Concerns (require addressing as condition of approval if given)
---------------------------------------------------------------
* Why change from uint to ulong for Table_ref? Why is UINT too small? How
many tables are you expecting to have to backup?
* Why are there calls like this? Isn't this adding unnecessary complexity?
Are they transformations? If so why do we need them? If not, I recommend
removing these calls.
inline const char* se_name(storage_engine_ref se) { return se->name.str; }
* I really don't like the continued use of the local version of the string
class. Why can we not use the calls in the strings library? Recommend using
the strings library and removing the duplication.
* I have confirmed in other works that the TABLE_LIST is freed when the
thread is destroyed. In fact, I have had issues when I freed the table list
manually. I recommend removing the code to free the TABLE_LIST.
* Inconsistent use of data type for indexes. See below. Please choose one
and stick with it -- ulong seems too large but probably the better choice.
In this case, add_db uses ulong but get_db uses uint. This could be the
source of a unique bug. Indeed, later uint pos; is one of the attributes of
the db iterator. Likewise, table_count is uint.
+ Db* get_db(uint pos) const;
+ Table* get_table(uint snap_no, ulong pos) const;
* Please do not use hard coded values in code. Please change all such
references to #defines or other tunable mechanisms. Also, why 256?
+ Snapshot_info *m_snap[256];
+ if (info.snap_count() > 256)
* We are mixing names again! We decided to call them 'drivers' but the patch
changes them back to 'engines'. Why? Please remove this and related changes.
- NATIVE_SNAPSHOT= BI_NATIVE, ///< snapshot created by native backup
driver
- DEFAULT_SNAPSHOT= BI_DEFAULT, ///< snapshot created by built-in,
blocking driver
- CS_SNAPSHOT= BI_CS ///< snapshot created by CS backup driver
+ NATIVE_SNAPSHOT= BI_NATIVE, ///< snapshot created by native backup
engine
+ DEFAULT_SNAPSHOT= BI_DEFAULT, ///< snapshot created by built-in,
blocking backup engine
+ CS_SNAPSHOT= BI_CS ///< snapshot created by built-in CS
backup engine
* Here is another example of using the wrong term. Shouldn't this be
'find_backup_driver'?
+ the best available backup engine for the table using @c
find_backup_engine()
* I would like to see this method changed to eliminate duplicates. This
would solve BUG#34480.
+Image_info::add_db(const String &db_name, ulong pos)
* I don't understand why the backup progress methods were abstracted and
pushed to inner classes. Can you please explain the rationale for this? To
me, this complicates the code as we would have to search for every
occurrence of the calls to work on them. Having them in the upper-level
method makes much more sense to me.
* The SHOW ARCHIVE command is still in the parser and the big switch, but
your patch sends an error to the user. I think this is incorrect (this isn't
a new problem BTW). I think we should remove SHOW ARCHIVE from the parser
and the big switch. We can always go back to get the code that did this
later when/if we implement the command. Please remove the command and code
for SHOW ARCHIVE.
* Remove conditional error statements from constructors. Constructors should
never have code that can produce errors that need to be acted on. Besides,
constructors should be fast. Example:
+Backup_restore_ctx::Backup_restore_ctx(THD *thd):
+ m_state(CREATED), m_thd(thd), m_thd_options(thd->options),
+ m_error(0), m_remove_loc(FALSE), m_stream(NULL), m_catalog(NULL)
+{
+ /*
+ Check for progress tables.
+ */
+ if (check_ob_progress_tables(thd))
+ m_error= ER_BACKUP_PROGRESS_TABLES;
+}
In this case, the call to the progress tables should occur prior to creating
the instance of the class. If this is not possible, please move this code to
the prepare() method. The code that checks for the progress tables is not
fast and can potentially take a long time (it performs and open and lock
table command).
* Please restore this comment (or something like it). I think this is a very
important implementation detail that should not be overlooked.
- Note: The block_ddl() call must occur before the information_schema
- is read so that any new tables (e.g. CREATE in progress) can
- be counted. Waiting until after this step caused backup to
- skip new or dropped tables.
* Please combine assertions into one statement. Debugging code with multiple
assertions together like the following can be annoying to the extreme when
debugging the code on Windows (not your fault -- it's a platform issue).
+ DBUG_ASSERT(is_valid());
+ DBUG_ASSERT(m_state == PREPARED_FOR_BACKUP);
+ DBUG_ASSERT(m_thd);
+ DBUG_ASSERT(m_stream);
+ DBUG_ASSERT(m_catalog);
* Are the DDL blocker calls on restore setup the way the original code was?
Specifically, is the DDL blocker's exception code turned on? If not, restore
will fail and block itself. Sorry, I cannot tell from the patch -- too much
code reorganization.
* This is another hard coded value that is sure to bite us later. Especially
if a third party vendor adds their own driver for their engine or an
alternative for one of ours. Please change this to a #define or something.
Why eight? Why not 11 or 20 or ...?
+ native_snapshots(8)
* Here also.
+ char buf[255]; // buffer for llstr
* I think I see why you have arbitrary starts with 'a' 'b' checks. Please
change the comments for all of these to be certain they are clearly
identified as debugging statements. My recommendation is to either remove
them completely or guard them with a DEBUG_EXECUTE_IF() macro.
* I really don't like some of the abstractions. For example, these calls to
the progress reporting function do nothing more than make the calls a class.
If you want a class for the progress reporting, please issue requests to
change the progress code and do not add artificial organization to the code.
I really must insist on this as it needlessly complicates the already
complex code.
+void Logger::report_state(enum_backup_state state)
+{
+ DBUG_ASSERT(m_state == RUNNING);
+
+ // TODO: info about state change in the log?
+ report_ob_state(m_op_id, state);
+}
