Re: bzr commit into mysql-6.0-backup branch (oystein.grovlen:2688) Bug#39089 WL#4384

[Rafal Somla]

Hi,

See some more explanations of my requests.

Øystein Grøvlen wrote:
>>  6. In Backup_restore_ctx::close() make sure that m_error is set as in 
>> the original code.
> 
> This part I do not quite understand.  I do not see how setting m_error
> would help.  As far as I can tell, the only thing that happens after
> close on failure, is that send_error() is called.  To affect
> send_error(), I think you need to register the error with the logger.
>

The intention is that non-zero error code in Backup_restore_ctx::m_error 
indicates that the context is in error state and can not continue normal 
operation. Usually, Backup_restore_ctx::fatal_error() method sets m_error to 
indicate error state. But in this case error is detected inside 
Backup_restore_ctx method and if it prevents further operation, m_error should 
be set. This is how the original code was working. My request is to preserve 
this behaviour.


>>> @@ -791,6 +782,7 @@ int Backup_restore_ctx::unlock_tables()
>>>   */  int Backup_restore_ctx::close()
>>
>> The original implementation of this method was setting m_error member 
>> which has a meaning - it indicates that the context object is in error 
>> state. Your patch changes this and I'm not convinced that this will 
>> have no bad effects.
>>
>>>  {
>>> +  int error= 0;
>>>    if (m_state == CLOSED)
>>>      return 0;
>>>  
>>> @@ -810,15 +802,10 @@ int Backup_restore_ctx::close()
>>>    }
>>>  
>>>    // unlock tables if they are still locked
>>> -
>>> -  // FIXME: detect errors if reported.
>>> -  unlock_tables();
>>> +  unlock_tables();                              // Never errors
>>>  
>>>    // unfreeze meta-data
>>> -
>>> -  // FIXME: detect errors if reported.
>>> -  // FIXME: error logging.
>>> -  obs::ddl_blocker_disable();
>>> +  obs::ddl_blocker_disable();                   // Never errors
>>>  
>>>    // restore thread options
>>>  
>>> @@ -826,10 +813,11 @@ int Backup_restore_ctx::close()
>>>  
>>>    // close stream
>>>  
>>> -  if (m_stream)
>>> -    // FIXME: detect errors if reported.
>>> -    // FIXME: error logging.
>>> -    m_stream->close();
>>> +  if (m_stream && !m_stream->close())
>>> +  {
>>> +    // Note error, but complete clean-up
>>> +    error= ER_BACKUP_CLOSE;
>>> +  }
>>>  
>>>    if (m_catalog)
>>>      m_catalog->save_end_time(when); // Note: no errors.
>>> @@ -861,19 +849,27 @@ int Backup_restore_ctx::close()
>>>       */
>>>      if (res && my_errno != ENOENT)
>>>      {
>>> -      report_error(ER_CANT_DELETE_FILE, m_path, my_errno);
>>> -      if (!m_error)
>>> -        m_error= ER_CANT_DELETE_FILE;
>>> +      error= ER_CANT_DELETE_FILE;
>>
>> Originally I was setting m_error to ER_CANT_DELETE_FILE only if no 
>> error was detected earlier. This is because in the situation when we 
>> have detected an error and then have another error while deleting the 
>> file, it is more important to report the first error IMO.
> 
> As I said, I do not think m_error will be picked up either.
> 

I'm not so much concerned whether it will be picked up or not. I just want to 
follow the (not explicitly stated) design requiring that m_error is non-zero 
whenever context is in error state. Whether it is/will be used by other code is 
another question.

Rafal