Hi Oystein,
Thank you for a very careful and detailed review. See my comments and
explanations below and expect an updated patch from me soon.
Øystein Grøvlen wrote:
> STATUS
> ======
>
> While patch is generally good and provides a good clean-up of error
> reporting, some error reports are now less informative or less
> specific. I request that those issues are looked into.
>
>
> REQUESTS
> ========
>
> 1. Some error reports are now less informative than previously. (See
> [2], [7]). I think that should be fixed.
I'd rather not deal with that issue in this patch. True that it changes some
error messages, but I think it is not as bad as you claim. See my explanations
below.
>
> 2. Fix long lines: [6], [13], [19].
Will do.
>
> COMMENTARY
> ==========
>
> 1. A lot of the error messages used for backup have very little value
> for a user and might as well have been replaced by "Internal error
> during BACKUP/RESTORE". What use is messages like these to the
> user:
>
> - "Error when preparing for backup operation"
> - "Can't enumerate server tables"
> - "Can't read backup archive preamble"
> - "Error when reading summary section of backup image"
> - "Can't create %-.64s backup driver"
> - "Could not create the validity point"
>
> Error messages should give useful information to user, not reflect
> the internal design/implementation of the software!
Sure, but this is not the scope of this patch. This patch aims at ensuring that
the user sees the firs error reported, as is the case for all other SQL
statements. Improving overall quality of error reporting was the task of WL#4384
(Online backup: Revise error reporting), at least before it was redefined to do
something simpler.
>
> 2. Changes in this patch (e.g., [5]) illustrate that making
> Backup_restore_ctx a sub-class of Logger was not a good design
> decision. Inheritance should only be used for is-a relationships.
> I do not think it makes sense to say that a context "is a" logger.
> However, a logger exist for a given context. Hence, encapsulation
> should be used instead.
>
I can agree that this design is not perfect. However, there was a reason behind
such decision. The context object encapsulates all the context needed to perform
backup/restore operations and offers methods for executing related actions. One
of such actions is reporting errors to the user (in the context of
backup/restore opearation). Thus the context object can be used to log messages
to a user and hence, in this sense, it "is a" logger. Although, I agree that one
can look at this from different perspectives.
>
> SUGGESTIONS
> ===========
>
> 1. Fix typos: [1], [4]
OK.
>
> 2. Skip masking of error numbers in SHOW WARNING [3].
I'd rather keep it as it is - see below.
>
> 3. Change Backup_info(Backup_restore_ctx&) to
> Backup_info(Logger&, THD*) [5].
OK. The same will go for Restore_info ctor. While at this, I'll make the ctros
private, because Backup/Restore_info instances are not supposed to be created
directly but only via Backup_restore_ctx::prepare_for_backup/restore() methods.
>
> 4. Try to give a more specific error message than
> BACKUP_VP_FAILED. [8]
I think it is not possible in the scope of this patch - see below.
>
> 5. Define a constant to be used for drv and active arrays. [9]
Ok, can do. This would be used in few more places as well.
>
> 6. execute_backup_command: Why is report_error called before
> send_error in one place but not the others [10]?
Explained below.
>
> 7. prepare_for_backup: Add a comment for handling errors from
> Logger::init as to why report_error is not called.
There is something fishy here. The original code reported ER_BACKUP_LOGGER_INIT
but Logger::init() never signals errors so it was dead code.
I think that:
- Logger::init() should detect and report errors to the caller.
- Logger::init() should log errors if detected - no need for caller to do it
because Logger has direct access to logging mechanisms...
I added @todo item to Logger::init() documentation and added "// logs errors"
comment at the call site.
>
> 8. I do not understand why you have changed code to assign return
> values to a local variable instead of using if-statements
> directly [11], [16], [18]. What is the value of that?
I explain my reasons below. The changes should not really be here but it would
save me some work if you let them in.
> If using such variables, please, be consistent wrt naming. Some
> places it is called ret, other places err. [20]
I'd rather keep it as it is - see below.
>
> 9. Is it OK to use different error codes for report_error and
> fatal_error for the same error? [12], [14], [15].
Yes, it is fine - I explain below in more detail.
>
> 10. Since setting m_error is encapsulated in a method (fatal_error), I
> think reading it should also be encapsulated. [17].
I'd rather keep it as it is - see below.
>
> 11. Add missing call of fatal_error? [21]
Yes, good catch!
>
> 12. Logger::write_message: Add a comment when declaring out on what is
> the difference between out and msg, and when each of them will be
> used.
Will do - good suggestion.
>
> 13. Private copy constructor has already been created. Should not
> provide implementation since we want compile error. [22]
OK, I'll remove the code from this patch (should merge in when I update the tree).
>
> 14. If changes wrt to calling bstream_next_chunk is not directly
> related to the purpose of this patch, I suggest leaving them
> out. [23]
I'd rather keep the changes here - see my arguments below.
>
> [X] refer to inline comments.
>
> --
> Øystein
>
> Rafal Somla wrote:
> > === modified file 'mysql-test/suite/backup/r/backup_views.result'
> > --- a/mysql-test/suite/backup/r/backup_views.result 2008-10-07
> 17:15:44 +0000
> > +++ b/mysql-test/suite/backup/r/backup_views.result 2008-11-12
> 11:20:04 +0000
> > @@ -277,10 +277,10 @@ DROP DATABASE bup_db2;
> > Restore database.
> > restore database with view dependency to other, non-existing db
> > RESTORE FROM 'bup_objectview1.bak';
> > -ERROR HY000: Could not restore view `bup_db1`.`v5`. Please check the
> view definition for possible missing dependencies.
> > +ERROR 42S02: Table 'bup_db2.t2' doesn't exist
> > DROP DATABASE bup_db1;
> > RESTORE FROM 'bup_objectview2.bak';
> > -ERROR HY000: Could not restore view `bup_db2`.`student_details`.
> Please check the view definition for possible missing dependencies.
> > +ERROR 42S02: Table 'bup_db1.t3' doesn't exist
>
> [2] The new error message is not as informative as the old one. I do
> not think this is a move in the right direction.
>
The main goal of this patch is to change error reporting infrastructure so that
the first reported error is sent to the client. This is the principle followed
in the whole server. For example consider this:
mysql> create procedure p() create view v2 as select * from t2;
Query OK, 0 rows affected (0.01 sec)
mysql> call p();
ERROR 1146 (42S02): Table 'test.t2' doesn't exist
The error message gives you no clue that the original problem was creating view v2.
Do you say that we should apply a different principle in the backup code and
then BUG#40303 should be cancelled?
If not, then I'm afraid there is not much we can do to improve informativeness
of the error message which user sees. It happens that the first error detected
and reported is during an attempt to access a non-existent table, and according
to the principle this error will be sent to the client. If we want something
else, we need a different principle. However, the situation is not that bad,
because BACKUP command will push additional error message on the error stack and
user can investigate it with SHOW ERRORS:
mysql> restore from 'db.bkp';
ERROR 1146 (42S02): Table 'test.t1' doesn't exist
mysql> show errors;
+-------+------+------------------------------------------------------------------------------------------------------+
| Level | Code | Message
|
+-------+------+------------------------------------------------------------------------------------------------------+
| Error | 1146 | Table 'test.t1' doesn't exist
|
| Error | 1678 | Could not restore view `db`.`v`. Please check the view
definition for possible missing dependencies. |
+-------+------+------------------------------------------------------------------------------------------------------+
This actually gives more information than the original code.
> > === modified file 'mysql-test/suite/backup/t/backup_errors.test'
> > --- a/mysql-test/suite/backup/t/backup_errors.test 2008-11-05
> 09:41:15 +0000
> > +++ b/mysql-test/suite/backup/t/backup_errors.test 2008-11-12
> 11:20:04 +0000
> > @@ -7,11 +7,14 @@
> > # TODO: When we know how to do that, check that the backup progress
> table
> > # contains appropriate rows when errors have been detected.
> >
> > +let $bdir= `SELECT @@backupdir`;
> > +let $ddir= `SELECT @@datadir`;
> > +
> > --disable_warnings
> > DROP DATABASE IF EXISTS adb;
> > DROP DATABASE IF EXISTS bdb;
> > --error 0,1
> > ---remove_file $MYSQLTEST_VARDIR/master-data/test.bak
> > +--remove_file $bdir/test.bak
> > --enable_warnings
> >
> > # non-existent backup archive
> > @@ -25,19 +28,27 @@ CREATE TABLE bdb.t1(a int) ENGINE=MEMORY
> > # invalid location
> > --error ER_BAD_PATH
> > BACKUP DATABASE adb TO '';
> > +--replace_column 2 #
> > +SHOW WARNINGS;
>
> [3] Do you really need mask the error code here? Should not error
> codes be pretty stable? Would it not be good to get a warning if
> someone does changes that changes the numbers of existing error codes?
Depends on whether we consider change of error codes a bug and want the test to
fail in this case or not. I personally think that there is no point in tracing
error code changes in our backup tests. Also, recently some work was spent on
transforming our test cases so that they are insensitive to error code values.
So my patch goes in line with this principle. If we want to revert this decision
and make them sensitive again, I suggest doing it as a separate task.
> > === modified file 'sql/backup/backup_info.cc'
> > --- a/sql/backup/backup_info.cc 2008-11-05 09:41:15 +0000
> > +++ b/sql/backup/backup_info.cc 2008-11-12 11:20:04 +0000
(cut)
> > @@ -301,7 +301,7 @@ Backup_info::Dep_node::get_key(const uch
> > @c find_backup_engine().
> > */
> > Backup_info::Backup_info(Backup_restore_ctx &ctx)
> > - :m_ctx(ctx), m_state(Backup_info::ERROR), native_snapshots(8),
> > + :m_log(ctx), m_thd(ctx.thd()), m_state(Backup_info::ERROR),
> native_snapshots(8),
>
> [5] I think this illustrates that making Backup_restore_ctx a subclass
> of Logger was a bad idea. Anyhow, if Backup_info is not supposed to
> deal with Backup_restore_ctx, I feel it is better to pass in Logger
> and THD as separate arguments to the constructor instead passing in a
> Backup_restore_ctx for Backup_info to pick sub-components.
>
I think I could find some arguments for tying Backup_info to the corresponding
Backup_restore_ctx instance on the interface level, and picking relevant parts
of the context on the implementation level, but it does not matter that much.
Thus I'll do the change here and in Restore_info ctor.
> [6] The above line is too long.
>
OK.
> > === modified file 'sql/backup/data_backup.cc'
> > --- a/sql/backup/data_backup.cc 2008-10-30 20:02:15 +0000
> > +++ b/sql/backup/data_backup.cc 2008-11-12 11:20:04 +0000
(cut)
> > @@ -448,9 +449,15 @@ int write_table_data(THD* thd, Backup_in
> >
> > Scheduler::Pump *p= new Scheduler::Pump(*i, s);
> >
> > - if (!p || !p->is_valid())
> > + if (!p)
> > {
> > - info.m_ctx.fatal_error(ER_OUT_OF_RESOURCES);
> > + log.report_error(ER_OUT_OF_RESOURCES);
> > + goto error;
> > + }
> > + if (!p->is_valid())
> > + {
> > + log.report_error(ER_BACKUP_CREATE_BACKUP_DRIVER,p->m_name);
> > + delete p;
> > goto error;
> > }
>
> [7] Currently, the only way that p is not valid is if allocation
> fails.
That is not true. The Scheduler::Pump constructor calls base Backup_pump
constructor which can report errors by setting state to backup_state::ERROR.
This can happen if bitmap_init() or Snapshot_info.get_backup_driver() fail. And
the latter can fail because it was not possible to create backup driver instance
or initialize it.
> Hence, OUT_OF_RESOURCES is more informative to a user than
> BACKUP_CREATE_BACKUP_DRIVER.
I'm puzzled. Clearly the original code gave less information than the current
one because it did not distinguish between the two problems that can be detected
here - either memory allocation failed, or there was some other problem during
creation of backup driver and the infrastructure required to use it. Thus the
current code gives to user the maximum of the information it possess.
> A user is given no clue why a backup
> driver could not be created.
>
You are right, but at least it does not give a false clue that the problem was
failed memory allocation.
To give even more precise information, we would have to increase the granularity
of error reporting inside the constructor. E.g., instead of setting state to
general ERROR value, constructor would have to set it to one of several error
values indicating the precise reason of the failure. Then the code would have to
interpret these values and log appropriate messages.
However, I think such change is outside the scope of this patch. This patch does
not change the original design of the code: we detect the fact that construction
of an object failed but do not detect the reason of this failure.
> > @@ -569,9 +576,12 @@ int write_table_data(THD* thd, Backup_in
> > int error= 0;
> > error= block_commits(thd, NULL);
> > if (error)
> > + {
> > + log.report_error(ER_BACKUP_VP_FAILED);
>
> [8] I do not feel this error gives much information about what is
> actually wrong. Is it possible to say something about when
> block_commits() fail and what the user could do about it?
>
I don't see any such thing and hence a general error message. I think, the
existence of a commit blocker should not be brought to the attention of the user
- it is a very low level implementation detail.
To increase the amount of information given, block_commits(...) should give more
information about the error (need to extend the interface) and the code above
should analyse it and give better message in user-relevant terms (if possible).
But again, this is not the scope of this patch. This patch improves the
situation a bit, because originally nothing was reported at all and now user
sees that BACKUP failed during VP creation, which *is* some information.
> > @@ -1371,11 +1377,25 @@ int restore_table_data(THD *thd, Restore
> > if (info.snap_count() == 0 || info.table_count() == 0) // nothing
> to restore
> > DBUG_RETURN(0);
> >
> > + Logger &log= info.m_log;
> > +
> > + /* Drv[n] points at restore driver used to process snapshot n. */
> > Restore_driver* drv[256];
> > + /*
> > + Active[n] is not NULL if driver drv[n] has been activated. Such
> driver needs
> > + an end() or cancel() call to shut it down properly.
> > + */
> > + Restore_driver* active[256];
>
> [9] A named constant should be used here instead of 256.
>
Not sure. The maximum number of allowable drivers is not a settable parameter.
It is fixed by the backup image format in which one byte is used to identify a
snapshot. This puts a fixed limit of max 256 snapshots inside backup image and
thus max 256 drivers used to process them.
But, since I see such suggestion for a second time, as a compromise I can define
a symbolic constants and document it explaining why it can not be changed.
> > === modified file 'sql/backup/kernel.cc'
> > --- a/sql/backup/kernel.cc 2008-10-30 20:02:15 +0000
> > +++ b/sql/backup/kernel.cc 2008-11-12 11:20:04 +0000
> > @@ -154,11 +154,8 @@ execute_backup_command(THD *thd, LEX *le
> > folders in the path could have been moved, deleted, etc.
> > */
> > if (backupdir->length() && my_access(backupdir->c_ptr(),
> (F_OK|W_OK)))
> > - {
> > - context.fatal_error(ER_BACKUP_BACKUPDIR, backupdir->c_ptr());
> > DBUG_RETURN(send_error(context, ER_BACKUP_BACKUPDIR,
> backupdir->c_ptr()));
> > - }
> > -
> > +
> > switch (lex->sql_command) {
> >
> > case SQLCOM_BACKUP:
> > @@ -195,7 +192,7 @@ execute_backup_command(THD *thd, LEX *le
> >
> > if (info->db_count() == 0)
> > {
> > - context.fatal_error(ER_BACKUP_NOTHING_TO_BACKUP);
> > + context.report_error(ER_BACKUP_NOTHING_TO_BACKUP);
> > DBUG_RETURN(send_error(context, ER_BACKUP_NOTHING_TO_BACKUP));
>
> [10] Why is report_error called here, but not for other errors in this
> method?
>
report_error() needs to be called only if the error was not already reported by
function or method called before, as is the case in this fragment.
Also, it is not good to call report_error() before
context.prepare_for_backup/restore(). Only after prepare_for_*() call, the
operation is started, backup logger is properly initialized and it makes sense
to log errors in the context of that operation. But, for example, my_access()
checks are done before the operation is even started, so the error is reported
only as a regular SQL error, not as error belonging to an ongoing backup/restore
operation.
> > @@ -496,22 +496,17 @@ int Backup_restore_ctx::prepare(String *
> > if (m_error)
> > return m_error;
> >
> > - // Prepare error reporting context.
> > -
> > - mysql_reset_errors(m_thd, 0); // Never errors
> > - m_thd->no_warnings_for_error= FALSE;
> > -
> > - save_errors(); // Never errors
> > -
> > + int ret= 0;
> >
> > /*
> > Check access for SUPER rights. If user does not have SUPER, fail
> with error.
> > +
> > + In case of error, we write only to backup logs, because
> check_global_access()
> > + pushes the same error on the error stack.
> > */
> > - if (check_global_access(m_thd, SUPER_ACL))
> > - {
> > - fatal_error(ER_SPECIFIC_ACCESS_DENIED_ERROR, "SUPER");
> > - return m_error;
> > - }
> > + ret= check_global_access(m_thd, SUPER_ACL);
> > + if (ret)
>
> [11] Why this change? What is better with assigning the return value
> to a local variable. I fell that complicates code since I need to be
> concerned with whether ret is only used in the line below or more
> times below. used
>
You are right - this change is not justified by the goal of the patch. Chuck
also pointed it out. It got it here because I was creating the patch from an
earlier prototype for dealing with interruption inside backup/restore command.
If check_global_access() is interrupted, then before we report error, we should
check if it is not in fact interruption. I.e., the code will look as follows:
ret= check_global_access(m_thd, SUPER_ACL);
if (<check for interruption>)
<handle the interruption>;
if (ret)
<no interruption - handle the error>;
So this change prepares for the future changes for BUG#35079/WL#4538. If you and
Chuck can accept this, then this can save me a substantial amount of work needed
for move these changes to the more appropriate patch.
> > @@ -658,12 +651,16 @@ Backup_restore_ctx::prepare_for_backup(S
> >
> > if (!info)
> > {
> > - fatal_error(ER_OUT_OF_RESOURCES);
> > + fatal_error(report_error(ER_OUT_OF_RESOURCES));
> > return NULL;
> > }
> >
> > if (!info->is_valid())
> > - return NULL; // Error has been logged by Backup_Info constructor
> > + {
> > + // Error has been logged by Backup_Info constructor
> > + fatal_error(ER_BACKUP_BACKUP_PREPARE);
>
> [12] So here fatal_error will register a different error from what is
> logged before. Which is reported to the user?
>
No, fatal_error(...) has been redefined and now it does not log anything. It
only moves context object to an error state by setting internal m_error variable
to the indicated error code. The error code stored in m_error is not currently
used - only the fact that m_error != 0 is detected in the code.
> > return NULL;
> > + }
> >
> > info->save_start_time(when);
> > m_catalog= info;
> >
> > + int ret;
> > +
> > /*
> > - Read catalogue from the input stream.
> > + Read header and catalogue from the input stream.
> > */
> >
> > - if (read_header(*info, *s))
> > - {
> > - fatal_error(ER_BACKUP_READ_HEADER);
> > - return NULL;
> > - }
> > -
> > - if (s->next_chunk() != BSTREAM_OK)
> > + ret= read_header(*info, *s); // Can log errors via callback
> functions.
> > + if (ret)
> > {
> > - fatal_error(ER_BACKUP_NEXT_CHUNK);
> > + if (!error_reported())
> > + report_error(ER_BACKUP_READ_HEADER);
> > + fatal_error(ret);
>
> [14] Why is potentially different error messages used here for
> reporr_error and fatal_error?
>
As explained above, the error code stored by fatal_error(..) does not really
matter. Thus we can store the exact code returned by read_header(..) so that
this information is not discarded in case it turns out to be handy to look at it.
> > @@ -1005,25 +965,19 @@ int Backup_restore_ctx::close()
> > Ignore ENOENT error since it is ok if the file doesn't exist.
> > */
> > if (res && my_errno != ENOENT)
> > - {
> > - error= ER_CANT_DELETE_FILE;
> > - }
> > + fatal_error(report_error(ER_CANT_DELETE_FILE, m_path.c_ptr(),
> my_errno));
> > }
> >
> > /* We report completion of the operation only if no errors were
> detected,
> > and logger has been initialized.
> > */
> > - if (!error)
> > + if (!m_error)
>
> [17] I suggest adding an access method for m_error.
>
This is a method of Backup_restore_ctx accessing one of its members - I don't
see a reason why it could not access it directly. On the other hand, the value
of m_error is not part of external interface of the context class. It is only an
internal flag indicating if the context object is in error state (if m_error !=
0) or not (if m_error == 0). The fatal_error() (which is private) is not here
for encapsulating m_error, but it is a helper method which contains the logic to
be executed when moving to error state (i.e., don't change m_error value if
already set).
> > @@ -1077,27 +1032,33 @@ int Backup_restore_ctx::do_backup()
> > DBUG_PRINT("backup",("Writing preamble"));
> > DEBUG_SYNC(m_thd, "backup_before_write_preamble");
> >
> > - if (write_preamble(info, s))
> > + ret= write_preamble(info, s); // Can Log errors via callback
> functions.
> > + if (ret)
> > {
> > - fatal_error(ER_BACKUP_WRITE_HEADER);
> > - DBUG_RETURN(m_error);
> > + if (!error_reported())
> > + report_error(ER_BACKUP_WRITE_HEADER);
> > + DBUG_RETURN(fatal_error(ret));
> > }
> >
> > DBUG_PRINT("backup",("Writing table data"));
> >
> > DEBUG_SYNC(m_thd, "before_backup_data");
> >
> > - if (write_table_data(m_thd, info, s)) // logs errors
> > - DBUG_RETURN(send_error(*this, ER_BACKUP_BACKUP));
> > + ret= write_table_data(m_thd, info, s); // logs errors
> > + if (ret)
> > + DBUG_RETURN(fatal_error(ret));
>
> [18] Same as [11]. In addition, in this method ret is used several
> times for different return values which I fear may create confusion.
>
And the same explanation. I don't understand your concern - do you suggest I
need to declare new variable each time I want to store a new value?
> > @@ -1225,17 +1183,12 @@ int Backup_restore_ctx::do_restore()
> >
> > disable_fkey_constraints(); // Never errors
> >
> > - if (read_meta_data(info, s))
> > - {
> > - m_thd->main_da.reset_diagnostics_area(); // Never errors
> > -
> > - fatal_error(ER_BACKUP_READ_META);
> > - DBUG_RETURN(m_error);
> > - }
> > -
> > - if (s.next_chunk() == BSTREAM_ERROR)
> > + err= read_meta_data(info, s); // Can log errors via callback
> functions.
> > + if (err)
>
> [20] Here you are calling it return variable err, other places ret.
> Please, be consistent.
>
I'm modifyng existing code. If err variable already exists, I use it. If it does
not, I create new one. I think name 'res' (or 'rc') is better than 'err' so I
don't want to name new variables 'err' only because some old code uses that name.
> > === modified file 'sql/backup/stream.h'
> > --- a/sql/backup/stream.h 2008-10-15 20:00:48 +0000
> > +++ b/sql/backup/stream.h 2008-11-12 11:20:04 +0000
(cut)
> > @@ -181,6 +179,10 @@ result_t
> > read_header(Image_info &info, Input_stream &s)
> > {
> > int ret= bstream_rd_header(&s,
> static_cast<st_bstream_image_header*>(&info));
> > +
> > + if (ret != BSTREAM_ERROR)
> > + ret= bstream_next_chunk(&s);
> > +
>
> [23] How is this change related to the purpose of this patch? If they
> are not, I think it is better to do this in another patch.
>
Not very much. However, what is wrong with this change? Do you think it is not
correct? Or do you think it is not a desirable change? Or, perhaps, you think
that it is big enough to justify a separate BUG/WL?
My position on this is that:
- it is correct,
- it is a change in a good direction (improving division of responsibilities
between sub-modules),
- it is simple enough to avoid wasting resources on a separate
roport/evaluate/impement/review cycle.
So, why not including it here?
Rafal
