Rafal,
Since your patch is so much more complex and completely different from my
solution and you are against a simple change, I suggest you take ownership
of this bug and I review it. We're spending too much time on this (I've
spent most of the morning working on it and you have clearly done a lot of
work on it).
I have reviewed your patch. You would need to add the new test result files
that I included in my patch (see below). The only thing I don't like about
the patch (beside its complexity) is the error messages. I would prefer the
error messages TODO's be done now, but if recorded properly I concede they
could be done in WL#4190.
Speaking of WL#4190, I suggest we (re)design error handling the way the
architects suggested and simplify our warnings and errors. The lines between
logging and error handling are blurred. Simplification will make our lives
much easier later on and enrich the users' experience. Clearly this is not
something we need to do for the alpha release.
<commentary>
I understand your comments but why such a large patch when the problem can
be solved with only 3 lines of code? Doesn't this speak more to the design
of the logger class than anything else? What is wrong with the logger
pushing the errors and warnings? It's still a logging function. Adding the
pushes doesn't change that.
I don't understand: "a) and b), as they are not (will be not) available
during the process" -- how is that possible? Even if we decouple the logger
class from the server we will still need to communicate with the server's
error handling code. I think this abstraction is too complex and we need not
go so far. What is the goal of such complexity? What precept of OOD are we
trying to achieve? Devil's advocate question: Why should a logger for online
backup not log warnings and errors and pass them to the server?
</commentary>
Chuck
===== mysql-test/r/backup_no_data.result 1.6 vs edited =====
--- 1.6/mysql-test/r/backup_no_data.result 2007-12-20 15:32:15 -05:00
+++ edited/mysql-test/r/backup_no_data.result 2008-01-24 11:24:45 -05:00
@@ -64,6 +64,8 @@
BACKUP DATABASE empty_db TO 'empty_db.bak';
backup_id
#
+Warnings:
+# 1618 Skipping view v1 in database empty_db
SHOW DATABASES;
Database
information_schema
===== mysql-test/r/backup_no_engine.result 1.5 vs edited =====
--- 1.5/mysql-test/r/backup_no_engine.result 2007-12-14 19:44:58 -05:00
+++ edited/mysql-test/r/backup_no_engine.result 2008-01-24 11:24:45 -05:00
@@ -5,6 +5,8 @@
BACKUP DATABASE db TO "db.backup";
backup_id
#
+Warnings:
+# 1619 Skipping table db.t2 since it has no valid storage engine
DROP DATABASE db;
CREATE DATABASE db;
RESTORE FROM "db.backup";
> -----Original Message-----
> From: Rafal Somla [mailto:rsomla@stripped]
> Sent: Thursday, January 24, 2008 8:05 AM
> To: cbell@stripped
> Cc: commits@stripped; Backup development list
> Subject: Re: bk commit into 6.0 tree (cbell:1.2784) BUG#33562
>
> Hi Chuck,
>
> I want to propose a different solution to the same problem.
>
> My rationale is that I want to separate error reporting in
> the BACKUP/RESTORE command from logging errors in the error
> log and progress table via Logger class.
>
> Generally there are several destinations where error or
> warning message can end up:
>
> a) error response from BACKUP/RESTORE command,
> b) SQL connection's error/warning stack which can be examined
> with SHOW ERRORS/WARNINGS,
> c) error log of the server,
> d) backup progress table (or equivalent),
> e) debug trace of the server (if enabled).
>
> Now, if we execute backup/restore operation asynchronously in
> a separate thread, then destinations a) and b) can not be
> used. The BACKUP/RESTORE command will usually return
> immediately without any errors and then, if error happens
> later, it should be reported to destinations c-e).
>
> My intention behind the Logger class was to make it a general
> interface to all online backup logging facilities. If all
> backup code consistently uses Logger methods to report
> errors, warnings and other messages, then they all should be
> reported in the same way and it is easy to change the way
> they are reported.
>
> Since Logger is supposed to be a general class for reporting
> during the whole backup/restore process, it can not report to
> destinations a) and b), as they are not (will be not)
> available during the process. This is why
> execute_backup_command() uses function report_errors() whose
> task is to look at errors reported during backup/restore
> operation and stored inside the logger and send them to the client.
>
> Report_errors() doesn't do its job very well and this is why
> we have this bug.
> To fix it, I propose to not change the Logger class, but the
> report_errors() function to do what it is supposed to do,
> including pushing errors and warnings if there are many of
> them. Actually few more things need to be fixed to make
> everything work as I imagined it. I prepared a patch
> introducing necessary changes - it should hopefully explain
> more what solution I have in mind. Please have a look and
> tell me if you agree with it.
>
> Rafal
>
>
> cbell@stripped wrote:
> > Below is the list of changes that have just been committed into a
> > local 6.0 repository of cbell. When cbell does a push these changes
> > will be propagated to the main repository and, within 24
> hours after
> > the push, to the public repository.
> > For information on how to access the public repository see
> > http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
> >
> > ChangeSet@stripped, 2008-01-23 13:57:48-05:00,
> cbell@mysql_cab_desk. +4 -0
> > BUG#33562 : Backup: no warning about disabled storage engine
> >
> > Added calls to save messages so that the SHOW ERRORS and
> > SHOW WARNINGS commands will present the errors and
> > warnings to the client properly.
> >
> > mysql-test/r/backup_no_data.result@stripped, 2008-01-23
> 13:57:37-05:00, cbell@mysql_cab_desk. +2 -0
> > BUG#33562 : Backup: no warning about disabled storage engine
> >
> > New result file
> >
> > mysql-test/r/backup_no_engine.result@stripped, 2008-01-23
> 13:57:37-05:00, cbell@mysql_cab_desk. +2 -0
> > BUG#33562 : Backup: no warning about disabled storage engine
> >
> > New result file
> >
> > sql/backup/kernel.cc@stripped, 2008-01-23 13:57:38-05:00,
> cbell@mysql_cab_desk. +2 -0
> > BUG#33562 : Backup: no warning about disabled storage engine
> >
> > Added call to reset messages when backup or restore starts.
> >
> > sql/backup/logger.cc@stripped, 2008-01-23 13:57:39-05:00,
> cbell@mysql_cab_desk. +4 -0
> > BUG#33562 : Backup: no warning about disabled storage engine
> >
> > Added calls to save messages so that the SHOW ERRORS and
> > SHOW WARNINGS commands will present the errors and
> > warnings to the client properly.
> >
>
