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

[Rafal Somla]

Hi Øystein,

Let me present some ideas behind error reporting mechanisms used in online 
backup so that we can find satisfactory solution for the 1st request I had in my 
review.

Øystein Grøvlen wrote:
>> REQUESTS
>> --------
>>  1. Fix the problem with errors only partially logged by internal 
>> server functions (see below).
> 
> It seems to me that it would be better if backup/restore picked up the
> already logged error, than to log a new one.  I must admit that I do
> not quite like this logging at different levels.  It would be better
> if errors where reported back to some central "brain", e.g
> do_backup()/do_restore, where it was decided what to actually log.
> That would make easier to do consistent reporting.  Like it is now, I
> think it is very likely that same error is logged several times (using
> different error codes).

Perhaps it would be a better solution, but design decision was made to report 
errors at the place where they are detected, if possible. The only reason for 
such decision was, if I recall correctly, is to avoid designing and implementing 
a proper error reporting infrastructure which would be needed for the 
alternative approach (so that functions can return enough info about an error 
for the caller to make a right decision). Also, this is the design used in all 
the rest of the server code.

So, until things are changed, we are stuck with the current design where the 
central place for reporting errors is (should be) an instance of backup::Logger 
class passed down the call stack.

The backup::Logger class is independent from the standard error reporting 
mechanism used in the server, which I call the "error stack" (you can push 
errors and warning onto this per-connection stack). This separation is done 
intentionally, in preparation for the situation where BACKUP/RESTORE commands 
are run in a separate process which is not tied to any client connection. It was 
also done so that the backup module is not using server's error reporting 
infrastructure and thus is less tied to the rest of the server.

Note that when BACKUP/RESTORE are run asynchronously, the standard destination 
of error messages is different than for other SQL statements. Normally, errors 
detected during execution of an SQL statement are reported to the issuer of that 
statement who is waiting for it to terminate. The errors and warnings are 
collected on the error stack and at the end of command execution they are sent 
to the client.

For BACKUP statement, if run asynchronously, things are bit different. A user 
will say

 > BACKUP ... TO ...

and this will return immediately reporting only backup_id. But the backup 
process will be started in the background and if it detects any errors, they are 
not supposed to reach the normal error stack of the connection but instead 
should be send to backup log tables and other destinations. Note that it is even 
possible that the connection will be closed before backup process terminates.

This is why we have backup::Logger as primary place for reporting errors in 
backup code. Since currently BACKUP and RESTORE are synchronous, and user must 
wait for the whole process to terminate, we still push every error or warning 
reported via backup::Logger class on the error stack, so that user sees all the 
errors when the statement fails. But this should be considered a temporary 
solution which will be removed in the future.

Now we have this problem. In backup code we use some functions defined in the 
server, such as memory allocation routines or list class. Ideally, such 
functions should be isolated into a utility library but as it is now, they are 
integral part of the server and strongly tied with it. In particular they use 
server's error reporting infrastructure and will push error messages on the 
error stack. But in our context we don't want them to push anything on the error 
stack because we want to control what goes there (i.e. only the backup::Logger 
class should push error messages). My idea of how this should work is as follows:

<call utility function e.g. sql_alloc>
if (<error has been detected>)
{
   logger.report_error(APPROPRIATE_ERROR_CODE); // in particular, this will push
                                                // error on the error stack
}

If the utility function also pushes something on the error stack then we would 
have 2 messages for the same error. So we need to handle this issue somehow. I 
see the following alternatives:

a) Ignore the issue and allow multiple error messages. Note that this can make 
sense. For example user can see something like this:

   Error | Out of memory				// from sql_alloc
   Error | Could not allocate input buffer	// from backup code

So the additional error message will give extra information about the problem.

b) Remove error reported by the utility function from the error stack.

c) Disable error stack during execution of backup code. I think this is possible 
because what happens with errors is determined by function hooks. One can 
replace the hooks so that they don't push anything on the error stack but only 
set some global flag for detecting the error. The backup::Logger would store 
error messages internally, and at the end of execution, the error stack would be 
re-enabled and all stored messages would be pushed there.

What do you think about this?

Rafal