Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2731) Bug#40303 Bug#40304

[Rafal Somla]

Hi Chuck,

thank you for the review. See my replies below and expect a new, updated patch 
from me soon.

Chuck Bell wrote:
> STATUS
> ------
> Changes requested
> 
> REQUESTS
> --------
> 1. Add comments stated in patch comments concerning fatal_error() to the
> doxygen comments for the method.

Good idea - I'll try to improve documentation of fatal_error() and m_error member.

> 
> 2. Please restore the { } around the debug return macro. This could cause
> compilation issues on some platforms.
> 

I'd rather keep the changes. First, using { } around single statement violates 
our coding guidelines. Second, I don't see it done in other code except, 
perhaps, the one you wrote. For example, see how DBUG_RETURN is used in 
sql/mysqld.cc.

> 3. I see an example of where including refactoring in this patch has left an
> inconsistency. The changes to the tests to use @backupdir and @datadir are
> warranted (but outside of the scope of the bug report) but do not change all
> tests in the backup and backup_engines suite. Please open a bug report
> requesting all tests be rewritten using this technique.

I can open new bug, but I don't understand why you object against moving the 
code in a good direction already in this patch - this will only mean less work 
for the person who will work on the new bug.

The occasion for the changes is that I modify one of the test, and while doing 
this I spot possible (minor) improvement and thus implemented it. If it is good, 
not very complex and improves things, why should you object. I don't think we 
should be hindered by our administrative processes when introducing small and 
obvious improvements to the code.

> 
> 4. I don't understand the need to change/reformat the code in the following
> manner:
> 
> if(something())
> {
>   return(error);
> }
> 
> to
> 
> ret= something()
> if(ret)
>   return(error);
> 
> If this is a style issue or developer preference please refrain from making
> these sort of changes. While mundane, changes like these can be the source
> of errors. This is done several places in the code. See details for
> examples.

No this is not a style issue. I agree that these changes are not justified in 
this patch. They landed here for "technical reasons". This is how I explained it 
for Oystein:

"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 
to move these changes to the more appropriate patch."

> 
> 5. Why is the assertion for m_log removed? Don't we need that?
> 

Because it is replaced by if(m_log) check. Thus now it is OK to have m_log==NULL 
as was the original intention.

> 6. Why is the report_error() not used in the patch fragment in kernel.cc?

Explained below.

> 
> COMMENTARY
> ----------
> It's a good patch. I have very little objections to the code in the patch
> that is within the scope of the bug report, but I feel there are too many
> extra coding bits added thereby increasing the risk of this patch
> unnecessarily.
> 
> SUGGESTIONS
> -----------
> 7. Don't include refactoring or other reorganization of the code where the
> changes are not within the scope of the bug report. There are several places
> where this takes place in this patch and I do not see the justification for
> it. I will not reject the patch on this point this time, but in the future
> please do not include changes that are out of the scope of the bug report --
> they introduce risk and complicate an already large patch. I suggest making
> either separate patches and/or new bug reports to handle these sort of
> changes. 

Perhaps we should discuss it to settle on a common procedure. I think it is OK 
to add refactoring to the code touched in the patch provided that:

  - it is an obvious improvement,
  - it is correct,
  - change is small enough to make reviewers reasonably sure that it does not
    introduce any unwanted "side effects"

If the change is not obviously for better or it looks too complex, then I agree 
that a separate BUG/WL should be started to allow design discussions and proper 
review cycle. But doing it in each case would be a waste of precious resources IMO.

> 
> DETAILS
> ------- 
>>  2731 Rafal Somla	2008-11-12
>>       BUG#40303 (Backup: Wrong error send to client if 
>> several errors reported)
>>       BUG#40304 (Backup: Errors reported during BACKUP show 
>> as warnings on the 
>>                  error stack)
>>       
>>       Before: Errors reported using backup::Logger class 
>> could be saved and in 
>>       case several errors are reported, the last one was sent 
>> to client as an
>>       error reply from BACKUP/RESTORE command.
>>       
>>       After: Errors reported with backup::Logger are reported 
>> to the server 
>>       using my_printf_error(..) function. This function 
>> pushes error on the 
>>       error stack (unles told otherwise) and stores the first 
>> reported error
>>       so that it is send to the client.
>>       
>>       Additionally.
>>       
>>       - Function log_error(..) is moved to the Logger class.
>>       
>>       - Code is changed do consistently use 
>> Logger::report/log_error() for error 
>>       reporting.
>>       
>>       - Semantics of Backup_restore_ctx::fatal_error() has 
>> been changed to 
>>       discourage its use as an error reporting facility. Now 
>> it is an internal helper 
>>       method which moves the context object into error state. 
>> It is used only by the 
>>       class methods and it is made private. The idea is that 
>> context object should 
>>       move into error state only as a result of executing one 
>> of its methods (not 
>>       externally).
> 
> [1] This should be added to code doxygen comments.
> 

Yes, good idea.

> 
>>   sql/backup/kernel.cc
>>     - Use explicit calls to report/log_error(..) for error 
>> logging - now fatal_error(..)
>>     does not log anything.
>>     - Simplified send_error(..) implementation - now it does 
>> not have to look for the
>>     last error reported. The first reported error is 
>> automatically stored by the 
>>     server and send to the client.
>>     - Move error reporting context preparations to Logger::init(..).
>>     - To clarify the code, calls to s->next_chunk() have been 
>> moved to wrapper 
>>     functions read_header(..), read_catalog(..) and 
>> read_meta_data(..) where they 
>>     really belong.
> 
> [7] I think this is out of scope of the patch. Nothing in the bug report or
> subsequent comments mentions the need for this change. I see the need for
> refactoring, but this doesn't improve the patch per the bug report. Indeed,
> it adds risk and therefore the potential of introducing more bugs. Please
> refrain from doing this sort of thing in the same patch as a bug report.
> 

When including this change, I thought it is too simple to justify using our 
resources on a separate report/evaluate/fix/review cycle. Do you see any 
concrete risks connected with it?

> ...
> 
>> === 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`;
> 
> [3] This sort of refactoring has left the tests in an inconsistent state --
> some use this while others still default to assuming backupdir == datadir.
> Please open a bug report to have all tests changed to use this technique.
> 

Keeping these changes will only help the person working on the new bug.

>> @@ -1072,9 +1080,7 @@ int Backup_pump::begin()
>>    if (ERROR == m_drv->begin(m_bw.buf_size))
>>    {
>>      state= backup_state::ERROR;
>> -    // We check if logger is always setup. Later the assertion can
>> -    // be replaced with "if (m_log)"
>> -    DBUG_ASSERT(m_log);
> 
> [5] Why was this removed (it happens other places too)? Don't we need it?
> 

See "if(m_log)" added below. The idea is that Backup_pump will log errors if it 
has a logger and will not do it if no logger is available.

>> +    if (m_log)
>>        m_log->report_error(ER_BACKUP_INIT_BACKUP_DRIVER, m_name);
>>      return ERROR;
>>    }

>> === 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()));
>> -  }
>> -
> 
> [6] Why isn't report_error() called here like in the other places (see
> below)?
>

This is because checks on backupdir are done before the backup/restore operation 
even starts (i.e., before a call to context.prepare_for_backup/restore). Then it 
is not good to report this error in the context of such an operation. Nothing 
will be logged to the backup logs, because the new backup_id has not been 
assigned yet. Server's error log will contain a confusing ER_BACKUP_BACKUPDIR 
message without a leading message informing that a backup/restore operation has 
been started.

Note that similar principle applies when checking user privileges. If user does 
not have SUPER privilege, he sees error from BACKUP/RESTORE statement, but 
nothing is logged via backup logger, because no operation has been started.

>> @@ -1134,9 +1095,7 @@ int Backup_restore_ctx::restore_triggers
>>  
>>    Image_info::Iterator *dbit= m_catalog->get_dbs();
>>    if (!dbit)
>> -  {
>> -    DBUG_RETURN(fatal_error(ER_OUT_OF_RESOURCES));
>> -  }
>> +    DBUG_RETURN(fatal_error(report_error(ER_OUT_OF_RESOURCES)));
> 
> [2] Removing the { } from around the macro can (and has) caused compilation
> errors (or was it warnings?) on some platforms. Please restore these.
> 

There are many places in the code (outside backup/ dir) where { } are not used, 
so if we get compilation errors because of this, all the other teams should see 
them as well. In that case, the solution would be to fix DBUG_RETURN macro I guess.

Rafal