Hi Chuck,
Status: undecided.
Commentary:
The patch in itself is almost fine. I have few comments.
But please give me an evidence that the architecture is approved.
I have seen a bunch of suggestions, what the behavior could be. And if
to use a variable or RESTORE *statement* option, and how the variable or
option could be named.
Have we asked Product Management, Architects, and Support?
Personally, I like the RESTORE...ENABLE EVENTS approach most. And a
positive variable name (restore_enables_events, default off) second
best. Also I'm not happy with the reporting of disabled events.
But as a code reviewer I do not feel entitled to approve an architecture
implicitly through a patch approval.
Details:
Chuck Bell, 10.11.2009 22:20:
...
> 2889 Chuck Bell 2009-11-10
> BUG#37445 : Do not automatically enable restored events
>
> This bug is a feature request to disable events on restore.
>
> This patch implements a mechanism to disable all events on
> restore and communicate to the user which events were
> disabled. This list is written to the console, the warning
> stack, and the backup_progress log.
>
> The variable (option, global, sesson) restore_disables_events
> is ON by default but may be turned off. When turned OFF,
> events are not disabled on restore and they retain their
> original state from backup.
>
> Note: A number of option files were created to allow the
> older tests to run correctly and suppress the disabling of
> events on restore.
...
> === modified file 'mysql-test/suite/backup/r/backup_events.result'
...
> +SHOW EVENTS FROM bup_events;
> +Db Name Definer Time zone Type Execute at Interval value Interval
> field Starts Ends Status Originator character_set_client collation_connection Database
> Collation
>
> +bup_events e1 root@localhost SYSTEM RECURRING NULL 3 HOUR # # ENABLED 1 latin1 latin1_swedish_ci latin1_swedish_ci
>
> +bup_events e2 root@localhost SYSTEM RECURRING NULL 3 HOUR # # DISABLED 1 latin1 latin1_swedish_ci latin1_swedish_ci
>
> +bup_events e3 root@localhost SYSTEM RECURRING NULL 3 HOUR # # DISABLED 1 latin1 latin1_swedish_ci latin1_swedish_ci
>
> +bup_events e4 root@localhost SYSTEM RECURRING NULL 3 HOUR # # ENABLED 1 latin1 latin1_swedish_ci latin1_swedish_ci
> +#
> +# Perform backup for later use.
> +#
> +BACKUP DATABASE bup_events TO 'bup_events.bak';
...
> +# Test Case 4 : Ensure all events are disabled on restore when
> +# restore_disables_events = ON.
> +# While global variable set.
...
> +RESTORE FROM 'bup_events.bak' OVERWRITE;
> backup_id
> #
...
> +# # The event 'bup_events.e1' was disabled during restore.
> +# # The event 'bup_events.e2' was disabled during restore.
> +# # The event 'bup_events.e3' was disabled during restore.
> +# # The event 'bup_events.e4' was disabled during restore.
> +SHOW WARNINGS;
> +Level Code Message
> +Warning # The event 'bup_events.e1' was disabled during restore.
> +Warning # The event 'bup_events.e2' was disabled during restore.
> +Warning # The event 'bup_events.e3' was disabled during restore.
> +Warning # The event 'bup_events.e4' was disabled during restore.
There is a warning for every event restored. Including those that were
already disabled at BACKUP time. Frankly, I won't want to see these
warnings at all. But if so, then please those that were enabled at
BACKUP time.
I don't think, a change in image format would be required. After all we
have this magic metadata "extra" information. I would use this field as
a sequence of {type, length, info}. So it can be extended by other
"extra" info later.
...
> === modified file 'mysql-test/suite/backup/t/backup_events.test'
...
> +# This test tests the behavior of events during restore. Specifically, that
> +# the code obeys the variable restore_disables_events.
> #
> +# The following test cases are presented.
> +# 1 - The variable restore_disables_events is ON by default.
> +# 2 - The variable restore_disables_events obeys session and
> +# global behavior.
> +# 3 - Ensure all events are disabled on restore when
> +# restore_disables_events = ON.
> +# While session variable set.
> +# 4 - Ensure all events are disabled on restore when
> +# restore_disables_events = ON.
> +# While global variable set.
You probably know that setting a global variable does not influence
existing sessions, not even the one, which sets the global variable.
This means that a session *must not* obey the global variable, if a
session variable of the same name exists.
It seems, the patch introduces an exception for this variable. I hope,
you adhere to my suggestion below. Then this test and/or its heading
need a change.
...
> +--echo # Create database with tables and events.
...
> +CREATE DATABASE bup_events;
> +CREATE TABLE bup_events.t1 (a int);
> +CREATE TABLE bup_events.t2 (b int);
SQL keywords shall be upper case: INT
...
> === added file 'mysql-test/suite/backup/t/backup_logs-master.opt'
> --- a/mysql-test/suite/backup/t/backup_logs-master.opt 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/backup/t/backup_logs-master.opt 2009-11-10 21:19:56 +0000
> @@ -0,0 +1 @@
> +--disable-restore-disables-events
...
> === added file 'mysql-test/suite/backup/t/backup_objects-master.opt'
> --- a/mysql-test/suite/backup/t/backup_objects-master.opt 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/backup/t/backup_objects-master.opt 2009-11-10 21:19:56 +0000
> @@ -0,0 +1 @@
> +--disable-restore-disables-events
>
> === added file 'mysql-test/suite/backup/t/backup_security-master.opt'
> --- a/mysql-test/suite/backup/t/backup_security-master.opt 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/backup/t/backup_security-master.opt 2009-11-10 21:19:56 +0000
> @@ -0,0 +1 @@
> +--disable-restore-disables-events
>
> === added file 'mysql-test/suite/backup/t/backup_triggers_and_events-master.opt'
> --- a/mysql-test/suite/backup/t/backup_triggers_and_events-master.opt 1970-01-01
> 00:00:00 +0000
> +++ b/mysql-test/suite/backup/t/backup_triggers_and_events-master.opt 2009-11-10
> 21:19:56 +0000
> @@ -0,0 +1 @@
> +--disable-restore-disables-events
Every new opt file bears the risk of an extra server restart during the
test run. If the changes in the result file are not too manyfold, I'd
prefer to change the tests/results where reasonable. Or set the variable
in the .test file.
> === modified file 'sql/backup/kernel.cc'
...
> @@ -1487,6 +1487,20 @@ int Backup_restore_ctx::restore_triggers
...
> + /*
> + If either the global variable restore_disables_events is
> + ON or the session variable by the same name is ON, disable
> + this event.
> + */
> + if (m_thd->variables.restore_disables_events ||
> + global_system_variables.restore_disables_events)
Uhhh. This breaks the well established session/global variable relation.
A session won't be able to override the global setting any more. This
would require an extra paragraph in the reference manual for this
variable. You will be asked for a good reason for this exception.
What would be wrong in the usual behavior, just to check the session
variable? Is it really necessary that a running session inherits the
global value instantly? A new session inherits the global values on start.
...
> === modified file 'sql/backup/logger.cc'
...
> +/**
> + Report that an event was disabled on restore.
> +
> + This method writes a warning message to the console, the warning stack,
> + and the backup progress log to tell the user that a specific event was
> + disabled on restore.
What is "the console" here? I know a console as a dedicated terminal in
the data center, which receives boot-, shutdown-, hardware failure-, and
perhaps intrusion detection system messages. It is usually difficult for
an application to write there. Also won't it be much appreciated if
messages about disabled events go there.
> +
> + @param[in] db_name Name of the database for the even.
Typo even -> event
...
> === modified file 'sql/si_objects.cc'
...
> +bool disable_event(THD *thd,
> + const char *db_name,
> + const char *event_name,
> + bool disable)
> +{
...
> + /*
> + We update the mysql table directly to avoid changing character
> + collation or character set, modified by, etc.
But suddenly we require UPDATE_ACL on mysql.event.
...
> + /* Allow to execute DDL operations. */
> + ::obs::bml_exception_on(thd);
That would be required with ALTER EVENT, I guess. But it should not be
necessary with UPDATE. Likewise bml_exception_off().
...
Regards
Ingo
--
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Wolf Frenkel
Vorsitzender des Aufsichtsrates: Martin Häring HRB München 161028
